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

[Paolo Abeni]

On 6/3/26 1:23 PM, Jakub Raczynski wrote:
>>> 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().

AFAICS when xdp_convert_buff_to_frame() returns NULL, it did not free
the xdp_buff; there should be ownership nor double free issue there.

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

There is also another suggestion: consolidate the return path with the
existing one for `res == STMMAC_XDP_CONSUMED && zc`.

/P