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

[Simon Glass]

Hi Heinrich,

On Wed, 15 Nov 2023 at 19:06, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/16/23 02:42, Simon Glass wrote:
> > 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.
>
> Please, observe that malloc() and efi_allocate_pages() use completely
> separate parts of the memory.
>
> When reaching ExitBootServices() the memory used by the EFI binary
> (relocated in efi_load_pe()) and for the configuration table with the
> device-tree have been allocated by efi_allocate_pages(). These addresses
> cannot neither allocated by malloc() nor freed with free().

OK...

To my reading, efi_load_pe() pulls the image apart into memory
allocated with efi_allocate_pages(). So that is separate memory? In
any case, it would end up in a 'struct bootflow'.

IMO the EFI memory-allocation functions should be called only when
booting, not before. I do worry about leakage...

[..]

Regards,
Simon