Re: [PATCH 2/2] EFI: further "need_to_free" adjustments

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 14 Oct 2020, Jan Beulich wrote:
> When processing "chain" directives, the previously loaded config file
> gets freed. This needs to be recorded accordingly such that no error
> path would try to free the same block of memory a 2nd time.
> 
> Furthermore, neither .addr nor .size being zero has any meaning towards
> the need to free an allocated chunk anymore. Drop (from read_file()) and
> replace (in Arm's efi_arch_use_config_file(), to sensibly retain the
> comment) respective assignments.
> 
> Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -591,7 +591,7 @@ static bool __init efi_arch_use_config_f
>  
>      fdt = lookup_fdt_config_table(SystemTable);
>      dtbfile.ptr = fdt;
> -    dtbfile.size = 0;  /* Config table memory can't be freed, so set size to 
> 0 */
> +    dtbfile.need_to_free = false; /* Config table memory can't be freed. */
>      if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 
> 0 )
>      {
>          /*
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -601,10 +601,7 @@ static bool __init read_file(EFI_FILE_HA
>                                      PFN_UP(size), &file->addr);
>      }
>      if ( EFI_ERROR(ret) )
> -    {
> -        file->addr = 0;
>          what = what ?: L"Allocation";
> -    }
>      else
>      {
>          file->need_to_free = true;
> @@ -1271,8 +1268,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>              name.s = get_value(&cfg, "global", "chain");
>              if ( !name.s )
>                  break;
> -            efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -            cfg.addr = 0;
> +            if ( cfg.need_to_free )
> +            {
> +                efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> +                cfg.need_to_free = false;
> +            }
>              if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
>              {
>                  PrintStr(L"Chained configuration file '");
>