Re: [RFC PATCH v2 08/24] unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] If mas_erase() fails due to an Out-Of-Memory (OOM) condition, it leaves the entry in the tree but __eh_frame_remove_section() still frees it via call_srcu().
- [Medium] Mixing `goto`-based error handling with scope-based cleanup helpers in the same function.
- [Low] `efi_mm` is missing the `INIT_MM_EH_FRAME` initializer.
--

--- Patch [8]: [RFC PATCH v2 08/24] unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree ---
commit 428d78732477ec23211b2b4732a9923dae762c55
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Store .eh_frame_hdr section data in per-mm maple tree

This commit associates .eh_frame_hdr sections with their mm_struct by
adding them to a per-mm maple tree. The tree is indexed by the
corresponding text address ranges.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> index f25a0923e315b..8cf19c5298fde 100644
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c

[ ... ]

> @@ -352,15 +355,76 @@ int eh_frame_add_section(unsigned long eh_frame_hdr_start,
>  	if (ret)
>  		goto err_free;
>  
> -	/* TODO nowhere to store it yet - just free it and return an error */
> -	ret = -ENOSYS;
> +	ret = mtree_insert_range(eh_frame_mt, sec->text_start, sec->text_end - 1,
> +				 sec, GFP_KERNEL_ACCOUNT);
> +	if (ret) {
> +		dbg("mtree_insert_range failed: text=%lx-%lx\n",
> +		    sec->text_start, sec->text_end);
> +		goto err_free;
> +	}
> +
> +	return 0;
>  
>  err_free:

[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
helpers in the same function?

The eh_frame_add_section function uses scoped_guard for the mmap lock but
retains a goto err_free label for allocation cleanup on failure.

The cleanup subsystem guidelines suggest that a function should either convert
all resources to scope-based cleanup or convert none of them to avoid
confusing ownership semantics.

>  	free_section(sec);
>  	return ret;
>  }

[ ... ]

> +static int __eh_frame_remove_section(struct ma_state *mas,
> +				     struct eh_frame_section *sec)
> +{
> +	if (mas_erase(mas) != sec) {
> +		dbg("mas_erase failed: text=%lx\n", sec->text_start);
> +		return -EINVAL;
> +	}
> +
> +	call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu);

[Severity: Critical]
Can this cause a use-after-free if the maple tree erase operation fails?

If mas_erase fails due to memory exhaustion (for example, during tree
rebalancing), it leaves the entry in the tree, sets mas_is_err(mas), and
returns the pointer to the entry.

Because it returns the entry pointer, the condition mas_erase(mas) != sec
will evaluate to false, skipping the error block.

The code would then proceed to call call_srcu and free the section while
it remains linked in the maple tree, potentially causing a crash when the
tree is traversed later. Should this check mas_is_err(mas) instead?

> +
> +	return 0;
> +}

[ ... ]

> diff --git a/mm/init-mm.c b/mm/init-mm.c
> index 3e792aad76261..25c26341fc4fb 100644
> --- a/mm/init-mm.c
> +++ b/mm/init-mm.c

[ ... ]

> @@ -48,6 +49,7 @@ struct mm_struct init_mm = {
>  #endif
>  	.flexible_array	= MM_STRUCT_FLEXIBLE_ARRAY_INIT,
>  	INIT_MM_CONTEXT(init_mm)
> +	INIT_MM_EH_FRAME
>  };

[Severity: Low]
Does efi_mm in drivers/firmware/efi/efi.c also need this initializer?

When a new initializer like INIT_MM_EH_FRAME is added to init_mm, it usually
needs to be added to all static mm_struct definitions.

Leaving it out means the eh_frame_mt maple tree inside efi_mm remains
zero-initialized rather than properly initialized with MTREE_INIT, which
might lead to lock splats or crashes if accessed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8