Upstream commit c2368b19807a ("net: devlink: introduce "unregistering" mark and use it during devlinks iteration") in v6.0 introduced a race when unregistering a devlink instance that can result in RCU stalls and in the system completely locking up. Exact details and reproducer can be found here [1]. The bug was inadvertently fixed in v6.3 by upstream commit d77278196441 ("devlink: bump the instance index directly when iterating").
This patchset fixes the bug by backporting the second commit and a related dependency from v6.3 to v6.1.y while adjusting them to the devlink file structure in v6.1.y (net/devlink/{core.c,devl_internal.h} -> net/devlink/leftover.c).
Tested by running the devlink tests under tools/testing/selftests/drivers/net/netdevsim/ and the reproducer mentioned in [1].
[1] https://lore.kernel.org/stable/20241001112035.973187-1-idosch@nvidia.com/
Jakub Kicinski (2): devlink: drop the filter argument from devlinks_xa_find_get devlink: bump the instance index directly when iterating
net/devlink/leftover.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
From: Jakub Kicinski kuba@kernel.org
commit 8861c0933c78e3631fe752feadc0d2a6e5eab1e1 upstream.
Looks like devlinks_xa_find_get() was intended to get the mark from the @filter argument. It doesn't actually use @filter, passing DEVLINK_REGISTERED to xa_find_fn() directly. Walking marks other than registered is unlikely so drop @filter argument completely.
Reviewed-by: Jiri Pirko jiri@nvidia.com Reviewed-by: Jacob Keller jacob.e.keller@intel.com Signed-off-by: Jakub Kicinski kuba@kernel.org [ Ido: Moved the changes from core.c and devl_internal.h to leftover.c ] Stable-dep-of: d77278196441 ("devlink: bump the instance index directly when iterating") Signed-off-by: Ido Schimmel idosch@nvidia.com --- net/devlink/leftover.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 032c7af065cd..68210b5fab8e 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -289,7 +289,7 @@ void devl_unlock(struct devlink *devlink) EXPORT_SYMBOL_GPL(devl_unlock);
static struct devlink * -devlinks_xa_find_get(struct net *net, unsigned long *indexp, xa_mark_t filter, +devlinks_xa_find_get(struct net *net, unsigned long *indexp, void * (*xa_find_fn)(struct xarray *, unsigned long *, unsigned long, xa_mark_t)) { @@ -322,30 +322,25 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, xa_mark_t filter, }
static struct devlink *devlinks_xa_find_get_first(struct net *net, - unsigned long *indexp, - xa_mark_t filter) + unsigned long *indexp) { - return devlinks_xa_find_get(net, indexp, filter, xa_find); + return devlinks_xa_find_get(net, indexp, xa_find); }
static struct devlink *devlinks_xa_find_get_next(struct net *net, - unsigned long *indexp, - xa_mark_t filter) + unsigned long *indexp) { - return devlinks_xa_find_get(net, indexp, filter, xa_find_after); + return devlinks_xa_find_get(net, indexp, xa_find_after); }
/* Iterate over devlink pointers which were possible to get reference to. * devlink_put() needs to be called for each iterated devlink pointer * in loop body in order to release the reference. */ -#define devlinks_xa_for_each_get(net, index, devlink, filter) \ - for (index = 0, \ - devlink = devlinks_xa_find_get_first(net, &index, filter); \ - devlink; devlink = devlinks_xa_find_get_next(net, &index, filter)) - #define devlinks_xa_for_each_registered_get(net, index, devlink) \ - devlinks_xa_for_each_get(net, index, devlink, DEVLINK_REGISTERED) + for (index = 0, \ + devlink = devlinks_xa_find_get_first(net, &index); \ + devlink; devlink = devlinks_xa_find_get_next(net, &index))
static struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs)
From: Jakub Kicinski kuba@kernel.org
commit d772781964415c63759572b917e21c4f7ec08d9f upstream.
xa_find_after() is designed to handle multi-index entries correctly. If a xarray has two entries one which spans indexes 0-3 and one at index 4 xa_find_after(0) will return the entry at index 4.
Having to juggle the two callbacks, however, is unnecessary in case of the devlink xarray, as there is 1:1 relationship with indexes.
Always use xa_find() and increment the index manually.
Signed-off-by: Jakub Kicinski kuba@kernel.org Reviewed-by: Jiri Pirko jiri@nvidia.com Signed-off-by: David S. Miller davem@davemloft.net [ Ido: Moved the changes from core.c and devl_internal.h to leftover.c ] Signed-off-by: Ido Schimmel idosch@nvidia.com --- net/devlink/leftover.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 68210b5fab8e..f1c6bd727a83 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -289,15 +289,13 @@ void devl_unlock(struct devlink *devlink) EXPORT_SYMBOL_GPL(devl_unlock);
static struct devlink * -devlinks_xa_find_get(struct net *net, unsigned long *indexp, - void * (*xa_find_fn)(struct xarray *, unsigned long *, - unsigned long, xa_mark_t)) +devlinks_xa_find_get(struct net *net, unsigned long *indexp) { - struct devlink *devlink; + struct devlink *devlink = NULL;
rcu_read_lock(); retry: - devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); + devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); if (!devlink) goto unlock;
@@ -306,31 +304,20 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, * This prevents live-lock of devlink_unregister() wait for completion. */ if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING)) - goto retry; + goto next;
- /* For a possible retry, the xa_find_after() should be always used */ - xa_find_fn = xa_find_after; if (!devlink_try_get(devlink)) - goto retry; + goto next; if (!net_eq(devlink_net(devlink), net)) { devlink_put(devlink); - goto retry; + goto next; } unlock: rcu_read_unlock(); return devlink; -} - -static struct devlink *devlinks_xa_find_get_first(struct net *net, - unsigned long *indexp) -{ - return devlinks_xa_find_get(net, indexp, xa_find); -} - -static struct devlink *devlinks_xa_find_get_next(struct net *net, - unsigned long *indexp) -{ - return devlinks_xa_find_get(net, indexp, xa_find_after); +next: + (*indexp)++; + goto retry; }
/* Iterate over devlink pointers which were possible to get reference to. @@ -338,9 +325,7 @@ static struct devlink *devlinks_xa_find_get_next(struct net *net, * in loop body in order to release the reference. */ #define devlinks_xa_for_each_registered_get(net, index, devlink) \ - for (index = 0, \ - devlink = devlinks_xa_find_get_first(net, &index); \ - devlink; devlink = devlinks_xa_find_get_next(net, &index)) + for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)
static struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs)
On Tue, Oct 15, 2024 at 02:36:23PM +0300, Ido Schimmel wrote:
Upstream commit c2368b19807a ("net: devlink: introduce "unregistering" mark and use it during devlinks iteration") in v6.0 introduced a race when unregistering a devlink instance that can result in RCU stalls and in the system completely locking up. Exact details and reproducer can be found here [1]. The bug was inadvertently fixed in v6.3 by upstream commit d77278196441 ("devlink: bump the instance index directly when iterating").
This patchset fixes the bug by backporting the second commit and a related dependency from v6.3 to v6.1.y while adjusting them to the devlink file structure in v6.1.y (net/devlink/{core.c,devl_internal.h} -> net/devlink/leftover.c).
Tested by running the devlink tests under tools/testing/selftests/drivers/net/netdevsim/ and the reproducer mentioned in [1].
[1] https://lore.kernel.org/stable/20241001112035.973187-1-idosch@nvidia.com/
Jakub Kicinski (2): devlink: drop the filter argument from devlinks_xa_find_get devlink: bump the instance index directly when iterating
net/devlink/leftover.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
-- 2.47.0
Both now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Ido Schimmel