On Thu, Jun 30, 2016 at 06:47:02PM +0200, Ard Biesheuvel wrote: > On 30 June 2016 at 18:27, Mark Rutland <mark.rutland@xxxxxxx> wrote: > > Hi, > > > > [Adding Ard and Leif] > > > > On Thu, Jun 30, 2016 at 09:35:33AM -0600, Jeff Hugo wrote: > >> From: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> > >> > >> There exists a race condition between when the efi stub grabs the memory > >> map, and when ExitBootServices is called at which point the EFI can process > >> an event which causes the memory map to be invalidated. > > > > For reference, do you have a particular example of such an event? > > > > Do these events cause the memory map to grow? > > > > Events are typically allowed to allocate or free memory, unless they > have the EVT_SIGNAL_EXIT_BOOT_SERVICES attribute. Whether they cause > the memory map to grow is hard to predict, so one must assume yes. Ok. > > That "partial shutdown" also means that giving up after a failed > > ExitBootServices() call is difficult. We can't log anything, and > > whatever we return to can't call any boot services. > > This is the unfortunate part: we lost our console so there is nothing > we can do except hang, or proceed without a memory map. Since we have > already allocated space for the static kernel image in this case, it > may be better to proceed so we can at least die loudly on earlycon > enabled configurations. Hmm... That might be best, assuming we can somehow signal to the kernel that something hass gone very wrong. > >> However the only alternative I can think of it to allocate a sufficently large > >> buffer so that it can be reused to hold the modified memory map. There doesn't > >> seem to be any limit to the new map, so any buffer space value I would choose > >> would be arbitrary, and there would be some chance that it would be insufficent. > >> efi_get_memory_map() would need to be modified to "return" the origional size > >> of the allocated buffer as well, so I feel like this solution makes a mess of > >> the code, reduces the value of the efi_get_memory_map() helper function, and for > >> all that effort, we still can't fully address this race condition. > >> > >> I guess the question is, where do we draw the line at "good enough" for > >> addressing this issue? Do we use efi_get_memory_map() since it appears to be > >> cleaner and does not seem to cause a problem today, despite violating the spec? > > > > We shouldn't be knowingly violating the UEFI spec. > > > > At the very least, this should be raised with the USWG. This feels like > > a specification bug, given that it's impossible (rather than simply > > difficult) to solve the issue. > > efi_get_memory_map() is the linux wrapper around the GetMemoryMap() > boot service, and the latter does not perform any allocations. The > spec also clearly states that GetMemoryMap() can be called after EBS() > has failed. Sure, I understood that. I find it surprising that the spec rules out the use of the memory allocation services that efi_get_memory_map() uses under the hood, given that I imagine those don't require asynchronous processing, and having those would cater for more extreme cases. > > Ideally, we have the rules regarding a failed call to ExitBootServices() > > tightened such that other boot services calls are valid. The current > > wording appears to result in a number of unsolvable issues. > > The only unsolvable issue is that we may find that we did not allocate > sufficient slack to cover the updated memory map. Typically, a > periodic event that happens to allocate and/or free some memory in its > handler may result in one or two entries to be added, but it is > bounded in practice even if the spec does not spell it out explicitly. > > So allocating a couple of entries' worth of slack should be sufficient > in virtually all cases. I agree that this is likely to work in practice, given some arbitrary amount of slack. I still think it's worth poking the USWG about this, as they may care about fixing this in the more general case spec-side. Even if we can't change things, it seems worth a note that the size of the memory map may grow. > >> Second issue- > >> When do we give up if we cannot get a good memory map to use with > >> ExitBootServices? Currently there is an infinite loop in my patch. I noticed > >> arch/x86/boot/compressed/eboot.c only retrys after the first failure. I think > >> a single retry is insufficent, we could do better, but I'm aware that an > >> infinite loop is generally a bad idea. Any strong opinions on when to quit? > >> 100 attempts? Either way, it seems the system will require a hard reboot if > >> the retry(s) ever end up failing. > > > > I think this depends on what the problematic events are. > > The wording of the spec suggests that two attempts at the most covers > all cases, and the EDK2 implementation confirms that: the first thing > it does is disarm the timer, and since all asynchronous processing in > UEFI is event based (no interrupts except for the timer or for debug), > this guarantees that the race condition that hit us the first time > does not exist anymore the second time around. > > I know this is all a bit hand wavy, but I never experienced the issue > in practice (to my knowledge) and I don't think it makes a huge lot of > sense to complicate this code too much only to cater for theoretical > spec violations. Understood. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
