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

[Simon Horman <horms@kernel.org>]

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: