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

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