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

[Niel Fourie]

Hi Marek,

On 01/12/2022 11:44, Marek Vasut wrote:
> On 12/1/22 09:24, Lukasz Majewski wrote:
>> On Wed, 30 Nov 2022 17:42:25 +0100
>> Niel Fourie <lusus at 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 at denx.de>
>>> Cc: Ramon Fried <rfried.dev at gmail.com>
>>> Cc: Marek Vasut <marex at denx.de>
>>> Cc: Lukasz Majewski <lukma at 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 at 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.

The reason is simple, the regular ethernet drivers do not get removed on 
stop() like for the gadget ethernet driver, and in their case `priv` is 
still valid.

I agree on your point that reworking the ethernet gadget code would be 
preferable, but this would be a much bigger effort (and if I were to do 
it, I would probably introduce even more bugs). I am not certain whether 
this would not also affect the non-DM gadget implementation as well, 
which still contain drivers like ci_udc.c which does not appear to have 
been ported to DM gadget yet? (I only see DM USB there...)

That said, I am not certain whether this is not also bug, as I am not 
certain whether the assumption that `priv` should be available after 
stop() is valid or not.

The documentation at 
https://source.denx.de/u-boot/u-boot/-/blob/master/doc/develop/driver-model/ethernet.rst?plain=1#L135 
states:

The **stop** function should turn off / disable the hardware and place 
it back in its reset state.  It can be called at any time (before any 
call to the related start() function), so make sure it can handle this 
sort of thing.

In such a complete reset state I am not certain whether the assumption 
that `priv` should exist is still valid, at least not without another 
call to dev_get_uclass_priv() and revalidating it first?

Lastly, this assumption that priv is still valid is rather new and it 
was introduced here:

https://source.denx.de/u-boot/u-boot/-/commit/fa795f452541ce07b33be603de36cac3c5d7dfcf

This commit appears to be a workaround for drivers which cannot deal 
with stop() being called at any time as required in the above quoted 
documentation.

I would consider adding a workaround to a workaround in this case to be 
the lesser evil, as tracking down this bug in the first place was like 
looking for a needle in a haystack. This change would at least save 
everybody else from strange crashes in particular configurations without 
any negative impact. But this is fortunately not my decision. :)

Best regards,
Niel Fourie

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80  Email: lusus at denx.de