| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0204b4f2-98b3-4463-9ae7-fc3657ce2fc1@suse.de>
Date: Sun, 21 Dec 2025 17:18:28 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: javierm@...hat.com, arnd@...db.de, richard.lyu@...e.com,
helgaas@...nel.org, x86@...nel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org,
loongarch@...ts.linux.dev, linux-riscv@...ts.infradead.org,
dri-devel@...ts.freedesktop.org, linux-hyperv@...r.kernel.org,
linux-pci@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 9/9] efi: libstub: Simplify interfaces for
primary_display
Hi
Am 16.12.25 um 14:23 schrieb Ard Biesheuvel:
> Hi Thomas
>
> On Wed, 26 Nov 2025 at 17:09, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Rename alloc_primary_display() and __alloc_primary_display(), clarify
>> free semantics to make interfaces easier to understand.
>>
>> Rename alloc_primary_display() to lookup_primary_display() as it
>> does not necessarily allocate. Then rename __alloc_primary_display()
>> to the new alloc_primary_display(). The helper belongs to
>> free_primary_display), so it should be named without underscores.
>>
>> The lookup helper does not necessarily allocate, so the output
>> parameter needs_free to indicate when free should be called.
> I don't understand why we need this. Whether or not the helper
> allocates is a compile time decision, and in builds where it doesn't,
> the free helper doesn't do anything.
>
> I'm all for making things simpler, but I don't think this patch
> achieves that tbh.
>
> I've queued up this series now up until this patch - once we converge
> on the simplification, I'm happy to apply it on top.
If you don't want this patch, just leave it out then. Coming from
another subsystem, I found the current logic and naming confusing THB.
Best regards
Thomas
>
> Thanks,
>
>
>
>> Pass
>> an argument through the calls to track this state. Put the free
>> handling into release_primary_display() for simplificy.
>>
>> Also move the comment fro primary_display.c to efi-stub-entry.c,
>> where it now describes lookup_primary_display().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@...e.de>
>> ---
>> drivers/firmware/efi/libstub/efi-stub-entry.c | 23 +++++++++++++++++--
>> drivers/firmware/efi/libstub/efi-stub.c | 22 ++++++++++++------
>> drivers/firmware/efi/libstub/efistub.h | 2 +-
>> .../firmware/efi/libstub/primary_display.c | 17 +-------------
>> drivers/firmware/efi/libstub/zboot.c | 6 +++--
>> 5 files changed, 42 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-entry.c b/drivers/firmware/efi/libstub/efi-stub-entry.c
>> index aa85e910fe59..3077b51fe0b2 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-entry.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-entry.c
>> @@ -14,10 +14,29 @@ static void *kernel_image_addr(void *addr)
>> return addr + kernel_image_offset;
>> }
>>
>> -struct sysfb_display_info *alloc_primary_display(void)
>> +/*
>> + * There are two ways of populating the core kernel's sysfb_primary_display
>> + * via the stub:
>> + *
>> + * - using a configuration table, which relies on the EFI init code to
>> + * locate the table and copy the contents; or
>> + *
>> + * - by linking directly to the core kernel's copy of the global symbol.
>> + *
>> + * The latter is preferred because it makes the EFIFB earlycon available very
>> + * early, but it only works if the EFI stub is part of the core kernel image
>> + * itself. The zboot decompressor can only use the configuration table
>> + * approach.
>> + */
>> +
>> +struct sysfb_display_info *lookup_primary_display(bool *needs_free)
>> {
>> + *needs_free = true;
>> +
>> if (IS_ENABLED(CONFIG_ARM))
>> - return __alloc_primary_display();
>> + return alloc_primary_display();
>> +
>> + *needs_free = false;
>>
>> if (IS_ENABLED(CONFIG_X86) ||
>> IS_ENABLED(CONFIG_EFI_EARLYCON) ||
>> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
>> index 42d6073bcd06..dc545f62c62b 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub.c
>> @@ -51,14 +51,14 @@ static bool flat_va_mapping = (EFI_RT_VIRTUAL_OFFSET != 0);
>> void __weak free_primary_display(struct sysfb_display_info *dpy)
>> { }
>>
>> -static struct sysfb_display_info *setup_primary_display(void)
>> +static struct sysfb_display_info *setup_primary_display(bool *dpy_needs_free)
>> {
>> struct sysfb_display_info *dpy;
>> struct screen_info *screen = NULL;
>> struct edid_info *edid = NULL;
>> efi_status_t status;
>>
>> - dpy = alloc_primary_display();
>> + dpy = lookup_primary_display(dpy_needs_free);
>> if (!dpy)
>> return NULL;
>> screen = &dpy->screen;
>> @@ -68,15 +68,22 @@ static struct sysfb_display_info *setup_primary_display(void)
>>
>> status = efi_setup_graphics(screen, edid);
>> if (status != EFI_SUCCESS)
>> - goto err_free_primary_display;
>> + goto err___free_primary_display;
>>
>> return dpy;
>>
>> -err_free_primary_display:
>> - free_primary_display(dpy);
>> +err___free_primary_display:
>> + if (*dpy_needs_free)
>> + free_primary_display(dpy);
>> return NULL;
>> }
>>
>> +static void release_primary_display(struct sysfb_display_info *dpy, bool dpy_needs_free)
>> +{
>> + if (dpy && dpy_needs_free)
>> + free_primary_display(dpy);
>> +}
>> +
>> static void install_memreserve_table(void)
>> {
>> struct linux_efi_memreserve *rsv;
>> @@ -156,13 +163,14 @@ efi_status_t efi_stub_common(efi_handle_t handle,
>> char *cmdline_ptr)
>> {
>> struct sysfb_display_info *dpy;
>> + bool dpy_needs_free;
>> efi_status_t status;
>>
>> status = check_platform_features();
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> - dpy = setup_primary_display();
>> + dpy = setup_primary_display(&dpy_needs_free);
>>
>> efi_retrieve_eventlog();
>>
>> @@ -182,7 +190,7 @@ efi_status_t efi_stub_common(efi_handle_t handle,
>>
>> status = efi_boot_kernel(handle, image, image_addr, cmdline_ptr);
>>
>> - free_primary_display(dpy);
>> + release_primary_display(dpy, dpy_needs_free);
>>
>> return status;
>> }
>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> index 979a21818cc1..1503ffb82903 100644
>> --- a/drivers/firmware/efi/libstub/efistub.h
>> +++ b/drivers/firmware/efi/libstub/efistub.h
>> @@ -1176,8 +1176,8 @@ efi_enable_reset_attack_mitigation(void) { }
>>
>> void efi_retrieve_eventlog(void);
>>
>> +struct sysfb_display_info *lookup_primary_display(bool *needs_free);
>> struct sysfb_display_info *alloc_primary_display(void);
>> -struct sysfb_display_info *__alloc_primary_display(void);
>> void free_primary_display(struct sysfb_display_info *dpy);
>>
>> void efi_cache_sync_image(unsigned long image_base,
>> diff --git a/drivers/firmware/efi/libstub/primary_display.c b/drivers/firmware/efi/libstub/primary_display.c
>> index cdaebab26514..34c54ac1e02a 100644
>> --- a/drivers/firmware/efi/libstub/primary_display.c
>> +++ b/drivers/firmware/efi/libstub/primary_display.c
>> @@ -7,24 +7,9 @@
>>
>> #include "efistub.h"
>>
>> -/*
>> - * There are two ways of populating the core kernel's sysfb_primary_display
>> - * via the stub:
>> - *
>> - * - using a configuration table, which relies on the EFI init code to
>> - * locate the table and copy the contents; or
>> - *
>> - * - by linking directly to the core kernel's copy of the global symbol.
>> - *
>> - * The latter is preferred because it makes the EFIFB earlycon available very
>> - * early, but it only works if the EFI stub is part of the core kernel image
>> - * itself. The zboot decompressor can only use the configuration table
>> - * approach.
>> - */
>> -
>> static efi_guid_t primary_display_guid = LINUX_EFI_PRIMARY_DISPLAY_TABLE_GUID;
>>
>> -struct sysfb_display_info *__alloc_primary_display(void)
>> +struct sysfb_display_info *alloc_primary_display(void)
>> {
>> struct sysfb_display_info *dpy;
>> efi_status_t status;
>> diff --git a/drivers/firmware/efi/libstub/zboot.c b/drivers/firmware/efi/libstub/zboot.c
>> index 4b76f74c56da..c1fd1fdbcb08 100644
>> --- a/drivers/firmware/efi/libstub/zboot.c
>> +++ b/drivers/firmware/efi/libstub/zboot.c
>> @@ -26,9 +26,11 @@ void __weak efi_cache_sync_image(unsigned long image_base,
>> // executable code loaded into memory to be safe for execution.
>> }
>>
>> -struct sysfb_display_info *alloc_primary_display(void)
>> +struct sysfb_display_info *lookup_primary_display(bool *needs_free)
>> {
>> - return __alloc_primary_display();
>> + *needs_free = true;
>> +
>> + return alloc_primary_display();
>> }
>>
>> asmlinkage efi_status_t __efiapi
>> --
>> 2.51.1
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
Powered by blists - more mailing lists