On Fri, Dec 2, 2022 at 12:36 PM Niel Fourie lusus@denx.de wrote:
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@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.
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/... 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/fa795f452541ce07b33be603de36ca...
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@denx.de
I will accept this patch, but please add some comment in the code about why the order is important.