Adding a few others in case interested... On Thu, 14 Sep 2023 18:47:13 +0200 Daniel Kiper <dki...@net-space.pl> wrote:
> On Wed, Jul 26, 2023 at 03:59:58PM -0500, Glenn Washburn wrote: > > This patch series (for me) was motivated by the "gdb: Add more support for > > debugging on EFI platforms" patch series, which allowed printing in early > > EFI init of the GDB command for properly loading symbols. That approach of > > having the functionality be included at compile time was ultimately > > rejected. During the discussion of that series, Robbie suggested[1] using > > patches by Peter and in the Redhat downstream repo which uses an EFI > > variable to store a GRUB env block. Using this, a user could store a > > variable in the env block stored in the EFI variable and then have GRUB > > load that env block in early init as a way to enable the printing of the > > GDB command. > > > > I've taken the original patches by Peter, made more suitable for including > > in GRUB, fixed some bugs, and added a minor feature. The first patch, adds > > env block loading in early EFI init from the GRUB_ENV EFI variable. The > > second patch is included to provide tools for GRUB to set this EFI env > > block itself, as opposed to needing to use the method suggested by > > Robbie[1], which requires GNU/Linux and the user-space grub2-editenv > > utility. Of course, if GRUB is crashing before one can run the > > efi-export-env command, then other ways of creating and setting the EFI > > envblk variable are necessary. The third patch adds a test which checks the > > usability of the commands and the early init loading. And the last patch, > > whichvmotivated this series, prints the GDB command string if the GRUB > > variable named "enable_earlyinit_gdbinfo" is present and is set to "1", > > after > > the env block is loaded from the GRUB_ENV EFI variable. We might want a > > different name for the GRUB variable, I don't really have a preference. > > I am torn apart. On one hand it is nice feature. On the other hand > it requires a lot of changes. So, I would prefer to defer merging this > thing after the release. Until you convince me I should do otherwise... I would contend that there's only few small changes that matter. Most of the changes are only executed if the user explicitly does something (ie set an EFI variable), so its opt-in mostly. And it would be nearly impossible to accidentally opt-in (especially considering there's no documentation on the name of the special variable "enable_earlyinit_gdbinfo", which I'm still not convinced is the right name). And the changes that do matter are in old code that I'd have expected to shown an issue by now. Here's my analysis of those. Patch 1: * grub_efi_set_variable() is added to grub_efi_init(). This looks scary, but all its doing is calling grub_efi_get_variable() and returning (assuming the user has not done the manual opt in previously). grub_efi_get_variable() is small and well-used. I would say this is low risk. Patch 2: * grub_efi_set_variable_with_attributes() is changed to have a different return value (ie to succeed) when the datasize is 0 and the variable does not exist. There are very few code paths to this function. I've checked the existing ones and none present a problem. Patch 3: * No changes that matter. Patch 4: * grub_env_get() is called in grub_efi_init(). The initial envblk is setup at load time to have no vars (in the BSS), so this call will fail quickly if using the initial envblk, which should be the case, unless the user has explicitly already run the "efi-export-env" command (which means there wasn't a failure on a previous run up to this point with these changes). The "if" block after will never be executed nor will with grub_strcmp(), unless there was explicit interaction as stated above. So the vast majority of new/"untested" code (though I have tested with the test in patch 3), never gets run unless the user opts in. In that case, we do not want to make the situation worse, but its already in a state if they are wanting to enable this feature (ie can't get to shell to run gdbinfo). Supposing that there is a bug in the "opt-in" code paths that cause GRUB to not reach the shell, the user then needs to run either the EFI shell or some other bootloader that can delete UEFI environment variables. While this could be a pain, I would expect this to be a reasonable ask when GRUB is already in a very messed up state (and a very low probability of this situation ever even occurring). These don't appear to be the kind of changes that would have issue with non-x86 architectures (for which it hasn't been tested on _baremetal_, with QEMU it has). And considering that UEFI variable getting/setting has been around a long time, I'd expect that to be well tested and not a source of risk here. I'll admit that including these changes, will probably be low impact as this is for really bad situations. However, that's precisely when you want these kinds of changes. And I think its worth considering that there's a large base of users using similar code via RedHat's patched GRUB. So I think its a pretty low risk overall. Since this is something I expect to be used more by the distros (I doubt we'll see anyone needing this kind of help here), I wonder if they have any thoughts on this issue? Glenn _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
