[PATCH net] netdevsim: tc: serialize access to nsim_block_cb_list

[PATCH net] netdevsim: tc: serialize access to nsim_block_cb_list

[Weiming Shi]

nsim_setup_tc() passes a single global nsim_block_cb_list, shared by every
netdevsim device, to flow_block_cb_setup_simple(). That helper does
list_add_tail()/list_del() on the list on block bind/unbind and walks it
in flow_block_cb_is_busy(); it takes no lock of its own and relies on the
caller for serialization.

The tc control path calls ndo_setup_tc(TC_SETUP_BLOCK) under rtnl, but the
nf_tables hardware offload path reaches it under the per-netns nftables
commit_mutex only. Two nft transactions committing offload chains from
different netns thus run flow_block_cb_setup_simple() on the same list
concurrently, corrupting it and freeing a flow_block_cb that the other CPU
still walks:

 list_del corruption. prev->next should be ffff88801f18ff00, but was ffff88801de82b00.
 kernel BUG at lib/list_debug.c:62!
 RIP: 0010:__list_del_entry_valid_or_report (lib/list_debug.c:62)
  flow_block_cb_setup_simple (net/core/flow_offload.c:369)
  nsim_setup_tc (drivers/net/netdevsim/tc.c)
  nft_block_offload_cmd (net/netfilter/nf_tables_offload.c:394)
  nft_flow_rule_offload_commit (net/netfilter/nf_tables_offload.c:585)
  nf_tables_commit (net/netfilter/nf_tables_api.c:10490)
  nfnetlink_rcv_batch (net/netfilter/nfnetlink.c:577)
  nfnetlink_rcv (net/netfilter/nfnetlink.c:649)
  netlink_unicast (net/netlink/af_netlink.c:1314)
  netlink_sendmsg (net/netlink/af_netlink.c:1889)
  __sys_sendto (net/socket.c:729)

With KASAN the same race is reported as a slab-use-after-free read of the
freed flow_block_cb (kmalloc-192) in flow_block_cb_setup_simple(). The
trace above is from a 6.12.y reproduction.

Serialize the list with a mutex. ndo_setup_tc(TC_SETUP_BLOCK) always runs
in process context, so sleeping on the mutex is fine.

Fixes: 955bcb6ea0df ("drivers: net: use flow block API")
Reported-by: Xiang Mei <xmei5@asu.edu>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 drivers/net/netdevsim/tc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netdevsim/tc.c b/drivers/net/netdevsim/tc.c
index a415e02a6df1..30dd7f924b71 100644
--- a/drivers/net/netdevsim/tc.c
+++ b/drivers/net/netdevsim/tc.c
@@ -72,11 +72,13 @@ static int nsim_setup_tc_ets(struct net_device *dev,
 }
 
 static LIST_HEAD(nsim_block_cb_list);
+static DEFINE_MUTEX(nsim_block_cb_lock);
 
 int
 nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
 {
 	struct netdevsim *ns = netdev_priv(dev);
+	int err;
 
 	switch (type) {
 	case TC_SETUP_QDISC_TAPRIO:
@@ -84,10 +86,13 @@ nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
 	case TC_SETUP_QDISC_ETS:
 		return nsim_setup_tc_ets(dev, type_data);
 	case TC_SETUP_BLOCK:
-		return flow_block_cb_setup_simple(type_data,
-						  &nsim_block_cb_list,
-						  nsim_setup_tc_block_cb,
-						  ns, ns, true);
+		mutex_lock(&nsim_block_cb_lock);
+		err = flow_block_cb_setup_simple(type_data,
+						 &nsim_block_cb_list,
+						 nsim_setup_tc_block_cb,
+						 ns, ns, true);
+		mutex_unlock(&nsim_block_cb_lock);
+		return err;
 	case TC_SETUP_FT:
 		return 0;
 	default:
-- 
2.43.0

Re: [PATCH net] netdevsim: tc: serialize access to nsim_block_cb_list

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260719175028.988301-2-bestswngs@gmail.com?part=1
---
--- Patch [1]: [PATCH net] netdevsim: tc: serialize access to nsim_block_cb_list ---
commit 27d18d348023b46d852b44b24e6f4ca81ab39bf3
Author: Weiming Shi <bestswngs@gmail.com>

netdevsim: tc: serialize access to nsim_block_cb_list

This commit attempts to fix a list corruption and slab-use-after-free race
condition between the nftables hardware offload path and the TC control
path. It adds a local mutex to netdevsim to serialize access to the global
nsim_block_cb_list during flow_block_cb_setup_simple().

> diff --git a/drivers/net/netdevsim/tc.c b/drivers/net/netdevsim/tc.c
> index a415e02a6df1e..30dd7f924b716 100644
> --- a/drivers/net/netdevsim/tc.c
> +++ b/drivers/net/netdevsim/tc.c
> @@ -72,11 +72,13 @@ static int nsim_setup_tc_ets(struct net_device *dev,
>  }
>  
>  static LIST_HEAD(nsim_block_cb_list);
> +static DEFINE_MUTEX(nsim_block_cb_lock);
>  
>  int
>  nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
>  {
>  	struct netdevsim *ns = netdev_priv(dev);
> +	int err;
>  
>  	switch (type) {
>  	case TC_SETUP_QDISC_TAPRIO:
[ ... ]
> @@ -84,10 +86,13 @@ nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
>  	case TC_SETUP_QDISC_ETS:
>  		return nsim_setup_tc_ets(dev, type_data);
>  	case TC_SETUP_BLOCK:
> -		return flow_block_cb_setup_simple(type_data,
> -						  &nsim_block_cb_list,
> -						  nsim_setup_tc_block_cb,
> -						  ns, ns, true);
> +		mutex_lock(&nsim_block_cb_lock);

Simon says: I'm wondering if you could take a look at the issue flagged
            below as it seems to undermine the fix proposed by this
	    patch.

[Severity: High]
This isn't a bug introduced by this patch, but will the new nsim_block_cb_lock
actually fully serialize access to the list?

If tcf_block_bind() fails in the core TC stack, its error unroll path
directly executes list_del(&block_cb->driver_list) while holding only
rtnl_lock.

If this error unroll happens concurrently with an nftables transaction that
holds the commit_mutex and the newly introduced nsim_block_cb_lock, the core
TC stack will modify nsim_block_cb_list completely bypassing the driver's
local lock.

Could this still lead to list corruption and slab-use-after-free bugs when the
concurrent nftables transaction walks the mutating list?

> +		err = flow_block_cb_setup_simple(type_data,

Simon says: I think the problem flagged below can be considered
            in the context of possible follow-up.

[Severity: High]
This is a pre-existing issue, but does adding a local lock in netdevsim mask
the underlying systemic race?

flow_block_cb_setup_simple() modifies driver-provided lists directly without
internal locking. Since the nftables hardware offload path runs without
rtnl_lock, unprivileged users can configure nftables concurrently and trigger
list corruption.

While this patch addresses the issue for netdevsim locally, won't this same
vulnerability persist across other hardware drivers (e.g., cxgb4, prestera,
i40e) that use TC_SETUP_BLOCK with statically allocated global lists?

> +						 &nsim_block_cb_list,
> +						 nsim_setup_tc_block_cb,
> +						 ns, ns, true);
> +		mutex_unlock(&nsim_block_cb_lock);
> +		return err;
>  	case TC_SETUP_FT:
>  		return 0;
>  	default: