[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:

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

[Weiming Shi]

Simon Horman <horms@kernel.org> 于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@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:

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

[PATCH net v2] net: flow_offload: serialize driver callback lists

[Weiming Shi]

Drivers keep flow_block_cb objects on callback lists which are often
shared globally. TC setup normally runs under RTNL, but nftables invokes
ndo_setup_tc() while holding only its per-netns commit mutex. Concurrent
transactions in different netns can therefore corrupt a shared driver
callback list.

The same list can also be changed after ndo_setup_tc() returns. If filter
replay fails while binding a TC block, tcf_block_bind() removes and frees
callbacks from its error path without synchronizing against an nftables
transaction walking the driver list.

Add a flow-offload callback mutex and hold it across direct and indirect
setup, callback ownership transfer and rollback. Move indirect-unregister
list removal into the flow-offload core, and keep indirect-device
registration transactional on setup failure. When TC must reject an
indirect bind after callback installation, unwind only the callbacks
created by that bind so existing callbacks remain intact.

Initialize the auxiliary callback list nodes so common error paths can
safely detach them. ENETC also inspects its callback list outside the
setup callback, so protect those accesses with the same mutex.

This fixes the following netdevsim failure:

  list_del corruption. next->prev should be ffff88800b3e30c0,
  but was ffff88800b539e40.
  WARNING: CPU: 0 PID: 79 at lib/list_debug.c:65
  RIP: __list_del_entry_valid_or_report
  flow_block_cb_setup_simple
  nft_block_offload_cmd
  nft_flow_rule_offload_commit
  nf_tables_commit
  nfnetlink_rcv_batch

Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support")
Reported-by: Xiang Mei <xmei5@asu.edu>
Link: https://lore.kernel.org/netdev/20260719175028.988301-2-bestswngs@gmail.com/
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---

v2:
  - Move serialization to common flow-offload paths, including rollback and indirect cleanup.
  - Preserve existing callbacks when an indirect bind fails.
    
  v1: https://lore.kernel.org/netdev/20260719175028.988301-2-bestswngs@gmail.com/
  review: https://lore.kernel.org/netdev/20260724142321.GL418547@horms.kernel.org/

 .../net/ethernet/freescale/enetc/enetc_qos.c  | 27 ++++--
 include/net/flow_offload.h                    |  6 +-
 net/core/flow_offload.c                       | 77 +++++++++++++++--
 net/netfilter/nf_flow_table_offload.c         | 28 +++---
 net/netfilter/nf_tables_offload.c             | 18 +++-
 net/sched/act_api.c                           |  4 +-
 net/sched/cls_api.c                           | 86 ++++++++++++++++---
 7 files changed, 201 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 7b17bca24f26..c994f878cacd 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -1601,30 +1601,41 @@ int enetc_set_psfp(struct net_device *ndev, bool en)
 
 int enetc_psfp_init(struct enetc_ndev_priv *priv)
 {
+	int err = 0;
+
+	flow_block_cb_lock();
 	if (epsfp.psfp_sfi_bitmap)
-		return 0;
+		goto out;
 
 	epsfp.psfp_sfi_bitmap = bitmap_zalloc(priv->psfp_cap.max_psfp_filter,
 					      GFP_KERNEL);
-	if (!epsfp.psfp_sfi_bitmap)
-		return -ENOMEM;
+	if (!epsfp.psfp_sfi_bitmap) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	spin_lock_init(&epsfp.psfp_lock);
 
 	if (list_empty(&enetc_block_cb_list))
 		epsfp.dev_bitmap = 0;
 
-	return 0;
+out:
+	flow_block_cb_unlock();
+	return err;
 }
 
 int enetc_psfp_clean(struct enetc_ndev_priv *priv)
 {
-	if (!list_empty(&enetc_block_cb_list))
-		return -EBUSY;
+	int err = -EBUSY;
 
-	clean_psfp_all();
+	flow_block_cb_lock();
+	if (list_empty(&enetc_block_cb_list)) {
+		clean_psfp_all();
+		err = 0;
+	}
+	flow_block_cb_unlock();
 
-	return 0;
+	return err;
 }
 
 int enetc_setup_tc_psfp(struct net_device *ndev, void *type_data)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 70a02ee14308..96a46ba2769d 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -647,6 +647,8 @@ struct flow_block_cb *flow_block_cb_lookup(struct flow_block *block,
 void *flow_block_cb_priv(struct flow_block_cb *block_cb);
 void flow_block_cb_incref(struct flow_block_cb *block_cb);
 unsigned int flow_block_cb_decref(struct flow_block_cb *block_cb);
+void flow_block_cb_lock(void);
+void flow_block_cb_unlock(void);
 
 static inline void flow_block_cb_add(struct flow_block_cb *block_cb,
 				     struct flow_block_offload *offload)
@@ -741,7 +743,9 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
 int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
 				enum tc_setup_type type, void *data,
 				struct flow_block_offload *bo,
-				void (*cleanup)(struct flow_block_cb *block_cb));
+				void (*cleanup)(struct flow_block_cb *block_cb),
+				bool *indr_dev_added);
+void flow_indr_dev_setup_abort(void *data);
 bool flow_indr_dev_exists(void);
 
 #endif /* _NET_FLOW_OFFLOAD_H */
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 5071d7fe6ce2..0b4d2a2cbaf8 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -270,6 +270,8 @@ struct flow_block_cb *flow_block_cb_alloc(flow_setup_cb_t *cb,
 	block_cb->cb_ident = cb_ident;
 	block_cb->cb_priv = cb_priv;
 	block_cb->release = release;
+	INIT_LIST_HEAD(&block_cb->driver_list);
+	INIT_LIST_HEAD(&block_cb->indr.list);
 
 	return block_cb;
 }
@@ -317,11 +319,27 @@ unsigned int flow_block_cb_decref(struct flow_block_cb *block_cb)
 }
 EXPORT_SYMBOL(flow_block_cb_decref);
 
+static DEFINE_MUTEX(flow_block_cb_mutex);
+
+void flow_block_cb_lock(void)
+{
+	mutex_lock(&flow_block_cb_mutex);
+}
+EXPORT_SYMBOL(flow_block_cb_lock);
+
+void flow_block_cb_unlock(void)
+{
+	mutex_unlock(&flow_block_cb_mutex);
+}
+EXPORT_SYMBOL(flow_block_cb_unlock);
+
 bool flow_block_cb_is_busy(flow_setup_cb_t *cb, void *cb_ident,
 			   struct list_head *driver_block_list)
 {
 	struct flow_block_cb *block_cb;
 
+	lockdep_assert_held(&flow_block_cb_mutex);
+
 	list_for_each_entry(block_cb, driver_block_list, driver_list) {
 		if (block_cb->cb == cb &&
 		    block_cb->cb_ident == cb_ident)
@@ -340,6 +358,8 @@ int flow_block_cb_setup_simple(struct flow_block_offload *f,
 {
 	struct flow_block_cb *block_cb;
 
+	lockdep_assert_held(&flow_block_cb_mutex);
+
 	if (ingress_only &&
 	    f->binder_type != FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
@@ -431,12 +451,14 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
 {
 	struct flow_indr_dev *indr_dev;
 
+	flow_block_cb_lock();
 	mutex_lock(&flow_indr_block_lock);
 	list_for_each_entry(indr_dev, &flow_block_indr_dev_list, list) {
 		if (indr_dev->cb == cb &&
 		    indr_dev->cb_priv == cb_priv) {
 			refcount_inc(&indr_dev->refcnt);
 			mutex_unlock(&flow_indr_block_lock);
+			flow_block_cb_unlock();
 			return 0;
 		}
 	}
@@ -444,12 +466,14 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
 	indr_dev = flow_indr_dev_alloc(cb, cb_priv);
 	if (!indr_dev) {
 		mutex_unlock(&flow_indr_block_lock);
+		flow_block_cb_unlock();
 		return -ENOMEM;
 	}
 
 	list_add(&indr_dev->list, &flow_block_indr_dev_list);
 	existing_qdiscs_register(cb, cb_priv);
 	mutex_unlock(&flow_indr_block_lock);
+	flow_block_cb_unlock();
 
 	tcf_action_reoffload_cb(cb, cb_priv, true);
 
@@ -465,8 +489,10 @@ static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
 
 	list_for_each_entry_safe(this, next, &flow_block_indr_list, indr.list) {
 		if (this->release == release &&
-		    this->indr.cb_priv == cb_priv)
+		    this->indr.cb_priv == cb_priv) {
+			list_del_init(&this->driver_list);
 			list_move(&this->indr.list, cleanup_list);
+		}
 	}
 }
 
@@ -486,6 +512,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
 	struct flow_indr_dev *this, *next, *indr_dev = NULL;
 	LIST_HEAD(cleanup_list);
 
+	flow_block_cb_lock();
 	mutex_lock(&flow_indr_block_lock);
 	list_for_each_entry_safe(this, next, &flow_block_indr_dev_list, list) {
 		if (this->cb == cb &&
@@ -499,11 +526,13 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
 
 	if (!indr_dev) {
 		mutex_unlock(&flow_indr_block_lock);
+		flow_block_cb_unlock();
 		return;
 	}
 
 	__flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
 	mutex_unlock(&flow_indr_block_lock);
+	flow_block_cb_unlock();
 
 	tcf_action_reoffload_cb(cb, cb_priv, false);
 	flow_block_indr_notify(&cleanup_list);
@@ -603,18 +632,31 @@ static int indir_dev_remove(void *data)
 int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
 				enum tc_setup_type type, void *data,
 				struct flow_block_offload *bo,
-				void (*cleanup)(struct flow_block_cb *block_cb))
+				void (*cleanup)(struct flow_block_cb *block_cb),
+				bool *indr_dev_added)
 {
 	struct flow_indr_dev *this;
 	u32 count = 0;
 	int err;
 
+	lockdep_assert_held(&flow_block_cb_mutex);
+
+	if (indr_dev_added)
+		*indr_dev_added = false;
+
 	mutex_lock(&flow_indr_block_lock);
 	if (bo) {
-		if (bo->command == FLOW_BLOCK_BIND)
-			indir_dev_add(data, dev, sch, type, cleanup, bo);
-		else if (bo->command == FLOW_BLOCK_UNBIND)
+		if (bo->command == FLOW_BLOCK_BIND) {
+			err = indir_dev_add(data, dev, sch, type, cleanup, bo);
+			if (!err) {
+				if (indr_dev_added)
+					*indr_dev_added = true;
+			} else if (err != -EEXIST) {
+				goto out_unlock;
+			}
+		} else if (bo->command == FLOW_BLOCK_UNBIND) {
 			indir_dev_remove(data);
+		}
 	}
 
 	list_for_each_entry(this, &flow_block_indr_dev_list, list) {
@@ -623,14 +665,35 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
 			count++;
 	}
 
+	err = (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
+
+out_unlock:
 	mutex_unlock(&flow_indr_block_lock);
 
-	return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
+	return err;
 }
 EXPORT_SYMBOL(flow_indr_dev_setup_offload);
 
+void flow_indr_dev_setup_abort(void *data)
+{
+	lockdep_assert_held(&flow_block_cb_mutex);
+
+	mutex_lock(&flow_indr_block_lock);
+	WARN_ON_ONCE(indir_dev_remove(data));
+	mutex_unlock(&flow_indr_block_lock);
+}
+EXPORT_SYMBOL(flow_indr_dev_setup_abort);
+
 bool flow_indr_dev_exists(void)
 {
-	return !list_empty(&flow_block_indr_dev_list);
+	bool exists;
+
+	flow_block_cb_lock();
+	mutex_lock(&flow_indr_block_lock);
+	exists = !list_empty(&flow_block_indr_dev_list);
+	mutex_unlock(&flow_indr_block_lock);
+	flow_block_cb_unlock();
+
+	return exists;
 }
 EXPORT_SYMBOL(flow_indr_dev_exists);
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 801a3dd9ceea..2612978b27b4 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -1186,7 +1186,6 @@ static int nf_flow_table_block_setup(struct nf_flowtable *flowtable,
 	struct flow_block_cb *block_cb, *next;
 	int err = 0;
 
-	down_write(&flowtable->flow_block_lock);
 	switch (cmd) {
 	case FLOW_BLOCK_BIND:
 		list_splice(&bo->cb_list, &flowtable->flow_block.cb_list);
@@ -1201,8 +1200,6 @@ static int nf_flow_table_block_setup(struct nf_flowtable *flowtable,
 		WARN_ON_ONCE(1);
 		err = -EOPNOTSUPP;
 	}
-	up_write(&flowtable->flow_block_lock);
-
 	return err;
 }
 
@@ -1230,7 +1227,6 @@ static void nf_flow_table_indr_cleanup(struct flow_block_cb *block_cb)
 	nf_flow_table_gc_cleanup(flowtable, dev);
 	down_write(&flowtable->flow_block_lock);
 	list_del(&block_cb->list);
-	list_del(&block_cb->driver_list);
 	flow_block_cb_free(block_cb);
 	up_write(&flowtable->flow_block_lock);
 }
@@ -1239,13 +1235,15 @@ static int nf_flow_table_indr_offload_cmd(struct flow_block_offload *bo,
 					  struct nf_flowtable *flowtable,
 					  struct net_device *dev,
 					  enum flow_block_command cmd,
-					  struct netlink_ext_ack *extack)
+					  struct netlink_ext_ack *extack,
+					  bool *indr_dev_added)
 {
 	nf_flow_table_block_offload_init(bo, dev_net(dev), cmd, flowtable,
 					 extack);
 
 	return flow_indr_dev_setup_offload(dev, NULL, TC_SETUP_FT, flowtable, bo,
-					   nf_flow_table_indr_cleanup);
+					   nf_flow_table_indr_cleanup,
+					   indr_dev_added);
 }
 
 static int nf_flow_table_offload_cmd(struct flow_block_offload *bo,
@@ -1258,9 +1256,7 @@ static int nf_flow_table_offload_cmd(struct flow_block_offload *bo,
 
 	nf_flow_table_block_offload_init(bo, dev_net(dev), cmd, flowtable,
 					 extack);
-	down_write(&flowtable->flow_block_lock);
 	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_FT, bo);
-	up_write(&flowtable->flow_block_lock);
 	if (err < 0)
 		return err;
 
@@ -1273,21 +1269,29 @@ int nf_flow_table_offload_setup(struct nf_flowtable *flowtable,
 {
 	struct netlink_ext_ack extack = {};
 	struct flow_block_offload bo;
+	bool indr_dev_added = false;
 	int err;
 
 	if (!nf_flowtable_hw_offload(flowtable))
 		return nf_flow_offload_xdp_setup(flowtable, dev, cmd);
 
+	flow_block_cb_lock();
+	down_write(&flowtable->flow_block_lock);
 	if (dev->netdev_ops->ndo_setup_tc)
 		err = nf_flow_table_offload_cmd(&bo, flowtable, dev, cmd,
 						&extack);
 	else
 		err = nf_flow_table_indr_offload_cmd(&bo, flowtable, dev, cmd,
-						     &extack);
-	if (err < 0)
-		return err;
+						     &extack,
+						     &indr_dev_added);
+	if (err >= 0)
+		err = nf_flow_table_block_setup(flowtable, &bo, cmd);
+	if (err < 0 && indr_dev_added)
+		flow_indr_dev_setup_abort(flowtable);
+	up_write(&flowtable->flow_block_lock);
+	flow_block_cb_unlock();
 
-	return nf_flow_table_block_setup(flowtable, &bo, cmd);
+	return err;
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_offload_setup);
 
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 8998a24651ff..7189e8c4e5a0 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -414,7 +414,6 @@ static void nft_indr_block_cleanup(struct flow_block_cb *block_cb)
 				    basechain, &extack);
 	nft_net = nft_pernet(net);
 	mutex_lock(&nft_net->commit_mutex);
-	list_del(&block_cb->driver_list);
 	list_move(&block_cb->list, &bo.cb_list);
 	nft_flow_offload_unbind(&bo, basechain);
 	mutex_unlock(&nft_net->commit_mutex);
@@ -426,19 +425,28 @@ static int nft_indr_block_offload_cmd(struct nft_base_chain *basechain,
 {
 	struct netlink_ext_ack extack = {};
 	struct flow_block_offload bo;
+	bool indr_dev_added = false;
 	int err;
 
 	nft_flow_block_offload_init(&bo, dev_net(dev), cmd, basechain, &extack);
 
 	err = flow_indr_dev_setup_offload(dev, NULL, TC_SETUP_BLOCK, basechain, &bo,
-					  nft_indr_block_cleanup);
-	if (err < 0)
+					  nft_indr_block_cleanup,
+					  &indr_dev_added);
+	if (err < 0) {
+		if (indr_dev_added)
+			flow_indr_dev_setup_abort(basechain);
 		return err;
+	}
 
 	if (list_empty(&bo.cb_list))
 		return -EOPNOTSUPP;
 
-	return nft_block_setup(basechain, &bo, cmd);
+	err = nft_block_setup(basechain, &bo, cmd);
+	if (err < 0 && indr_dev_added)
+		flow_indr_dev_setup_abort(basechain);
+
+	return err;
 }
 
 static int nft_chain_offload_cmd(struct nft_base_chain *basechain,
@@ -447,10 +455,12 @@ static int nft_chain_offload_cmd(struct nft_base_chain *basechain,
 {
 	int err;
 
+	flow_block_cb_lock();
 	if (dev->netdev_ops->ndo_setup_tc)
 		err = nft_block_offload_cmd(basechain, dev, cmd);
 	else
 		err = nft_indr_block_offload_cmd(basechain, dev, cmd);
+	flow_block_cb_unlock();
 
 	return err;
 }
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f141634df214..317b026f0463 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -205,8 +205,10 @@ static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
 {
 	int err;
 
+	flow_block_cb_lock();
 	err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
-					  fl_act, NULL, NULL);
+					  fl_act, NULL, NULL, NULL);
+	flow_block_cb_unlock();
 	if (err < 0)
 		return err;
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index fee4524adc98..1c230e33e1a5 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -808,7 +808,6 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
 			       &extack);
 	rtnl_lock();
 	down_write(&block->cb_lock);
-	list_del(&block_cb->driver_list);
 	list_move(&block_cb->list, &bo.cb_list);
 	tcf_block_unbind(block, &bo);
 	up_write(&block->cb_lock);
@@ -824,17 +823,20 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
 				 struct net_device *dev, struct Qdisc *sch,
 				 struct tcf_block_ext_info *ei,
 				 enum flow_block_command command,
-				 struct netlink_ext_ack *extack)
+				 struct netlink_ext_ack *extack,
+				 bool *indr_dev_added,
+				 unsigned int *indr_cb_count,
+				 bool *indr_unlocked_driver_cb)
 {
 	struct flow_block_offload bo = {};
+	unsigned int cb_count;
+	int err;
 
 	tcf_block_offload_init(&bo, dev, sch, command, ei->binder_type,
 			       &block->flow_block, tcf_block_shared(block),
 			       extack);
 
 	if (dev->netdev_ops->ndo_setup_tc) {
-		int err;
-
 		err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
 		if (err < 0) {
 			if (err != -EOPNOTSUPP)
@@ -845,21 +847,66 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
 		return tcf_block_setup(block, &bo);
 	}
 
-	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
-				    tc_block_indr_cleanup);
-	tcf_block_setup(block, &bo);
+	err = flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
+					  tc_block_indr_cleanup,
+					  indr_dev_added);
+	if (err < 0)
+		return err;
+	cb_count = list_count_nodes(&bo.cb_list);
+	err = tcf_block_setup(block, &bo);
+	if (err)
+		return err;
+	if (indr_cb_count)
+		*indr_cb_count = cb_count;
+	if (indr_unlocked_driver_cb)
+		*indr_unlocked_driver_cb = bo.unlocked_driver_cb;
 
 	return -EOPNOTSUPP;
 }
 
+static void
+tcf_block_indr_bind_rollback(struct tcf_block *block, struct net_device *dev,
+			     struct Qdisc *sch, struct tcf_block_ext_info *ei,
+			     unsigned int cb_count, bool unlocked_driver_cb,
+			     bool remove_indr_dev)
+{
+	struct flow_block_offload bo = {};
+	struct flow_block_cb *block_cb;
+
+	tcf_block_offload_init(&bo, dev, sch, FLOW_BLOCK_UNBIND,
+			       ei->binder_type, &block->flow_block,
+			       tcf_block_shared(block), NULL);
+	bo.unlocked_driver_cb = unlocked_driver_cb;
+
+	if (remove_indr_dev)
+		flow_indr_dev_setup_abort(block);
+
+	/* tcf_block_bind() splices this bind's callbacks at the list head. */
+	while (cb_count--) {
+		if (WARN_ON_ONCE(list_empty(&block->flow_block.cb_list)))
+			break;
+
+		block_cb = list_first_entry(&block->flow_block.cb_list,
+					    struct flow_block_cb, list);
+		list_del_init(&block_cb->driver_list);
+		list_del_init(&block_cb->indr.list);
+		list_move(&block_cb->list, &bo.cb_list);
+	}
+	tcf_block_unbind(block, &bo);
+}
+
 static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
 				  struct tcf_block_ext_info *ei,
 				  struct netlink_ext_ack *extack)
 {
 	struct net_device *dev = q->dev_queue->dev;
+	bool indr_unlocked_driver_cb = false;
+	bool indr_dev_added = false;
+	unsigned int indr_cb_count = 0;
 	int err;
 
 	down_write(&block->cb_lock);
+	flow_block_cb_lock();
 
 	/* If tc offload feature is disabled and the block we try to bind
 	 * to already has some offloaded filters, forbid to bind.
@@ -872,14 +919,15 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
 		goto err_unlock;
 	}
 
-	err = tcf_block_offload_cmd(block, dev, q, ei, FLOW_BLOCK_BIND, extack);
+	err = tcf_block_offload_cmd(block, dev, q, ei, FLOW_BLOCK_BIND, extack,
+				    &indr_dev_added, &indr_cb_count,
+				    &indr_unlocked_driver_cb);
 	if (err == -EOPNOTSUPP)
 		goto no_offload_dev_inc;
 	if (err)
 		goto err_unlock;
 
-	up_write(&block->cb_lock);
-	return 0;
+	goto out_unlock;
 
 no_offload_dev_inc:
 	if (tcf_block_offload_in_use(block))
@@ -888,6 +936,15 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
 	err = 0;
 	block->nooffloaddevcnt++;
 err_unlock:
+	if (err && indr_cb_count)
+		tcf_block_indr_bind_rollback(block, dev, q, ei,
+					     indr_cb_count,
+					     indr_unlocked_driver_cb,
+					     indr_dev_added);
+	else if (err && indr_dev_added)
+		flow_indr_dev_setup_abort(block);
+out_unlock:
+	flow_block_cb_unlock();
 	up_write(&block->cb_lock);
 	return err;
 }
@@ -899,14 +956,18 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
 	int err;
 
 	down_write(&block->cb_lock);
-	err = tcf_block_offload_cmd(block, dev, q, ei, FLOW_BLOCK_UNBIND, NULL);
+	flow_block_cb_lock();
+	err = tcf_block_offload_cmd(block, dev, q, ei, FLOW_BLOCK_UNBIND, NULL,
+				    NULL, NULL, NULL);
 	if (err == -EOPNOTSUPP)
 		goto no_offload_dev_dec;
+	flow_block_cb_unlock();
 	up_write(&block->cb_lock);
 	return;
 
 no_offload_dev_dec:
 	WARN_ON(block->nooffloaddevcnt-- == 0);
+	flow_block_cb_unlock();
 	up_write(&block->cb_lock);
 }
 
@@ -1649,7 +1710,8 @@ static int tcf_block_bind(struct tcf_block *block,
 
 err_unroll:
 	list_for_each_entry_safe(block_cb, next, &bo->cb_list, list) {
-		list_del(&block_cb->driver_list);
+		list_del_init(&block_cb->driver_list);
+		list_del_init(&block_cb->indr.list);
 		if (i-- > 0) {
 			list_del(&block_cb->list);
 			tcf_block_playback_offloads(block, block_cb->cb,
-- 
2.55.0

Re: [PATCH net v2] net: flow_offload: serialize driver callback lists

[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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260726120338.488658-1-bestswngs@gmail.com?part=1
---
--- Patch [1]: [PATCH net v2] net: flow_offload: serialize driver callback lists ---
commit 93c807b64d24dcddc15896bc3604b3d8341855e1
Author: Weiming Shi <bestswngs@gmail.com>

net: flow_offload: serialize driver callback lists

This adds a global flow-offload callback mutex that is held across direct
and indirect ndo_setup_tc() setup, callback ownership transfer and
rollback, moves indirect-unregister list removal into the flow-offload
core, and makes indirect-device registration transactional on setup
failure.

Link: https://lore.kernel.org/netdev/20260719175028.988301-2-bestswngs@gmail.com/

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index 7b17bca24f26..c994f878cacd 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -1601,30 +1601,41 @@ int enetc_set_psfp(struct net_device *ndev, bool en)
>  
>  int enetc_psfp_init(struct enetc_ndev_priv *priv)
>  {
> +	int err = 0;
> +
> +	flow_block_cb_lock();
>  	if (epsfp.psfp_sfi_bitmap)
> -		return 0;
> +		goto out;
>  
>  	epsfp.psfp_sfi_bitmap = bitmap_zalloc(priv->psfp_cap.max_psfp_filter,
>  					      GFP_KERNEL);
> -	if (!epsfp.psfp_sfi_bitmap)
> -		return -ENOMEM;
> +	if (!epsfp.psfp_sfi_bitmap) {
> +		err = -ENOMEM;
> +		goto out;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but since this hunk rewrites
exactly the bitmap allocation site, is the psfp_sfi_bitmap lifecycle
asymmetric here?

The UNBIND path in enetc_setup_tc_psfp() frees the bitmap:

	case FLOW_BLOCK_UNBIND:
		...
		clear_bit(port, &epsfp.dev_bitmap);
		if (!epsfp.dev_bitmap)
			clean_psfp_all();

and clean_psfp_all() -> clean_psfp_sfi_bitmap() does bitmap_free() and
sets epsfp.psfp_sfi_bitmap = NULL.

The BIND path only does set_bit(port, &epsfp.dev_bitmap), it never
reallocates. enetc_psfp_init() is reached only from the inline
enetc_psfp_enable() in enetc.h, called from enetc_pf_set_features() on a
NETIF_F_HW_TC change and once at probe, and it early-returns when the
pointer is non-NULL.

So after "tc qdisc del dev eth0 clsact" followed by "tc qdisc add dev eth0
clsact" (features unchanged, so tc_can_offload() still passes), does the
first flower rule with a PSFP action hit a NULL dereference in
enetc_get_free_index()?

	index = find_first_zero_bit(epsfp.psfp_sfi_bitmap, max_size);

There is no NULL guard on any of the psfp_sfi_bitmap users.

>  
>  	spin_lock_init(&epsfp.psfp_lock);
>  
>  	if (list_empty(&enetc_block_cb_list))
>  		epsfp.dev_bitmap = 0;
>  
> -	return 0;
> +out:
> +	flow_block_cb_unlock();
> +	return err;
>  }
>  
>  int enetc_psfp_clean(struct enetc_ndev_priv *priv)
>  {
> -	if (!list_empty(&enetc_block_cb_list))
> -		return -EBUSY;
> +	int err = -EBUSY;
>  
> -	clean_psfp_all();
> +	flow_block_cb_lock();
> +	if (list_empty(&enetc_block_cb_list)) {
> +		clean_psfp_all();
> +		err = 0;
> +	}
> +	flow_block_cb_unlock();
>  
> -	return 0;
> +	return err;
>  }

[Severity: Low]
Is exporting the core mutex for drivers to take directly the intended
layering here?  The driver now runs bitmap_zalloc() and clean_psfp_all()
while holding a net/core mutex, which inverts the usual "core calls
driver" direction.

The new contract - a driver must hold flow_block_cb_mutex whenever it
touches its own driver_block_list - is expressed only as
lockdep_assert_held() inside flow_block_cb_is_busy() and
flow_block_cb_setup_simple(), and is not documented anywhere in
include/net/flow_offload.h.  Only ENETC was converted, so other drivers
that inspect their callback list outside ndo_setup_tc() remain
unprotected.

Would it be preferable to keep the mutex private to
net/core/flow_offload.c and give drivers a purpose-built helper instead?

> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 70a02ee14308..96a46ba2769d 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -647,6 +647,8 @@ struct flow_block_cb *flow_block_cb_lookup(struct flow_block *block,
>  void *flow_block_cb_priv(struct flow_block_cb *block_cb);
>  void flow_block_cb_incref(struct flow_block_cb *block_cb);
>  unsigned int flow_block_cb_decref(struct flow_block_cb *block_cb);
> +void flow_block_cb_lock(void);
> +void flow_block_cb_unlock(void);
>  
>  static inline void flow_block_cb_add(struct flow_block_cb *block_cb,
>  				     struct flow_block_offload *offload)
> @@ -741,7 +743,9 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>  int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
>  				enum tc_setup_type type, void *data,
>  				struct flow_block_offload *bo,
> -				void (*cleanup)(struct flow_block_cb *block_cb));
> +				void (*cleanup)(struct flow_block_cb *block_cb),
> +				bool *indr_dev_added);
> +void flow_indr_dev_setup_abort(void *data);

[Severity: Low]
Could this be split into a series?  As it stands one patch with Fixes: +
Cc: stable carries at least five independent changes:

  - the new global mutex plus exported lock/unlock API
  - INIT_LIST_HEAD() of the block_cb auxiliary list nodes
  - moving the indirect-cleanup list_del(&block_cb->driver_list) into the
    core
  - transactional indirect-device registration, changing the prototype of
    the exported flow_indr_dev_setup_offload() and adding the exported
    flow_indr_dev_setup_abort()
  - a brand new TC rollback path for failed indirect binds

plus the unrelated ENETC change.

The single Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware
offload support") does not cover the TC-side changes (the moved
list_del() in tc_block_indr_cleanup() and the new
tcf_block_indr_bind_rollback()), which are not introduced by the nftables
offload commit.

>  bool flow_indr_dev_exists(void);
>  
>  #endif /* _NET_FLOW_OFFLOAD_H */
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 5071d7fe6ce2..0b4d2a2cbaf8 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c

[ ... ]

> @@ -317,11 +319,27 @@ unsigned int flow_block_cb_decref(struct flow_block_cb *block_cb)
>  }
>  EXPORT_SYMBOL(flow_block_cb_decref);
>  
> +static DEFINE_MUTEX(flow_block_cb_mutex);
> +
> +void flow_block_cb_lock(void)
> +{
> +	mutex_lock(&flow_block_cb_mutex);
> +}
> +EXPORT_SYMBOL(flow_block_cb_lock);

[Severity: Medium]
Should the scalability impact of a single system-wide mutex be discussed
in the commit message?

This mutex becomes the outermost lock on every flow-offload configuration
path (TC block bind/unbind, nftables chain offload, flowtable offload, TC
action offload, indirect device register/unregister, and even
flow_indr_dev_exists()), and it is held across the driver's
ndo_setup_tc().

The two sides acquire it with different outer locks:

nft_chain_offload_cmd()			(no RTNL)
    flow_block_cb_lock()
        dev->netdev_ops->ndo_setup_tc()

tcf_block_offload_bind()		(under RTNL)
    down_write(&block->cb_lock)
    flow_block_cb_lock()

So an nftables commit in one netns that stalls inside a driver or
firmware ndo_setup_tc() while holding flow_block_cb_mutex will stall any
RTNL holder waiting for that mutex, i.e. netdev configuration on
completely unrelated devices.

Were cheaper alternatives considered, such as a lock supplied per driver
callback list, taking RTNL in the nft offload path, or scoping the lock
per netdev?

> +
> +void flow_block_cb_unlock(void)
> +{
> +	mutex_unlock(&flow_block_cb_mutex);
> +}
> +EXPORT_SYMBOL(flow_block_cb_unlock);

[ ... ]

> @@ -486,6 +512,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>  	struct flow_indr_dev *this, *next, *indr_dev = NULL;
>  	LIST_HEAD(cleanup_list);
>  
> +	flow_block_cb_lock();
>  	mutex_lock(&flow_indr_block_lock);
>  	list_for_each_entry_safe(this, next, &flow_block_indr_dev_list, list) {
>  		if (this->cb == cb &&
> @@ -499,11 +526,13 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>  
>  	if (!indr_dev) {
>  		mutex_unlock(&flow_indr_block_lock);
> +		flow_block_cb_unlock();
>  		return;
>  	}
>  
>  	__flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
>  	mutex_unlock(&flow_indr_block_lock);
> +	flow_block_cb_unlock();
>  
>  	tcf_action_reoffload_cb(cb, cb_priv, false);
>  	flow_block_indr_notify(&cleanup_list);

[Severity: High]
This isn't a bug introduced by this patch, but does the indirect cleanup
path still leave the very corruption window the patch aims to close?

flow_block_cb_unlock() runs before flow_block_indr_notify(), and
flow_block_indr_notify() calls this->indr.cleanup(this), i.e.
tc_block_indr_cleanup() in net/sched/cls_api.c:

	rtnl_lock();
	down_write(&block->cb_lock);
	list_move(&block_cb->list, &bo.cb_list);
	tcf_block_unbind(block, &bo);

so that deletion from block->flow_block.cb_list happens with only RTNL +
cb_lock held.

Meanwhile the register side splices into the same list head with a
disjoint lock set:

net/core/flow_offload.c:existing_qdiscs_register() {
	...
	cb(cur->dev, cur->sch, cb_priv, cur->type, &bo, cur->data, cur->cleanup);
	list_splice(&bo.cb_list, cur->cb_list);
	...
}

That runs from flow_indr_dev_register() holding flow_block_cb_mutex +
flow_indr_block_lock, but neither RTNL nor block->cb_lock (cb_list here
is bo->cb_list_head, i.e. &block->flow_block.cb_list, recorded by
indir_dev_add()).

Can two indirect-offload drivers (or repeated load/unload of one) then
run splice and list_del concurrently on one live tcf_block cb_list, and
produce the same "list_del corruption ... next->prev should be" splat?
Note tc_block_indr_cleanup() itself takes RTNL, so it cannot simply be
pulled under the new mutex.

> @@ -603,18 +632,31 @@ static int indir_dev_remove(void *data)
>  int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
>  				enum tc_setup_type type, void *data,
>  				struct flow_block_offload *bo,
> -				void (*cleanup)(struct flow_block_cb *block_cb))
> +				void (*cleanup)(struct flow_block_cb *block_cb),
> +				bool *indr_dev_added)
>  {
>  	struct flow_indr_dev *this;
>  	u32 count = 0;
>  	int err;
>  
> +	lockdep_assert_held(&flow_block_cb_mutex);
> +
> +	if (indr_dev_added)
> +		*indr_dev_added = false;
> +
>  	mutex_lock(&flow_indr_block_lock);
>  	if (bo) {
> -		if (bo->command == FLOW_BLOCK_BIND)
> -			indir_dev_add(data, dev, sch, type, cleanup, bo);
> -		else if (bo->command == FLOW_BLOCK_UNBIND)
> +		if (bo->command == FLOW_BLOCK_BIND) {
> +			err = indir_dev_add(data, dev, sch, type, cleanup, bo);
> +			if (!err) {
> +				if (indr_dev_added)
> +					*indr_dev_added = true;
> +			} else if (err != -EEXIST) {
> +				goto out_unlock;
> +			}
> +		} else if (bo->command == FLOW_BLOCK_UNBIND) {
>  			indir_dev_remove(data);
> +		}
>  	}

[Severity: Medium]
Does this turn a previously silent degradation into a user-visible
failure?

Before this change the return value of indir_dev_add() was discarded, so
a failed bookkeeping allocation did not affect the outcome.  Now an
-ENOMEM from indir_dev_add() aborts the whole setup before any indirect
driver callback runs, and cls_api propagates it:

net/sched/cls_api.c:tcf_block_offload_cmd() {
	...
	if (err < 0)
		return err;
	cb_count = list_count_nodes(&bo.cb_list);
	err = tcf_block_setup(block, &bo);
	if (err)
		return err;
	...
}

net/sched/cls_api.c:tcf_block_offload_bind() {
	...
	if (err == -EOPNOTSUPP)
		goto no_offload_dev_inc;
	if (err)
		goto err_unlock;
	...
}

Only -EOPNOTSUPP still reaches the software fallback, so "tc qdisc add"
on an indirect device, nft base-chain offload and flowtable offload can
now fail to user space where they previously succeeded with
block->nooffloaddevcnt++.  The same applies to a non-EOPNOTSUPP error
from tp->ops->reoffload during filter replay, which tcf_block_setup() now
forwards.

The commit message only says "keep indirect-device registration
transactional on setup failure".  Could it state this new failure mode
explicitly, given the Cc: stable tag?

>  
>  	list_for_each_entry(this, &flow_block_indr_dev_list, list) {
> @@ -623,14 +665,35 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
>  			count++;
>  	}
>  
> +	err = (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
> +
> +out_unlock:
>  	mutex_unlock(&flow_indr_block_lock);
>  
> -	return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
> +	return err;
>  }
>  EXPORT_SYMBOL(flow_indr_dev_setup_offload);
>  
> +void flow_indr_dev_setup_abort(void *data)
> +{
> +	lockdep_assert_held(&flow_block_cb_mutex);
> +
> +	mutex_lock(&flow_indr_block_lock);
> +	WARN_ON_ONCE(indir_dev_remove(data));
> +	mutex_unlock(&flow_indr_block_lock);
> +}
> +EXPORT_SYMBOL(flow_indr_dev_setup_abort);

[Severity: Low]
Would it be better for the core to undo its own bookkeeping rather than
export partial state plus an abort call?

indir_dev_add()/indir_dev_remove() are internal to
net/core/flow_offload.c, yet callers now receive bool *indr_dev_added and
must remember to call flow_indr_dev_setup_abort() on every later failure.
The same snippet is duplicated in nf_tables_offload.c (twice),
nf_flow_table_offload.c and cls_api.c (twice), with three different
failure semantics for one core operation: nft and flowtable only unwind
the indr_dev bookkeeping, cls_api additionally unwinds installed
callbacks.

Some of the new arms are also dead, since nft_flow_offload_bind() only
does list_splice() and returns 0, and nf_flow_table_block_setup() cannot
fail for BIND.

If a future caller forgets the abort, does the stale struct
flow_indir_dev_info left on flow_indir_dev_list later drive a bogus
replay through existing_qdiscs_register()?

> +
>  bool flow_indr_dev_exists(void)
>  {
> -	return !list_empty(&flow_block_indr_dev_list);
> +	bool exists;
> +
> +	flow_block_cb_lock();
> +	mutex_lock(&flow_indr_block_lock);
> +	exists = !list_empty(&flow_block_indr_dev_list);
> +	mutex_unlock(&flow_indr_block_lock);
> +	flow_block_cb_unlock();
> +
> +	return exists;
>  }
>  EXPORT_SYMBOL(flow_indr_dev_exists);

[Severity: Medium]
Is the global mutex needed for this predicate?  flow_indr_dev_exists() is
a cheap existence test used by nft_chain_offload_support() in
net/netfilter/nf_tables_offload.c, and taking flow_block_cb_mutex here
imposes the new outermost-lock ordering constraint on all of its callers.

> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 801a3dd9ceea..2612978b27b4 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c

[ ... ]

> @@ -1273,21 +1269,29 @@ int nf_flow_table_offload_setup(struct nf_flowtable *flowtable,
>  {
>  	struct netlink_ext_ack extack = {};
>  	struct flow_block_offload bo;
> +	bool indr_dev_added = false;
>  	int err;
>  
>  	if (!nf_flowtable_hw_offload(flowtable))
>  		return nf_flow_offload_xdp_setup(flowtable, dev, cmd);
>  
> +	flow_block_cb_lock();
> +	down_write(&flowtable->flow_block_lock);
>  	if (dev->netdev_ops->ndo_setup_tc)
>  		err = nf_flow_table_offload_cmd(&bo, flowtable, dev, cmd,
>  						&extack);
>  	else
>  		err = nf_flow_table_indr_offload_cmd(&bo, flowtable, dev, cmd,
> -						     &extack);
> -	if (err < 0)
> -		return err;
> +						     &extack,
> +						     &indr_dev_added);
> +	if (err >= 0)
> +		err = nf_flow_table_block_setup(flowtable, &bo, cmd);
> +	if (err < 0 && indr_dev_added)
> +		flow_indr_dev_setup_abort(flowtable);
> +	up_write(&flowtable->flow_block_lock);
> +	flow_block_cb_unlock();
>  
> -	return nf_flow_table_block_setup(flowtable, &bo, cmd);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(nf_flow_table_offload_setup);
>  
> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> index 8998a24651ff..7189e8c4e5a0 100644
> --- a/net/netfilter/nf_tables_offload.c
> +++ b/net/netfilter/nf_tables_offload.c

[ ... ]

> @@ -426,19 +425,28 @@ static int nft_indr_block_offload_cmd(struct nft_base_chain *basechain,
>  {
>  	struct netlink_ext_ack extack = {};
>  	struct flow_block_offload bo;
> +	bool indr_dev_added = false;
>  	int err;
>  
>  	nft_flow_block_offload_init(&bo, dev_net(dev), cmd, basechain, &extack);
>  
>  	err = flow_indr_dev_setup_offload(dev, NULL, TC_SETUP_BLOCK, basechain, &bo,
> -					  nft_indr_block_cleanup);
> -	if (err < 0)
> +					  nft_indr_block_cleanup,
> +					  &indr_dev_added);
> +	if (err < 0) {
> +		if (indr_dev_added)
> +			flow_indr_dev_setup_abort(basechain);
>  		return err;
> +	}
>  
>  	if (list_empty(&bo.cb_list))
>  		return -EOPNOTSUPP;

[Severity: Low]
Should this exit also call flow_indr_dev_setup_abort()?  It is the only
error return of the restructured function that skips the new cleanup.

The branch is dead today, since flow_indr_dev_setup_offload() already
folds the empty-cb_list case into -EOPNOTSUPP:

net/core/flow_offload.c:flow_indr_dev_setup_offload() {
	...
	err = (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
	...
}

with bo non-NULL here, and its only other negative return is -ENOMEM via
goto out_unlock.  Should it become reachable, would this exit leave the
flow_indir_dev_info created by indir_dev_add() on flow_indir_dev_list with
data pointing at a base chain the failing nft transaction is about to
destroy?

>  
> -	return nft_block_setup(basechain, &bo, cmd);
> +	err = nft_block_setup(basechain, &bo, cmd);
> +	if (err < 0 && indr_dev_added)
> +		flow_indr_dev_setup_abort(basechain);
> +
> +	return err;
>  }

[ ... ]

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index fee4524adc98..1c230e33e1a5 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c

[ ... ]

> @@ -845,21 +847,66 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
>  		return tcf_block_setup(block, &bo);
>  	}
>  
> -	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
> -				    tc_block_indr_cleanup);
> -	tcf_block_setup(block, &bo);
> +	err = flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
> +					  tc_block_indr_cleanup,
> +					  indr_dev_added);
> +	if (err < 0)
> +		return err;
> +	cb_count = list_count_nodes(&bo.cb_list);
> +	err = tcf_block_setup(block, &bo);
> +	if (err)
> +		return err;
> +	if (indr_cb_count)
> +		*indr_cb_count = cb_count;
> +	if (indr_unlocked_driver_cb)
> +		*indr_unlocked_driver_cb = bo.unlocked_driver_cb;
>  
>  	return -EOPNOTSUPP;
>  }
>  
> +static void
> +tcf_block_indr_bind_rollback(struct tcf_block *block, struct net_device *dev,
> +			     struct Qdisc *sch, struct tcf_block_ext_info *ei,
> +			     unsigned int cb_count, bool unlocked_driver_cb,
> +			     bool remove_indr_dev)
> +{
> +	struct flow_block_offload bo = {};
> +	struct flow_block_cb *block_cb;
> +
> +	tcf_block_offload_init(&bo, dev, sch, FLOW_BLOCK_UNBIND,
> +			       ei->binder_type, &block->flow_block,
> +			       tcf_block_shared(block), NULL);
> +	bo.unlocked_driver_cb = unlocked_driver_cb;
> +
> +	if (remove_indr_dev)
> +		flow_indr_dev_setup_abort(block);
> +
> +	/* tcf_block_bind() splices this bind's callbacks at the list head. */
> +	while (cb_count--) {
> +		if (WARN_ON_ONCE(list_empty(&block->flow_block.cb_list)))
> +			break;
> +
> +		block_cb = list_first_entry(&block->flow_block.cb_list,
> +					    struct flow_block_cb, list);
> +		list_del_init(&block_cb->driver_list);
> +		list_del_init(&block_cb->indr.list);
> +		list_move(&block_cb->list, &bo.cb_list);
> +	}
> +	tcf_block_unbind(block, &bo);
> +}

[Severity: Low]
Is identifying "this bind's callbacks" by popping cb_count entries off the
head of block->flow_block.cb_list robust enough?

The count is taken from bo.cb_list before tcf_block_setup(), and the
recovery relies on tcf_block_bind() using list_splice() (head insertion)
and on nothing else inserting in between.  The
WARN_ON_ONCE(list_empty(...)) inside the pop loop suggests the invariant
isn't structurally guaranteed.

If the splice direction in tcf_block_bind() ever changes, or another
writer inserts into that list, would this unbind and free callbacks owned
by a different binder?

Separately, tcf_block_offload_cmd() still returns -EOPNOTSUPP as the
success sentinel for the indirect path while carrying three new
out-parameters, so a caller passing NULL loses the rollback ability
silently.  Could the bind's callbacks be tracked explicitly instead, for
example on a local list?

[Severity: Low]
Which lock protects block_cb->indr.list here?  This helper does
list_del_init(&block_cb->indr.list) on the file-global
flow_block_indr_list while holding only block->cb_lock and the new
flow_block_cb_mutex.

Every mutator of that list in net/core/flow_offload.c takes
flow_indr_block_lock, e.g.

net/core/flow_offload.c:__flow_block_indr_cleanup() {
	...
		list_del_init(&this->driver_list);
		list_move(&this->indr.list, cleanup_list);
	...
}

and flow_indr_dev_setup_abort() itself takes flow_indr_block_lock for the
equivalent bookkeeping.  Today the lock sets do intersect on
flow_block_cb_mutex, so there is no race, but the rule becomes implicit.
Could a comment plus lockdep asserts (including on the exported
flow_indr_block_cb_alloc(), which has none) be added?

> +
>  static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>  				  struct tcf_block_ext_info *ei,
>  				  struct netlink_ext_ack *extack)
>  {
>  	struct net_device *dev = q->dev_queue->dev;
> +	bool indr_unlocked_driver_cb = false;
> +	bool indr_dev_added = false;
> +	unsigned int indr_cb_count = 0;
>  	int err;
>  
>  	down_write(&block->cb_lock);
> +	flow_block_cb_lock();
>  
>  	/* If tc offload feature is disabled and the block we try to bind
>  	 * to already has some offloaded filters, forbid to bind.

[ ... ]

> @@ -888,6 +936,15 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>  	err = 0;
>  	block->nooffloaddevcnt++;
>  err_unlock:
> +	if (err && indr_cb_count)
> +		tcf_block_indr_bind_rollback(block, dev, q, ei,
> +					     indr_cb_count,
> +					     indr_unlocked_driver_cb,
> +					     indr_dev_added);
> +	else if (err && indr_dev_added)
> +		flow_indr_dev_setup_abort(block);
> +out_unlock:
> +	flow_block_cb_unlock();
>  	up_write(&block->cb_lock);
>  	return err;
>  }

[ ... ]

> @@ -1649,7 +1710,8 @@ static int tcf_block_bind(struct tcf_block *block,
>  
>  err_unroll:
>  	list_for_each_entry_safe(block_cb, next, &bo->cb_list, list) {
> -		list_del(&block_cb->driver_list);
> +		list_del_init(&block_cb->driver_list);
> +		list_del_init(&block_cb->indr.list);

[Severity: Low]
Same question as above for this site: the err_unroll path now mutates the
global flow_block_indr_list without flow_indr_block_lock, and
tcf_block_bind()/tcf_block_unbind() only assert block->cb_lock.

>  		if (i-- > 0) {
>  			list_del(&block_cb->list);
>  			tcf_block_playback_offloads(block, block_cb->cb,