[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Heinrich,

On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/15/23 17:23, Heinrich Schuchardt wrote:
> > On 11/15/23 16:50, Simon Glass wrote:
> >> Hi Heinrich,
> >>
> >> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> wrote:
> >>>
> >>>
> >>>
> >>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
> >>> <sjg at chromium.org>:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
> >>>> <xypron.glpk at gmx.de> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
> >>>>> <sjg at chromium.org>:
> >>>>>> When a USB device is unbound, it causes any bootflows attached to
> >>>>>> it to
> >>>>>> be removed, via a call to bootdev_clear_bootflows() from
> >>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >>>>>> bootflow.
> >>>>>>
> >>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
> >>>>>> called, which unbinds the device. For EFI, this happens in
> >>>>>> efi_exit_boot_services() which means that the bootflow disappears
> >>>>>> before it is finished with.
> >>>>>
> >>>>> After ExitBootServices() no driver of U-Boot can be used as the
> >>>>> operating system is in charge.
> >>>>>
> >>>>> Any bootflow inside U-Boot is terminated by definition when
> >>>>> reaching ExitBootServices.
> >>>>>
> >>>>>>
> >>>>>> There is no need to unbind all the USB devices just to quiesce them.
> >>>>>> Add a new usb_pause() call which removes them but leaves them bound.
> >>>>>
> >>>>> As U-Boot must not access any device after ExitBootServices() it is
> >>>>> unclear to me what you want to achieve.
> >>>>
> >>>> I can't remember exactly what is needed from the bootflow, but
> >>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
> >>>> have been better if I had mentioned this explicitly,  But then this
> >>>> patch has been sitting around for ages...
> >>>>
> >>>> In any case, the intent of exit-boot-services is not to free all the
> >>>> memory used, since some of it may have been used to hold data that the
> >>>> app continues to use.
> >>>
> >>> The EFI application reads the memory map and receives an ID which it
> >>> passes to ExitBootServiced() after this point any memory not marked
> >>> as EFI runtime can be used by the EFI app. This includes the memory
> >>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
> >>> here.
> >>>
> >>> Starting the EFI app via StartImage() must  terminate any running
> >>> U-Boot "boot flow".
> >>>
> >>> After ExitBootServices() the EFI application cannot return to U-Boot
> >>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
> >>>
> >>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
> >>>
> >>>
> >>>
> >>>>
> >>>> Also there is U-Boot code between when the devices are unbound and
> >>>> when U-Boot actually exits back to the app.
> >>>>
> >>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
> >>>> quite a while to find.
> >>>
> >>> We need a better understanding of the problem that you experience to
> >>> find an adequate solution. Why does removing all devices lead to
> >>> hanging the system?
> >>
> >> Are you able to test this? With your better knowledge of EFI you might
> >> be able to figure out something else that is going on. But in my case
> >> it causes some memory to be freed (perhaps the memory holding the EFI
> >> app?), which is then overwritten by something else being malloc()'d,
> >
> > The memory for the EFI app is not assigned via malloc(). It is allocated
> > by AllocatedPages().
> >
> > Reading "some memory freed" does not give me confidence that the problem
> > is sufficiently analyzed.
> >
> >> so the boot hangs. It is hard to see what is going on after the app
> >> starts.
> >>
> >> This patch was sent almost two months ago and fixes a real bug. The
> >> first few versions attracted no comment now it is being blocked
> >> because you don't understand how it fixes anything.
> >
> > Do you understand why unbinding the devices causes the problem?
> >
> >>
> >> I can get the hardware up again and try this but it will take a while.
> >
> > Digging a bit deeper seems to be the right approach.
>
> Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
> https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/
>
> points to a probable root cause:
>
> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
> free(bflow->buf)
>
> In the EFI boot bflow->buf points to $kernel_addr_r which never was
> allocated and therefore must not be freed.

Yes, this is the root cause of the crash.

However, this patch should still be applied. I can update the commit
message if you like.

Freeing the FDT and kernel before booting them is just not safe,
whether EFI or anything else. Freed memory is not guaranteed to hang
around for any length of time. For example, with truetype fonts,
malloc() is called during any console output and will likely corrupt
the images just as they are being booted.

We should leave the devices bound when booting.

Regards,
Simon