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

[Weiming Shi]

Simon Horman <horms@xxxxxxxxxx> 于2026年7月24日周五 22:23写道：
>
>
> 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@xxxxxxxxx?part=1
> ---
> --- Patch [1]: [PATCH net] netdevsim: tc: serialize access to nsim_block_cb_list ---
> commit 27d18d348023b46d852b44b24e6f4ca81ab39bf3
> Author: Weiming Shi <bestswngs@xxxxxxxxx>
>
> 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:

Hi Simon,

Thanks for the feedback. That makes sense, and I agree with the
concern. I'll work on a v2 patch to address this.

Thanks,
Weiming