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

[Ramon Fried]

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