Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[Simon Horman <simon.horman@corigine.com>]

On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote:
> Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
> flow_block_cb_incref unconditionally, even for a newly allocated cb.
> Fixes a use-after-free bug. Also fix the accidentally inverted refcount
> check on unbind.

Hi Felix,

my 2c worth.

Firstly, it's usually best to have one fix per patch.

Second, this seems to conflicts with:

  [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for
        offloading flows from wlan devices

So I guess that series will need to be rebased on this one at some point.

> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  .../net/ethernet/mediatek/mtk_ppe_offload.c   | 23 +++++++++++--------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 81afd5ee3fbf..43cc510b51bd 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	struct mtk_eth *eth = mac->hw;
>  	static LIST_HEAD(block_cb_list);
>  	struct flow_block_cb *block_cb;
> +	bool register_block = false;
>  	flow_setup_cb_t *cb;
>  
>  	if (!eth->soc->offload_version)
> @@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	switch (f->command) {
>  	case FLOW_BLOCK_BIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
> -		if (block_cb) {
> -			flow_block_cb_incref(block_cb);
> -			return 0;
> +		if (!block_cb) {
> +			block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> +			if (IS_ERR(block_cb))
> +				return PTR_ERR(block_cb);
> +
> +			register_block = true;
>  		}
> -		block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> -		if (IS_ERR(block_cb))
> -			return PTR_ERR(block_cb);
>  
> -		flow_block_cb_add(block_cb, f);
> -		list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		flow_block_cb_incref(block_cb);
> +
> +		if (register_block) {
> +			flow_block_cb_add(block_cb, f);
> +			list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		}
>  		return 0;

I think the existing code was more idiomatic, and could
be maintained by simply adding a call to flow_block_cb_incref()
before the call to flow_block_cb_add().

@@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
 		if (IS_ERR(block_cb))
 			return PTR_ERR(block_cb);
 
+		flow_block_cb_incref(block_cb);
 		flow_block_cb_add(block_cb, f);
 		list_add_tail(&block_cb->driver_list, &block_cb_list);
 		return 0;

>  	case FLOW_BLOCK_UNBIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
>  		if (!block_cb)
>  			return -ENOENT;
>  
> -		if (flow_block_cb_decref(block_cb)) {
> +		if (!flow_block_cb_decref(block_cb)) {
>  			flow_block_cb_remove(block_cb, f);
>  			list_del(&block_cb->driver_list);
>  		}
> -- 
> 2.39.0
>