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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote:
> 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.  
> 
> Firstly, it's usually best to have one fix per patch.

Or at least reword the commit message to make it look like it's fixing
the refcounting logic?

> >  		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;

That'd be my go to fix as well, FWIW.

Alternatively - the block cannot be removed until FLOW_BLOCK_UNBIND is
called, right? So the register_block bool can be skipped and
refcount taken after flow_block_cb_add() / list_add_tail().
That way at least the new block handling doesn't have to be split
into two chunks.