| Message ID | 20231013121029.353351-5-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b5f4e371336a62a48f6ae51abb8366e968a8f88f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: fix a deadlock when taking devlink instance lock while holding RTNL lock | expand |
On Fri, Oct 13, 2023 at 02:10:26PM +0200, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Lockdep reports following issue: > > WARNING: possible circular locking dependency detected > ------------------------------------------------------ > devlink/8191 is trying to acquire lock: > ffff88813f32c250 (&devlink->lock_key#14){+.+.}-{3:3}, at: devlink_rel_devlink_handle_put+0x11e/0x2d0 > > but task is already holding lock: > ffffffff8511eca8 (rtnl_mutex){+.+.}-{3:3}, at: unregister_netdev+0xe/0x20 > > which lock already depends on the new lock. > > the existing dependency chain (in reverse order) is: > > -> #3 (rtnl_mutex){+.+.}-{3:3}: ... > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(rtnl_mutex); > lock(mlx5_intf_mutex); > lock(rtnl_mutex); > lock(&devlink->lock_key#14); > > Problem is taking the devlink instance lock of nested instance when RTNL > is already held. > > To fix this, don't take the devlink instance lock when putting nested > handle. Instead, rely on the preparations done by previous two patches > to be able to access device pointer and obtain netns id without devlink > instance lock held. > > Fixes: c137743bce02 ("devlink: introduce object and nested devlink relationship infra") > Signed-off-by: Jiri Pirko <jiri@nvidia.com> Reviewed-by: Simon Horman <horms@kernel.org>
diff --git a/net/devlink/core.c b/net/devlink/core.c index c47c9e6c744f..655903ddbdfd 100644 --- a/net/devlink/core.c +++ b/net/devlink/core.c @@ -183,9 +183,8 @@ static struct devlink_rel *devlink_rel_find(unsigned long rel_index) DEVLINK_REL_IN_USE); } -static struct devlink *devlink_rel_devlink_get_lock(u32 rel_index) +static struct devlink *devlink_rel_devlink_get(u32 rel_index) { - struct devlink *devlink; struct devlink_rel *rel; u32 devlink_index; @@ -198,16 +197,7 @@ static struct devlink *devlink_rel_devlink_get_lock(u32 rel_index) xa_unlock(&devlink_rels); if (!rel) return NULL; - devlink = devlinks_xa_get(devlink_index); - if (!devlink) - return NULL; - devl_lock(devlink); - if (!devl_is_registered(devlink)) { - devl_unlock(devlink); - devlink_put(devlink); - return NULL; - } - return devlink; + return devlinks_xa_get(devlink_index); } int devlink_rel_devlink_handle_put(struct sk_buff *msg, struct devlink *devlink, @@ -218,11 +208,10 @@ int devlink_rel_devlink_handle_put(struct sk_buff *msg, struct devlink *devlink, struct devlink *rel_devlink; int err; - rel_devlink = devlink_rel_devlink_get_lock(rel_index); + rel_devlink = devlink_rel_devlink_get(rel_index); if (!rel_devlink) return 0; err = devlink_nl_put_nested_handle(msg, net, rel_devlink, attrtype); - devl_unlock(rel_devlink); devlink_put(rel_devlink); if (!err && msg_updated) *msg_updated = true;