Re: [PATCH net] net: flow_offload: protect driver_block_list in flow_block_cb_setup_simple()

[Florian Westphal <fw@strlen.de>]

Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 11 Feb 2026 13:06:48 +0100 Florian Westphal wrote:
> > Shigeru Yoshida <syoshida@redhat.com> wrote:
> > > syzbot reported a list_del corruption in flow_block_cb_setup_simple(). [0]
> > > 
> > > flow_block_cb_setup_simple() accesses the driver_block_list (e.g.,
> > > netdevsim's nsim_block_cb_list) without any synchronization. The
> > > nftables offload path calls into this function via ndo_setup_tc while
> > > holding the per-netns commit_mutex, but this mutex does not prevent
> > > concurrent access from tasks in different network namespaces that
> > > share the same driver_block_list, leading to list corruption:
> > > 
> > > - Task A (FLOW_BLOCK_BIND) calls list_add_tail() to insert a new
> > >   flow_block_cb into driver_block_list.
> > > 
> > > - Task B (FLOW_BLOCK_UNBIND) concurrently calls list_del() on another
> > >   flow_block_cb from the same list.  
> > 
> > Looking at the *upper layer*, I don't think it expected drivers to use
> > a single global list for this bit something that is scoped to the
> > net_device.
> 
> Maybe subjective but the fix seems a little off to me.
> Isn't flow_block_cb_setup_simple() just a "simple" implementation 
> for reuse in drivers locking in there doesn't really guarantee much?

Not sure what you mean.  I see the same pattern as netdevsim in all
drivers using this API.  Random example:

static LIST_HEAD(ice_repr_block_cb_list);

[..]
   return flow_block_cb_setup_simple((struct flow_block_offload *)
                                     type_data,
                                     &ice_repr_block_cb_list,
                                     ice_repr_setup_tc_block_cb,
                                     np, np, true);

This is safe only as long as all ice_repr_setup_tc() calls happen
in same net namespace.  I don't think we can rely on this.

> If we think netdevsim is doing something odd, let's make it work
> like real drivers.

I fear fixing netdevsim to not use single list will resolve the
syzbot report but AFAICS this pattern is in many drivers.

> TBH I thought block setup was always under rtnl_lock.

netdevices.rst says:
"``TC_SETUP_BLOCK`` and ``TC_SETUP_FT`` are running under NFT locks
        (i.e. no ``rtnl_lock`` and no device instance lock)."

I don't think it will be possible to change it.

nf_tables_netdev_event is called with rtnl_lock and it can then
take the pernet nf_tables transaction mutex.

Maybe it would be possible to rework flow_block_cb_setup_simple()
to not depend on an external list_head argument, but its not easy to
test such a patch nor do I think its going to be -net material let
alone something that -stable likes to digest.