[Skiboot] [PATCH] core/mem_region: Give up on freeing unmanaged memory block

[Oliver O'Halloran]

On Fri, May 26, 2017 at 3:21 PM, Gavin Shan <gwshan at linux.vnet.ibm.com> wrote:
> When adding memory regions to global list, the overlapped one
> (including it's descriptor) is purged. However, the descriptor might
> be in BSS (not HEAP) section. It will trigger the exception caused
> by assert() in mem_free() as below backtrace indicates. The memory
> regions causing this are "ibm,os-reserve" and "hostboot-reserve-0"
> separately. The former descriptor is resident in BSS section while
> the later one is in HEAP:
>
> CPU 0050 Backtrace:
>  S: 0000000031d43a00 R: 00000000300136d0   .backtrace+0x2c
>  S: 0000000031d43a90 R: 00000000300191ec   ._abort+0x4c
>  S: 0000000031d43b10 R: 0000000030019268   .assert_fail+0x34
>  S: 0000000031d43b90 R: 0000000030015d08   .mem_free+0x64
>  S: 0000000031d43c20 R: 0000000030017080   .__free+0x38
>  S: 0000000031d43cb0 R: 0000000030015518   .add_region+0x230
>  S: 0000000031d43d60 R: 0000000030016854   .mem_region_init+0x2c8
>  S: 0000000031d43e30 R: 0000000030014994   .main_cpu_entry+0x3ec
>  S: 0000000031d43f00 R: 0000000030002648   boot_entry+0x198
>
> This fixes the issue by doing nothing when trying to freeing memory
> block which is resident in unmanaged (not HEAP) region.
>
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>

Not a fan. The specific bug you're seeing here is because of hostboot
being broken on FSP machines and reserving memory that it shouldn't.
This patch papers over the problem by making any invalid free()
silently fail which is something we should avoid.

> ---
>  core/mem_region.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/core/mem_region.c b/core/mem_region.c
> index a314558..bfb5ac9 100644
> --- a/core/mem_region.c
> +++ b/core/mem_region.c
> @@ -469,9 +469,10 @@ void mem_free(struct mem_region *region, void *mem, const char *location)
>         if (!mem)
>                 return;
>
> -       /* Your memory is in the region, right? */
> -       assert(mem >= region_start(region) + sizeof(*hdr));
> -       assert(mem < region_start(region) + region->len);
> +       /* Give up if it is in unmanaged and invalid regions */
> +       if (mem < (region_start(region) + sizeof(*hdr)) ||
> +           mem >= (region_start(region) + region->len))
> +               return;
>
>         /* Grab header. */
>         hdr = mem - sizeof(*hdr);
> --
> 2.7.4
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot