Re: [PATCH] ipmi: fix suspicious RCU usage warning

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/12/25 18:29, Paul E. McKenney wrote:
> On Wed, Mar 12, 2025 at 01:19:32PM -0400, Rik van Riel wrote:
>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
>> index 1e5313748f8b..a2823763fd37 100644
>> --- a/drivers/char/ipmi/ipmi_msghandler.c
>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
>> @@ -1235,7 +1235,7 @@ int ipmi_create_user(unsigned int          if_num,
>>   		return -ENOMEM;
>>   
>>   	index = srcu_read_lock(&ipmi_interfaces_srcu);
>> -	list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
>> +	list_for_each_entry_srcu(intf, &ipmi_interfaces, link) {
> 
> Doesn't the above line want to be something like this?
> 
> +	list_for_each_entry_srcu(intf, &ipmi_interfaces, link,
> 				 srcu_read_lock_held(&ipmi_interfaces_srcu)) {

Ouch what a mess.  There are multiple occurrences of this,
almost all susceptible to the same warning.

I'd start with:

-#define ipmi_interfaces_mutex_held() \
-	lockdep_is_held(&ipmi_interfaces_mutex)
+#define for_each_ipmi_interface(intf) \
+	list_for_each_entry_srcu(intf, &ipmi_interfaces, link,
+				 srcu_read_lock_held(&ipmi_interfaces_srcu)
+				 || lockdep_is_held(&ipmi_interfaces_mutex)) {

and use the for_each_ipmi_interface() macro throughout the file.

Here is the list... all of them are using _rcu, plus:

- ipmi_smi_watcher_register is using the wrong lockdep_is_held() assertion,
   but would warn if fixed
- ipmi_add_smi is using _rcu but otherwise correct
- ipmi_get_smi_info is using _rcu and can warn
- ipmi_timeout is using _rcu and can warn
- panic_event is using _rcu and can warn, and is also not
   using any protection around the walk.  Taking srcu_read_lock
   would be much better


On top of this, intf->users_srcu never does a synchronize_srcu, so I'm
a bit at a loss at how it is protecting the list.  The safest change
here is probably:

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index f2a56c624f54..dc8936254c1b 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3769,12 +3769,12 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
  	intf->in_shutdown = true;
  	list_del_rcu(&intf->link);
  	mutex_unlock(&ipmi_interfaces_mutex);
-	synchronize_srcu(&ipmi_interfaces_srcu);
  
  	/* At this point no users can be added to the interface. */
  
  	device_remove_file(intf->si_dev, &intf->nr_msgs_devattr);
  	device_remove_file(intf->si_dev, &intf->nr_users_devattr);
+	synchronize_srcu(&ipmi_interfaces_srcu);
  
  	/*
  	 * Call all the watcher interfaces to tell them that

... plus replacing all uses of intf->users_srcu with ipmi_interfaces_srcu.


A couple more issues:

- in handle_read_event_rsp() there's a lone rcu_read_unlock()
that should become srcu_read_unlock() (currently for intf->users_srcu;
modulo changes like the above)

- while the intf->cmd_rcvrs list is protected by regular RCU,
there are many other occurrences of rcu_read_lock(), typically
followed by

         if (intf->in_shutdown) {
                 rv = -ENODEV;
                 goto out_err;
         }

and I think they should use interfaces_srcu instead.

Paolo

>>   		if (intf->intf_num == if_num)
>>   			goto found;
>>   	}
>> -- 
>> 2.48.1
>>
>