Mailing List Archive: [PATCH v4 1/4] efi/boot.c: add file.need_to

[PATCH v4 1/4] efi/boot.c: add file.need_to_free

Sep 14, 2020, 4:50 AM

Post #1 of 3 (6 views)

The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
xen/common/efi/boot.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..7156139174 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -102,6 +102,7 @@ union string {

struct file {
UINTN size;
+ bool need_to_free;
union {
EFI_PHYSICAL_ADDRESS addr;
void *ptr;
@@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
if ( !efi_bs )
efi_arch_halt();

- if ( cfg.addr )
+ if ( cfg.addr && cfg.need_to_free )
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
- if ( kernel.addr )
+ if ( kernel.addr && kernel.need_to_free )
efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
- if ( ramdisk.addr )
+ if ( ramdisk.addr && ramdisk.need_to_free )
efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
- if ( xsm.addr )
+ if ( xsm.addr && xsm.need_to_free )
efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));

efi_arch_blexit();
@@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
PFN_UP(size), &file->addr);
+ file->need_to_free = true;
}
if ( EFI_ERROR(ret) )
{
--
2.25.1

Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free [ In reply to ]

roger.pau at citrix

Sep 15, 2020, 11:43 PM

Post #2 of 3 (6 views)

Permalink

On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote:
> The config file, kernel, initrd, etc should only be freed if they
> are allocated with the UEFI allocator.
>
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> xen/common/efi/boot.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..7156139174 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>
> struct file {
> UINTN size;
> + bool need_to_free;
> union {
> EFI_PHYSICAL_ADDRESS addr;
> void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
> if ( !efi_bs )
> efi_arch_halt();
>
> - if ( cfg.addr )
> + if ( cfg.addr && cfg.need_to_free )
> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> - if ( kernel.addr )
> + if ( kernel.addr && kernel.need_to_free )
> efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> - if ( ramdisk.addr )
> + if ( ramdisk.addr && ramdisk.need_to_free )
> efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> - if ( xsm.addr )
> + if ( xsm.addr && xsm.need_to_free )
> efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>
> efi_arch_blexit();
> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
> ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> PFN_UP(size), &file->addr);
> + file->need_to_free = true;

Strictly speaking, don't you need to set need_to_free only if
AllocatePages has succeed? I guess it doesn't matter much because addr
would be zapped to 0 if allocation fails.

Thanks, Roger.

Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free [ In reply to ]

jbeulich at suse

Sep 17, 2020, 4:06 AM

Post #3 of 3 (6 views)

Permalink

On 16.09.2020 08:43, Roger Pau Monné wrote:
> On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote:
>> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>> if ( !efi_bs )
>> efi_arch_halt();
>>
>> - if ( cfg.addr )
>> + if ( cfg.addr && cfg.need_to_free )
>> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>> - if ( kernel.addr )
>> + if ( kernel.addr && kernel.need_to_free )
>> efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
>> - if ( ramdisk.addr )
>> + if ( ramdisk.addr && ramdisk.need_to_free )
>> efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
>> - if ( xsm.addr )
>> + if ( xsm.addr && xsm.need_to_free )
>> efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));

All these look to be able to become just "if ( xyz.need_to_free )"
if ...

>> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>> HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>> ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>> PFN_UP(size), &file->addr);
>> + file->need_to_free = true;
>
> Strictly speaking, don't you need to set need_to_free only if
> AllocatePages has succeed?

... this was followed, so I think the adjustment wants making.

> I guess it doesn't matter much because addr
> would be zapped to 0 if allocation fails.

Perhaps this zapping then also becomes unnecessary, albeit I
didn't look very closely yet.

Jan