Re: [PATCH] net: eth-uclass: change state before stop() in eth_halt()

[Marek Vasut <marex@denx.de>]

On 12/1/22 09:24, Lukasz Majewski wrote:
> On Wed, 30 Nov 2022 17:42:25 +0100
> Niel Fourie <lusus@denx.de> wrote:
> 
>> In eth_halt(), change the private uclass state before calling
>> stop() instead of afterwards, to avoid writing to memory which
>> may have been freed during stop().
>>
>> In the ethernet gadget implementation, the gadget device gets
>> probed during start() and removed during stop(), which includes
>> freeing `uclass_priv_` to which `priv` is pointing. Writing to
>> `priv` after stop() may corrupt the `fd` member of `struct
>> malloc_chunk`, which represents the freed block, and could cause
>> hard-to-debug crashes on subsequent calls to malloc()/free().
>>
>> Signed-off-by: Niel Fourie <lusus@denx.de>
>> Cc: Ramon Fried <rfried.dev@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Lukasz Majewski <lukma@denx.de>
>> ---
>>   net/eth-uclass.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index f41da4b37b3..bc3b9751e32 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -342,9 +342,9 @@ void eth_halt(void)
>>   	if (!priv || !priv->running)
>>   		return;
>>   
>> -	eth_get_ops(current)->stop(current);
>>   	priv->state = ETH_STATE_PASSIVE;
>>   	priv->running = false;
>> +	eth_get_ops(current)->stop(current);
>>   }
>>   
>>   int eth_is_active(struct udevice *dev)
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>

How come nobody triggered this problem with regular ethernet in U-Boot ?

If this is isolated to USB gadget ethernet, then please do not hack 
around this in core networking code, but rather fix the USB ethernet 
gadget itself. It seems that gadget code should not unregister the 
gadget in drivers/usb/gadget/ether.c _usb_eth_halt() , at least not fully.