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
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
