The shaper insertion and update code takes xa_lock() explicitly. Paolo explained that the locking was purely to avoid re-taking the lock in loops. But it may be mis-read as if it was expecting readers to be fenced off by xa_lock. Readers of XArray are purely under RCU. Remove explicit taking of xa_lock(). All writers to hierarchy->shapers are serialized by the netdev instance lock (or run after netdev is made inaccessible to readers). Signed-off-by: Jakub Kicinski --- net/shaper/shaper.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index dea9270f3e57..a5b42b697a93 100644 --- a/net/shaper/shaper.c +++ b/net/shaper/shaper.c @@ -429,7 +429,6 @@ static void net_shaper_commit(struct net_shaper_binding *binding, int index; int i; - xa_lock(&hierarchy->shapers); for (i = 0; i < nr_shapers; ++i) { index = net_shaper_handle_to_index(&shapers[i].handle); @@ -442,7 +441,6 @@ static void net_shaper_commit(struct net_shaper_binding *binding, /* ... publish to lockless readers. */ smp_store_release(&cur->valid, true); } - xa_unlock(&hierarchy->shapers); } /* Rollback all the tentative inserts from the hierarchy. */ @@ -455,14 +453,12 @@ static void net_shaper_rollback(struct net_shaper_binding *binding) if (!hierarchy) return; - xa_lock(&hierarchy->shapers); xa_for_each(&hierarchy->shapers, index, cur) { if (cur->valid) continue; - __xa_erase(&hierarchy->shapers, index); + xa_erase(&hierarchy->shapers, index); kfree_rcu(cur, rcu); } - xa_unlock(&hierarchy->shapers); } static int net_shaper_parse_handle(const struct nlattr *attr, @@ -1472,12 +1468,10 @@ static void net_shaper_flush(struct net_shaper_binding *binding) if (!hierarchy) return; - xa_lock(&hierarchy->shapers); xa_for_each(&hierarchy->shapers, index, cur) { - __xa_erase(&hierarchy->shapers, index); + xa_erase(&hierarchy->shapers, index); kfree(cur); } - xa_unlock(&hierarchy->shapers); kfree(hierarchy); } -- 2.54.0 If we fail to insert a node into the XArray in net_shaper_pre_insert() we can free it directly - it was never visible to the RCU readers. Signed-off-by: Jakub Kicinski --- net/shaper/shaper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index a5b42b697a93..33958462e5e9 100644 --- a/net/shaper/shaper.c +++ b/net/shaper/shaper.c @@ -406,7 +406,7 @@ static int net_shaper_pre_insert(struct net_shaper_binding *binding, prev = xa_store(&hierarchy->shapers, index, cur, GFP_KERNEL); if (xa_err(prev)) { NL_SET_ERR_MSG(extack, "Can't insert shaper into device store"); - kfree_rcu(cur, rcu); + kfree(cur); ret = xa_err(prev); goto free_id; } -- 2.54.0 We keep getting misguided patches to fix the flush. Add a comment. Signed-off-by: Jakub Kicinski --- net/shaper/shaper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index 33958462e5e9..5a3b44c5d10f 100644 --- a/net/shaper/shaper.c +++ b/net/shaper/shaper.c @@ -1470,6 +1470,10 @@ static void net_shaper_flush(struct net_shaper_binding *binding) xa_for_each(&hierarchy->shapers, index, cur) { xa_erase(&hierarchy->shapers, index); + /* No need to use kfree_rcu(), netdev is already unpublished, + * and synchronize_rcu() has been run as part of + * unregister_netdevice(). + */ kfree(cur); } -- 2.54.0 Various AI scan tools may complain that we don't support resuming the cap dump. This is true, but the cap dumps are tiny. net_shaper_nl_cap_pre_dumpit() sets up the dump for just one device, so the size of the dump scales with NET_SHAPER_SCOPE_MAX (3). We don't expect them to ever need more than a 4kB page. Document this. Signed-off-by: Jakub Kicinski --- net/shaper/shaper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index 5a3b44c5d10f..b65b356da16b 100644 --- a/net/shaper/shaper.c +++ b/net/shaper/shaper.c @@ -1452,6 +1452,8 @@ int net_shaper_nl_cap_get_dumpit(struct sk_buff *skb, ret = net_shaper_cap_fill_one(skb, binding, scope, flags, info); + /* cap dumps are tiny, we expect them to fit in a single skb */ + WARN_ON_ONCE(ret == -EMSGSIZE); if (ret) return ret; } -- 2.54.0