[PATCH] net: flow_offload: clean up indirect flow block on bind failure

[PATCH] net: flow_offload: clean up indirect flow block on bind failure

[Shuangpeng Bai]

flow_indr_dev_setup_offload() records FLOW_BLOCK_BIND requests in
flow_indir_dev_list before it calls registered indirect providers. When no
registered provider binds the block, it returns -EOPNOTSUPP. The replay
entry is left behind.

One affected path is nftables netdev offload. A netdev base chain on a
device without ndo_setup_tc can take the indirect path if an indirect
provider is registered. If none of the registered providers binds the
block, the operation fails with -EOPNOTSUPP.

The failed NEWCHAIN transaction can then abort and free the nft_base_chain,
while the replay entry still points at that basechain and its flow_block
cb_list. If another provider is registered later, the stale entry is
replayed, potentially passing freed data to the provider callback or
splicing callback entries into freed memory.

Use the return value of indir_dev_add() only to track ownership of the
replay entry. Preserve the existing behavior of not propagating -EEXIST
or -ENOMEM. If this invocation inserted an entry, remove it again when
the bind fails.

Fixes: 74fc4f828769 ("net: Fix offloading indirect devices dependency on qdisc order creation")

Cc: stable@vger.kernel.org
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 net/core/flow_offload.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 5071d7fe6ce2..5fbb1ae4679d 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -606,15 +606,20 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
 				void (*cleanup)(struct flow_block_cb *block_cb))
 {
 	struct flow_indr_dev *this;
+	bool replay_added = false;
 	u32 count = 0;
 	int err;
 
 	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) {
+			int add_err;
+
+			add_err = indir_dev_add(data, dev, sch, type, cleanup, bo);
+			replay_added = add_err == 0;
+		} else if (bo->command == FLOW_BLOCK_UNBIND) {
 			indir_dev_remove(data);
+		}
 	}
 
 	list_for_each_entry(this, &flow_block_indr_dev_list, list) {
@@ -623,9 +628,17 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
 			count++;
 	}
 
+	if (bo && list_empty(&bo->cb_list)) {
+		if (replay_added)
+			indir_dev_remove(data);
+		err = -EOPNOTSUPP;
+	} else {
+		err = count;
+	}
+
 	mutex_unlock(&flow_indr_block_lock);
 
-	return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
+	return err;
 }
 EXPORT_SYMBOL(flow_indr_dev_setup_offload);
 
-- 
2.43.0

Re: [PATCH] net: flow_offload: clean up indirect flow block on bind failure

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: flow_offload: clean up indirect flow block on bind failure

This patch tries to fix a use-after-free in the nftables netdev offload
path by removing the replay entry that flow_indr_dev_setup_offload()
inserts into flow_indir_dev_list when no registered indirect provider
binds the block and the call returns -EOPNOTSUPP.

> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 5071d7fe6ce2..5fbb1ae4679d 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c
> @@ -606,15 +606,20 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
>  				void (*cleanup)(struct flow_block_cb *block_cb))
>  {
>  	struct flow_indr_dev *this;
> +	bool replay_added = false;
>  	u32 count = 0;
>  	int err;
>  
>  	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) {
> +			int add_err;
> +
> +			add_err = indir_dev_add(data, dev, sch, type, cleanup, bo);
> +			replay_added = add_err == 0;
> +		} else if (bo->command == FLOW_BLOCK_UNBIND) {
>  			indir_dev_remove(data);
> +		}
>  	}
>  
>  	list_for_each_entry(this, &flow_block_indr_dev_list, list) {
> @@ -623,9 +628,17 @@ int flow_indr_dev_setup_offload(struct net_device *dev,	struct Qdisc *sch,
>  			count++;
>  	}
>  
> +	if (bo && list_empty(&bo->cb_list)) {
> +		if (replay_added)
> +			indir_dev_remove(data);
> +		err = -EOPNOTSUPP;
> +	} else {
> +		err = count;
> +	}

[High]
Does this change reintroduce the exact scenario that commit
74fc4f828769 ("net: Fix offloading indirect devices dependency on qdisc
order creation") was written to address?

The purpose of flow_indir_dev_list is to remember BIND requests made
when no indirect provider has registered yet, so that a driver
registering later via flow_indr_dev_register() can replay them through
existing_qdiscs_register().

Consider a TC ingress qdisc on a device without ndo_setup_tc that is
created before any indirect provider registers.  With this patch:

  - indir_dev_add() succeeds, so replay_added = true
  - flow_block_indr_dev_list is empty, so no cb runs
  - bo->cb_list is therefore empty
  - the newly-inserted replay entry is immediately removed via
    indir_dev_remove(data)

When a driver (for example mlx5/nfp/bnxt/ice/sfc) later calls
flow_indr_dev_register(), existing_qdiscs_register() will not find the
qdisc and the hardware offload will silently never happen.

For the TC path, tcf_block_offload_cmd() calls
flow_indr_dev_setup_offload() and its return value is effectively
absorbed by tcf_block_offload_bind(), which treats -EOPNOTSUPP as
"no offload yet" via the no_offload_dev_inc path; the tcf_block itself
stays alive.  The replay entry it inserted is still valid state that
future provider registrations depend on.

Could the fix be scoped only to the caller that actually frees the
block on -EOPNOTSUPP (nftables netdev), for example by having nft's
abort path call indir_dev_remove() explicitly, or by adding an opt-in
flag on flow_block_offload so that only callers that will free their
state on failure request the cleanup?

As written, the cleanup fires unconditionally for every BIND that
produces -EOPNOTSUPP, including TC, tc actions and nf_flow_table paths
that legitimately keep their state alive on -EOPNOTSUPP and rely on
the replay entry being preserved.

>  
>  	mutex_unlock(&flow_indr_block_lock);
>  
> -	return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
> +	return err;
>  }
>  EXPORT_SYMBOL(flow_indr_dev_setup_offload);
>
-- 
pw-bot: cr