| Message ID | 20200921115113.1278655-3-hudson@trmm.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Unified Xen hypervisor/kernel/initrd images | expand |
On 21.09.2020 13:51, 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> Strictly speaking with the changes done from v5 to v6 this tag would have needed dropping. I guess Roger is fine with it being kept, though. > @@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str) > if ( !efi_bs ) > efi_arch_halt(); > > - if ( cfg.addr ) > + if ( cfg.need_to_free ) > efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > - if ( kernel.addr ) > + if ( kernel.need_to_free ) > efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > - if ( ramdisk.addr ) > + if ( ramdisk.need_to_free ) > efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > - if ( xsm.addr ) > + if ( xsm.need_to_free ) > efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > > efi_arch_blexit(); Doesn't this need similar changes in both efi_arch_blexit()? Jan
On Mon, Sep 21, 2020 at 07:51: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 157fe0e8c5..c2ce0c7294 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; > char *str; > @@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str) > if ( !efi_bs ) > efi_arch_halt(); > > - if ( cfg.addr ) > + if ( cfg.need_to_free ) If you drop the addr check here... > efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > - if ( kernel.addr ) > + if ( kernel.need_to_free ) > efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > - if ( ramdisk.addr ) > + if ( ramdisk.need_to_free ) > efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > - if ( xsm.addr ) > + if ( xsm.need_to_free ) > efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > > efi_arch_blexit(); > @@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > } > else > { > + file->need_to_free = true; ... I think you need to clear need_to_free if AllocatePages fails? Thanks, Roger.
On 29.09.2020 12:45, Roger Pau Monné wrote: > On Mon, Sep 21, 2020 at 07:51:10AM -0400, Trammell Hudson wrote: >> --- 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; >> char *str; >> @@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str) >> if ( !efi_bs ) >> efi_arch_halt(); >> >> - if ( cfg.addr ) >> + if ( cfg.need_to_free ) > > If you drop the addr check here... > >> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); >> - if ( kernel.addr ) >> + if ( kernel.need_to_free ) >> efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); >> - if ( ramdisk.addr ) >> + if ( ramdisk.need_to_free ) >> efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); >> - if ( xsm.addr ) >> + if ( xsm.need_to_free ) >> efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); >> >> efi_arch_blexit(); >> @@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, >> } >> else >> { >> + file->need_to_free = true; > > ... I think you need to clear need_to_free if AllocatePages fails? But this has been moved to the success path, or am I overlooking anything? Jan
On Tue, Sep 29, 2020 at 01:00:06PM +0200, Jan Beulich wrote: > On 29.09.2020 12:45, Roger Pau Monné wrote: > > On Mon, Sep 21, 2020 at 07:51:10AM -0400, Trammell Hudson wrote: > >> --- 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; > >> char *str; > >> @@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str) > >> if ( !efi_bs ) > >> efi_arch_halt(); > >> > >> - if ( cfg.addr ) > >> + if ( cfg.need_to_free ) > > > > If you drop the addr check here... > > > >> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > >> - if ( kernel.addr ) > >> + if ( kernel.need_to_free ) > >> efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > >> - if ( ramdisk.addr ) > >> + if ( ramdisk.need_to_free ) > >> efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > >> - if ( xsm.addr ) > >> + if ( xsm.need_to_free ) > >> efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > >> > >> efi_arch_blexit(); > >> @@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > >> } > >> else > >> { > >> + file->need_to_free = true; > > > > ... I think you need to clear need_to_free if AllocatePages fails? > > But this has been moved to the success path, or am I overlooking > anything? Oh yes, sorry, somehow I looked at the wrong context. Please ignore my comment. Roger.
On Tuesday, September 29, 2020 6:17 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 21.09.2020 13:51, Trammell Hudson wrote: > [...] > > Reviewed-by: Roger Pau Monné roger.pau@citrix.com > > Strictly speaking with the changes done from v5 to v6 this tag > would have needed dropping. I guess Roger is fine with it being > kept, though. Ok. > [...] > Doesn't this need similar changes in both efi_arch_blexit()? It does -- I had missed that there was a place to free them and had assumed that they were just leaked. Fixed in v7. -- Trammell
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 157fe0e8c5..c2ce0c7294 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; char *str; @@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str) if ( !efi_bs ) efi_arch_halt(); - if ( cfg.addr ) + if ( cfg.need_to_free ) efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.addr ) + if ( kernel.need_to_free ) efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.addr ) + if ( ramdisk.need_to_free ) efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.addr ) + if ( xsm.need_to_free ) efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); efi_arch_blexit(); @@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, } else { + file->need_to_free = true; file->size = size; if ( file != &cfg ) {