Re: [PATCH net-next v2 1/2] net/stmmac: Check for STMMAC_DOWN flag in all XDP paths

[Jakub Raczynski]

> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3591755ea30be..3a66f28425275 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5267,6 +5267,9 @@ static int stmmac_xdp_xmit_back(struct stmmac_priv *priv,
> >  	if (unlikely(!xdpf))
> >  		return STMMAC_XDP_CONSUMED;
> >  
> > +	if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
> > +		return STMMAC_XDP_CONSUMED;
> Does returning STMMAC_XDP_CONSUMED here cause a memory leak and a double-free
> in the zero-copy path?
> Since this check occurs after xdp_convert_buff_to_frame() is called earlier
> in stmmac_xdp_xmit_back(), the conversion has already transferred ownership. 
> In zero-copy mode, xdp_convert_buff_to_frame() allocates a new page and frees
> the original buffer.
> Returning STMMAC_XDP_CONSUMED at this point leaks the newly allocated page
> because xdp_return_frame(xdpf) is not called.
> Furthermore, the caller stmmac_rx_zc() handles STMMAC_XDP_CONSUMED by freeing
> the buffer again:
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_zc() {
> 	...
> 	case STMMAC_XDP_CONSUMED:
> 		xsk_buff_free(buf->xdp);
> 		fallthrough;
> 	...
> }
> Could this result in a double-free? Would it be safer to check the STMMAC_DOWN
> flag before the buffer conversion takes place?

Gonna respond to Sashiko review.

Funny how in first version of the patch Sashiko wanted XDP_CONSUMED instead
of error and now it considers it error, but thats beauty of AI. But 1st concern
was valid.

As for this, same issue would be when if(unlikely(!xdpf)) triggers,
or even worse, because it would free page that failed in stmmac_rx_zc().

Issue regarding ownership is valid though and this should be fixed -
it is freed by xdp_convert_buff_to_frame() but is dangling so should either
specifically free xdpf or xdp_return_frame().

"Would checking this before buffer conversion take place be safer?"
Sure, why not, but it will make code bit more ugly.

BR
Jakub Raczynski