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

[Jakub Kicinski <kuba@kernel.org>]

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