Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
On Wed, Nov 15, 2023 at 06:35:23PM -0700, Simon Glass wrote: > EFI applications can be very large and thus used to cause boot failures > when malloc() space was exhausted. > > A recent changed fixed this by using the kernel_addr_r environment var > as the address of the buffer. However, it still frees the buffer when > the bootflow is discarded. > > Fix this by introducing a flag to indicate whether the buffer was > allocated, or not. > > Note that kernel_addr_r is not the last word here. It might be better > to use lmb to place images. But there is a lot of refactoring to do > before we can remove the environment variables. The distro scripts rely > on them so it is safe for bootstd to do so too. > > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file > > Signed-off-by: Simon Glass > Reported by: Simon Glass > Reported by: Shantur Rathore > Reviewed-by: Heinrich Schuchardt > Tested-by: Shantur Rathore Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
On Thu, Nov 16, 2023 at 1:35 AM Simon Glass wrote:
>
> EFI applications can be very large and thus used to cause boot failures
> when malloc() space was exhausted.
>
> A recent changed fixed this by using the kernel_addr_r environment var
> as the address of the buffer. However, it still frees the buffer when
> the bootflow is discarded.
>
> Fix this by introducing a flag to indicate whether the buffer was
> allocated, or not.
>
> Note that kernel_addr_r is not the last word here. It might be better
> to use lmb to place images. But there is a lot of refactoring to do
> before we can remove the environment variables. The distro scripts rely
> on them so it is safe for bootstd to do so too.
>
> Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
>
> Signed-off-by: Simon Glass
> Reported by: Simon Glass
> Reported by: Shantur Rathore
Tested-by: Shantur Rathore
> ---
>
> boot/bootflow.c | 3 ++-
> boot/bootmeth_efi.c | 1 +
> include/bootflow.h | 5 -
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 6922e7e0c4e7..1ea2966ae9a5 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> free(bflow->name);
> free(bflow->subdir);
> free(bflow->fname);
> - free(bflow->buf);
> + if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> + free(bflow->buf);
> free(bflow->os_name);
> free(bflow->fdt_fname);
> free(bflow->bootmeth_priv);
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index ae936c8daa18..9ba7734911e1 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow,
> ulong addr)
> if (ret)
> return log_msg_ret("read", ret);
> bflow->buf = map_sysmem(addr, bflow->size);
> + bflow->flags |= BOOTFLOWF_STATIC_BUF;
>
> set_efi_bootdev(desc, bflow);
>
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 44d3741eacae..fede8f22a2b8 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -43,9 +43,12 @@ enum bootflow_state_t {
> * and it is using the prior-stage FDT, which is the U-Boot control FDT.
> * This is only possible with the EFI bootmeth (distro-efi) and only when
> * CONFIG_OF_HAS_PRIOR_STAGE is enabled
> + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set,
> rather
> + * than being allocated by malloc().
> */
> enum bootflow_flags_t {
> BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> + BOOTFLOWF_STATIC_BUF= 1 << 1,
> };
>
> /**
> @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> * @fname: Filename of bootflow file (allocated)
> * @logo: Logo to display for this bootflow (BMP format)
> * @logo_size: Size of the logo in bytes
> - * @buf: Bootflow file contents (allocated)
> + * @buf: Bootflow file contents (allocated unless @flags &
> BOOTFLOWF_STATIC_BUF)
> * @size: Size of bootflow file in bytes
> * @err: Error number received (0 if OK)
> * @os_name: Name of the OS / distro being booted, or NULL if not known
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Shantur, On Thu, 16 Nov 2023 at 09:45, Shantur Rathore wrote: > > Hi Simon, > > > > [please can you avoid top-posting as it makes it had for people to read > > later] > > > > Sure thing. Thanks! > > > There is a pending series there which I haven't got to yet. > > > I can add that change to my bootmeth_efi dhcp fixes patch series if you want. Yes please > > > BTW, for this patch we need a test which covers EFI on sandbox, > > perhaps running the helloworld program. The current bootstd testing > > does not extend into the efi_loader code, so there is really no > > coverage of the code in efi_exit_boot_services() from bootstd. > > I am not sure how sandbox works. > I was able to test my setup which was crashing and was fixed after > these changes. OK, good. In that case we normally send 'Tested-by: My Name https://docs.u-boot.org/en/latest/arch/sandbox/sandbox.html [2] https://docs.u-boot.org/en/latest/develop/testing.html [3] https://docs.u-boot.org/en/latest/develop/tests_sandbox.html
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
On Thu, Nov 16, 2023 at 1:35 AM Simon Glass wrote:
>
> EFI applications can be very large and thus used to cause boot failures
> when malloc() space was exhausted.
>
> A recent changed fixed this by using the kernel_addr_r environment var
> as the address of the buffer. However, it still frees the buffer when
> the bootflow is discarded.
>
> Fix this by introducing a flag to indicate whether the buffer was
> allocated, or not.
>
> Note that kernel_addr_r is not the last word here. It might be better
> to use lmb to place images. But there is a lot of refactoring to do
> before we can remove the environment variables. The distro scripts rely
> on them so it is safe for bootstd to do so too.
>
> Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
>
> Signed-off-by: Simon Glass
> Reported by: Simon Glass
> Reported by: Shantur Rathore
Tested by: Shantur Rathore
> ---
>
> boot/bootflow.c | 3 ++-
> boot/bootmeth_efi.c | 1 +
> include/bootflow.h | 5 -
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 6922e7e0c4e7..1ea2966ae9a5 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> free(bflow->name);
> free(bflow->subdir);
> free(bflow->fname);
> - free(bflow->buf);
> + if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> + free(bflow->buf);
> free(bflow->os_name);
> free(bflow->fdt_fname);
> free(bflow->bootmeth_priv);
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index ae936c8daa18..9ba7734911e1 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow,
> ulong addr)
> if (ret)
> return log_msg_ret("read", ret);
> bflow->buf = map_sysmem(addr, bflow->size);
> + bflow->flags |= BOOTFLOWF_STATIC_BUF;
>
> set_efi_bootdev(desc, bflow);
>
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 44d3741eacae..fede8f22a2b8 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -43,9 +43,12 @@ enum bootflow_state_t {
> * and it is using the prior-stage FDT, which is the U-Boot control FDT.
> * This is only possible with the EFI bootmeth (distro-efi) and only when
> * CONFIG_OF_HAS_PRIOR_STAGE is enabled
> + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set,
> rather
> + * than being allocated by malloc().
> */
> enum bootflow_flags_t {
> BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> + BOOTFLOWF_STATIC_BUF= 1 << 1,
> };
>
> /**
> @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> * @fname: Filename of bootflow file (allocated)
> * @logo: Logo to display for this bootflow (BMP format)
> * @logo_size: Size of the logo in bytes
> - * @buf: Bootflow file contents (allocated)
> + * @buf: Bootflow file contents (allocated unless @flags &
> BOOTFLOWF_STATIC_BUF)
> * @size: Size of bootflow file in bytes
> * @err: Error number received (0 if OK)
> * @os_name: Name of the OS / distro being booted, or NULL if not known
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Simon,
> [please can you avoid top-posting as it makes it had for people to read later]
>
Sure thing.
> There is a pending series there which I haven't got to yet.
>
I can add that change to my bootmeth_efi dhcp fixes patch series if you want.
> BTW, for this patch we need a test which covers EFI on sandbox,
> perhaps running the helloworld program. The current bootstd testing
> does not extend into the efi_loader code, so there is really no
> coverage of the code in efi_exit_boot_services() from bootstd.
I am not sure how sandbox works.
I was able to test my setup which was crashing and was fixed after
these changes.
Regard,
Shantur
>
> Regards,
> Simon
>
>
> >
> > Kind regards,
> > Shantur
> >
> > On Thu, Nov 16, 2023 at 1:35 AM Simon Glass wrote:
> > >
> > > EFI applications can be very large and thus used to cause boot failures
> > > when malloc() space was exhausted.
> > >
> > > A recent changed fixed this by using the kernel_addr_r environment var
> > > as the address of the buffer. However, it still frees the buffer when
> > > the bootflow is discarded.
> > >
> > > Fix this by introducing a flag to indicate whether the buffer was
> > > allocated, or not.
> > >
> > > Note that kernel_addr_r is not the last word here. It might be better
> > > to use lmb to place images. But there is a lot of refactoring to do
> > > before we can remove the environment variables. The distro scripts rely
> > > on them so it is safe for bootstd to do so too.
> > >
> > > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
> > >
> > > Signed-off-by: Simon Glass
> > > Reported by: Simon Glass
> > > Reported by: Shantur Rathore
> > > ---
> > >
> > > boot/bootflow.c | 3 ++-
> > > boot/bootmeth_efi.c | 1 +
> > > include/bootflow.h | 5 -
> > > 3 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > index 6922e7e0c4e7..1ea2966ae9a5 100644
> > > --- a/boot/bootflow.c
> > > +++ b/boot/bootflow.c
> > > @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> > > free(bflow->name);
> > > free(bflow->subdir);
> > > free(bflow->fname);
> > > - free(bflow->buf);
> > > + if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> > > + free(bflow->buf);
> > > free(bflow->os_name);
> > > free(bflow->fdt_fname);
> > > free(bflow->bootmeth_priv);
> > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> > > index ae936c8daa18..9ba7734911e1 100644
> > > --- a/boot/bootmeth_efi.c
> > > +++ b/boot/bootmeth_efi.c
> > > @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow,
> > > ulong addr)
> > > if (ret)
> > > return log_msg_ret("read", ret);
> > > bflow->buf = map_sysmem(addr, bflow->size);
> > > + bflow->flags |= BOOTFLOWF_STATIC_BUF;
> > >
> > > set_efi_bootdev(desc, bflow);
> > >
> > > diff --git a/include/bootflow.h b/include/bootflow.h
> > > index 44d3741eacae..fede8f22a2b8 100644
> > > --- a/include/bootflow.h
> > > +++ b/include/bootflow.h
> > > @@ -43,9 +43,12 @@ enum bootflow_state_t {
> > > * and it is using the prior-stage FDT, which is the U-Boot control
> > > FDT.
> > > * This is only possible with the EFI bootmeth (distro-efi) and only
> > > when
> > > * CONFIG_OF_HAS_PRIOR_STAGE is enabled
> > > + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set,
> > > rather
> > > + * than being allocated by malloc().
> > > */
> > > enum bootflow_flags_t {
> > > BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> > > + BOOTFLOWF_STATIC_BUF= 1 << 1,
> > > };
> > >
> > > /**
> > > @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> > > * @fname: Filename of bootflow file (allocated)
> > > * @logo: Logo to display for this bootflow (BMP format)
> > > * @logo_size: Size of the logo in bytes
> > > - * @buf: Bootflow file contents (allocated)
> > > + * @buf: Bootflow file contents (allocated unless @flags &
> > > BOOTFLOWF_STATIC_BUF)
> > > * @size: Size of bootflow file in bytes
> > > * @err: Error number received (0 if OK)
> > > * @os_name: Name of the OS / distro being booted, or NULL if not known
> > > --
> > > 2.43.0.rc0.421.g78406f8d94-goog
> > >
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Shantur,
On Thu, 16 Nov 2023 at 04:02, Shantur Rathore wrote:
>
> Hi Simon,
>
> Thanks for the patch.
> Ater this patch, booting off USB works fine over USB disk.
> Maybe you need the same flag for dhcp as well just after dhcp_run() here
> https://github.com/u-boot/u-boot/blob/master/boot/bootmeth_efi.c#L356
[please can you avoid top-posting as it makes it had for people to read later]
There is a pending series there which I haven't got to yet.
BTW, for this patch we need a test which covers EFI on sandbox,
perhaps running the helloworld program. The current bootstd testing
does not extend into the efi_loader code, so there is really no
coverage of the code in efi_exit_boot_services() from bootstd.
Regards,
Simon
>
> Kind regards,
> Shantur
>
> On Thu, Nov 16, 2023 at 1:35 AM Simon Glass wrote:
> >
> > EFI applications can be very large and thus used to cause boot failures
> > when malloc() space was exhausted.
> >
> > A recent changed fixed this by using the kernel_addr_r environment var
> > as the address of the buffer. However, it still frees the buffer when
> > the bootflow is discarded.
> >
> > Fix this by introducing a flag to indicate whether the buffer was
> > allocated, or not.
> >
> > Note that kernel_addr_r is not the last word here. It might be better
> > to use lmb to place images. But there is a lot of refactoring to do
> > before we can remove the environment variables. The distro scripts rely
> > on them so it is safe for bootstd to do so too.
> >
> > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
> >
> > Signed-off-by: Simon Glass
> > Reported by: Simon Glass
> > Reported by: Shantur Rathore
> > ---
> >
> > boot/bootflow.c | 3 ++-
> > boot/bootmeth_efi.c | 1 +
> > include/bootflow.h | 5 -
> > 3 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > index 6922e7e0c4e7..1ea2966ae9a5 100644
> > --- a/boot/bootflow.c
> > +++ b/boot/bootflow.c
> > @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> > free(bflow->name);
> > free(bflow->subdir);
> > free(bflow->fname);
> > - free(bflow->buf);
> > + if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> > + free(bflow->buf);
> > free(bflow->os_name);
> > free(bflow->fdt_fname);
> > free(bflow->bootmeth_priv);
> > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> > index ae936c8daa18..9ba7734911e1 100644
> > --- a/boot/bootmeth_efi.c
> > +++ b/boot/bootmeth_efi.c
> > @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow,
> > ulong addr)
> > if (ret)
> > return log_msg_ret("read", ret);
> > bflow->buf = map_sysmem(addr, bflow->size);
> > + bflow->flags |= BOOTFLOWF_STATIC_BUF;
> >
> > set_efi_bootdev(desc, bflow);
> >
> > diff --git a/include/bootflow.h b/include/bootflow.h
> > index 44d3741eacae..fede8f22a2b8 100644
> > --- a/include/bootflow.h
> > +++ b/include/bootflow.h
> > @@ -43,9 +43,12 @@ enum bootflow_state_t {
> > * and it is using the prior-stage FDT, which is the U-Boot control
> > FDT.
> > * This is only possible with the EFI bootmeth (distro-efi) and only
> > when
> > * CONFIG_OF_HAS_PRIOR_STAGE is enabled
> > + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set,
> > rather
> > + * than being allocated by malloc().
> > */
> > enum bootflow_flags_t {
> > BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> > + BOOTFLOWF_STATIC_BUF= 1 << 1,
> > };
> >
> > /**
> > @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> > * @fname: Filename of bootflow file (allocated)
> > * @logo: Logo to display for this bootflow (BMP format)
> > * @logo_size: Size of the logo in bytes
> > - * @buf: Bootflow file contents (allocated)
> > + * @buf: Bootflow file contents (allocated unless @flags &
> > BOOTFLOWF_STATIC_BUF)
> > * @size: Size of bootflow file in bytes
> > * @err: Error number received (0 if OK)
> > * @os_name: Name of the OS / distro being booted, or NULL if not known
> > --
> > 2.43.0.rc0.421.g78406f8d94-goog
> >
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Simon,
Thanks for the patch.
Ater this patch, booting off USB works fine over USB disk.
Maybe you need the same flag for dhcp as well just after dhcp_run() here
https://github.com/u-boot/u-boot/blob/master/boot/bootmeth_efi.c#L356
Kind regards,
Shantur
On Thu, Nov 16, 2023 at 1:35 AM Simon Glass wrote:
>
> EFI applications can be very large and thus used to cause boot failures
> when malloc() space was exhausted.
>
> A recent changed fixed this by using the kernel_addr_r environment var
> as the address of the buffer. However, it still frees the buffer when
> the bootflow is discarded.
>
> Fix this by introducing a flag to indicate whether the buffer was
> allocated, or not.
>
> Note that kernel_addr_r is not the last word here. It might be better
> to use lmb to place images. But there is a lot of refactoring to do
> before we can remove the environment variables. The distro scripts rely
> on them so it is safe for bootstd to do so too.
>
> Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
>
> Signed-off-by: Simon Glass
> Reported by: Simon Glass
> Reported by: Shantur Rathore
> ---
>
> boot/bootflow.c | 3 ++-
> boot/bootmeth_efi.c | 1 +
> include/bootflow.h | 5 -
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 6922e7e0c4e7..1ea2966ae9a5 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> free(bflow->name);
> free(bflow->subdir);
> free(bflow->fname);
> - free(bflow->buf);
> + if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> + free(bflow->buf);
> free(bflow->os_name);
> free(bflow->fdt_fname);
> free(bflow->bootmeth_priv);
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index ae936c8daa18..9ba7734911e1 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow,
> ulong addr)
> if (ret)
> return log_msg_ret("read", ret);
> bflow->buf = map_sysmem(addr, bflow->size);
> + bflow->flags |= BOOTFLOWF_STATIC_BUF;
>
> set_efi_bootdev(desc, bflow);
>
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 44d3741eacae..fede8f22a2b8 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -43,9 +43,12 @@ enum bootflow_state_t {
> * and it is using the prior-stage FDT, which is the U-Boot control FDT.
> * This is only possible with the EFI bootmeth (distro-efi) and only when
> * CONFIG_OF_HAS_PRIOR_STAGE is enabled
> + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set,
> rather
> + * than being allocated by malloc().
> */
> enum bootflow_flags_t {
> BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> + BOOTFLOWF_STATIC_BUF= 1 << 1,
> };
>
> /**
> @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> * @fname: Filename of bootflow file (allocated)
> * @logo: Logo to display for this bootflow (BMP format)
> * @logo_size: Size of the logo in bytes
> - * @buf: Bootflow file contents (allocated)
> + * @buf: Bootflow file contents (allocated unless @flags &
> BOOTFLOWF_STATIC_BUF)
> * @size: Size of bootflow file in bytes
> * @err: Error number received (0 if OK)
> * @os_name: Name of the OS / distro being booted, or NULL if not known
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Heinrich, On Wed, 15 Nov 2023 at 19:18, Heinrich Schuchardt wrote: > > On 11/16/23 02:35, Simon Glass wrote: > > EFI applications can be very large and thus used to cause boot failures > > when malloc() space was exhausted. > > > > A recent changed fixed this by using the kernel_addr_r environment var > > as the address of the buffer. However, it still frees the buffer when > > the bootflow is discarded. > > > > Fix this by introducing a flag to indicate whether the buffer was > > allocated, or not. > > > > Note that kernel_addr_r is not the last word here. It might be better > > to use lmb to place images. But there is a lot of refactoring to do > > We need a discussion about memory management. We currently have: > > * malloc > * EFI - AllocatePages(), AllocatePool() > * lmb > * preassigned addresses like $kernel_addr_r, $fdt_addr_r Yes indeed. There have been discussions already, but we need to do this on the ML. > > EFI manages memory on page level. It needs to provide a complete memory > map to the OS with flags for different types of memory. Currently it is > not aware of where files are loaded and might allocate those regions for > its own usage. > > lmb does not track any allocations but tries to recreate a memory map on > the fly whenever a file is loaded. > > It would be preferable to allocate memory for files and require to > explicitly unload a file before reusing the same memory area. > > We should start a separate thread on this topic. Indeed. If you start it, I will reply with some thoughts. The nice thing is that with bootstd we can start to tidy all this up and make sure it has tests. > > > before we can remove the environment variables. The distro scripts rely > > on them so it is safe for bootstd to do so too. > > > > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file > > > > Signed-off-by: Simon Glass > > Reported by: Simon Glass > > Reported by: Shantur Rathore > > Reviewed-by: Heinrich Schuchardt > [..] Regards, Simon
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
On 11/16/23 02:35, Simon Glass wrote:
EFI applications can be very large and thus used to cause boot failures
when malloc() space was exhausted.
A recent changed fixed this by using the kernel_addr_r environment var
as the address of the buffer. However, it still frees the buffer when
the bootflow is discarded.
Fix this by introducing a flag to indicate whether the buffer was
allocated, or not.
Note that kernel_addr_r is not the last word here. It might be better
to use lmb to place images. But there is a lot of refactoring to do
We need a discussion about memory management. We currently have:
* malloc
* EFI - AllocatePages(), AllocatePool()
* lmb
* preassigned addresses like $kernel_addr_r, $fdt_addr_r
EFI manages memory on page level. It needs to provide a complete memory
map to the OS with flags for different types of memory. Currently it is
not aware of where files are loaded and might allocate those regions for
its own usage.
lmb does not track any allocations but tries to recreate a memory map on
the fly whenever a file is loaded.
It would be preferable to allocate memory for files and require to
explicitly unload a file before reusing the same memory area.
We should start a separate thread on this topic.
before we can remove the environment variables. The distro scripts rely
on them so it is safe for bootstd to do so too.
Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
Signed-off-by: Simon Glass
Reported by: Simon Glass
Reported by: Shantur Rathore
Reviewed-by: Heinrich Schuchardt
---
boot/bootflow.c | 3 ++-
boot/bootmeth_efi.c | 1 +
include/bootflow.h | 5 -
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/boot/bootflow.c b/boot/bootflow.c
index 6922e7e0c4e7..1ea2966ae9a5 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
free(bflow->name);
free(bflow->subdir);
free(bflow->fname);
- free(bflow->buf);
+ if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
+ free(bflow->buf);
free(bflow->os_name);
free(bflow->fdt_fname);
free(bflow->bootmeth_priv);
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index ae936c8daa18..9ba7734911e1 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow, ulong
addr)
if (ret)
return log_msg_ret("read", ret);
bflow->buf = map_sysmem(addr, bflow->size);
+ bflow->flags |= BOOTFLOWF_STATIC_BUF;
set_efi_bootdev(desc, bflow);
diff --git a/include/bootflow.h b/include/bootflow.h
index 44d3741eacae..fede8f22a2b8 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -43,9 +43,12 @@ enum bootflow_state_t {
*and it is using the prior-stage FDT, which is the U-Boot control FDT.
*This is only possible with the EFI bootmeth (distro-efi) and only when
*CONFIG_OF_HAS_PRIOR_STAGE is enabled
+ * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
+ * than being allocated by malloc().
*/
enum bootflow_flags_t {
BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
+ BOOTFLOWF_STATIC_BUF= 1 << 1,
};
/**
@@ -72,7 +75,7 @@ enum bootflow_flags_t {
* @fname: Filename of bootflow file (allocated)
* @logo: Logo to display for this bootflow (BMP format)
* @logo_size: Size of the logo in bytes
- * @buf: Bootflow file contents (allocated)
+ * @buf: Bootflow file contents (allocated unless @flags &
BOOTFLOWF_STATIC_BUF)
* @size: Size of bootflow file in bytes
* @err: Error number received (0 if OK)
* @os_name: Name of the OS / distro being booted, or NULL if not known
[PATCH] bootstd: Avoid freeing a non-allocated buffer
EFI applications can be very large and thus used to cause boot failures
when malloc() space was exhausted.
A recent changed fixed this by using the kernel_addr_r environment var
as the address of the buffer. However, it still frees the buffer when
the bootflow is discarded.
Fix this by introducing a flag to indicate whether the buffer was
allocated, or not.
Note that kernel_addr_r is not the last word here. It might be better
to use lmb to place images. But there is a lot of refactoring to do
before we can remove the environment variables. The distro scripts rely
on them so it is safe for bootstd to do so too.
Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
Signed-off-by: Simon Glass
Reported by: Simon Glass
Reported by: Shantur Rathore
---
boot/bootflow.c | 3 ++-
boot/bootmeth_efi.c | 1 +
include/bootflow.h | 5 -
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/boot/bootflow.c b/boot/bootflow.c
index 6922e7e0c4e7..1ea2966ae9a5 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
free(bflow->name);
free(bflow->subdir);
free(bflow->fname);
- free(bflow->buf);
+ if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
+ free(bflow->buf);
free(bflow->os_name);
free(bflow->fdt_fname);
free(bflow->bootmeth_priv);
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index ae936c8daa18..9ba7734911e1 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow, ulong
addr)
if (ret)
return log_msg_ret("read", ret);
bflow->buf = map_sysmem(addr, bflow->size);
+ bflow->flags |= BOOTFLOWF_STATIC_BUF;
set_efi_bootdev(desc, bflow);
diff --git a/include/bootflow.h b/include/bootflow.h
index 44d3741eacae..fede8f22a2b8 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -43,9 +43,12 @@ enum bootflow_state_t {
* and it is using the prior-stage FDT, which is the U-Boot control FDT.
* This is only possible with the EFI bootmeth (distro-efi) and only when
* CONFIG_OF_HAS_PRIOR_STAGE is enabled
+ * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
+ * than being allocated by malloc().
*/
enum bootflow_flags_t {
BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
+ BOOTFLOWF_STATIC_BUF= 1 << 1,
};
/**
@@ -72,7 +75,7 @@ enum bootflow_flags_t {
* @fname: Filename of bootflow file (allocated)
* @logo: Logo to display for this bootflow (BMP format)
* @logo_size: Size of the logo in bytes
- * @buf: Bootflow file contents (allocated)
+ * @buf: Bootflow file contents (allocated unless @flags &
BOOTFLOWF_STATIC_BUF)
* @size: Size of bootflow file in bytes
* @err: Error number received (0 if OK)
* @os_name: Name of the OS / distro being booted, or NULL if not known
--
2.43.0.rc0.421.g78406f8d94-goog
