Re: [PATCH v2 1/1] mm/execmem: Make the populate and alloc atomic

Mike Rapoport
2026-03-18T14:42:01Z

Hi Hubert,

On Tue, Mar 17, 2026 at 12:50:20PM +0000, Hubert Mazur wrote:
> Subject: mm/execmem: Make the populate and alloc atomic
>
> When a memory block is requested from the execmem manager, it tries
> to find a suitable fragment in the free_areas. In case there is no
> such block, a new memory area is added to free_areas and then
> allocated to the caller. Those two operations must be atomic
> to ensure that no other memory request consumes it.
>
> Signed-off-by: Hubert Mazur <hmazur@google.com>
> ---
> mm/execmem.c | 61 +++++++++++++++++++++++++++++-----------------------
> 1 file changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/mm/execmem.c b/mm/execmem.c
> index 810a4ba9c924..9043f0f8f61a 100644
> --- a/mm/execmem.c
> +++ b/mm/execmem.c
> @@ -203,13 +203,6 @@ static int execmem_cache_add_locked(void *ptr, size_t size, gfp_t gfp_mask)
> return mas_store_gfp(&mas, (void *)lower, gfp_mask);
> }
>
> -static int execmem_cache_add(void *ptr, size_t size, gfp_t gfp_mask)
> -{
> - guard(mutex)(&execmem_cache.mutex);
> -
> - return execmem_cache_add_locked(ptr, size, gfp_mask);
> -}
> -
> static bool within_range(struct execmem_range *range, struct ma_state *mas,
> size_t size)
> {
> @@ -225,18 +218,16 @@ static bool within_range(struct execmem_range *range, struct ma_state *mas,
> return false;
> }
>
> -static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> +static void *__execmem_cache_alloc_locked(struct execmem_range *range, size_t size)

No need to keep prefix underscores here.

> {
> struct maple_tree *free_areas = &execmem_cache.free_areas;
> struct maple_tree *busy_areas = &execmem_cache.busy_areas;
> MA_STATE(mas_free, free_areas, 0, ULONG_MAX);
> MA_STATE(mas_busy, busy_areas, 0, ULONG_MAX);
> - struct mutex *mutex = &execmem_cache.mutex;
> unsigned long addr, last, area_size = 0;
> void *area, *ptr = NULL;
> int err;
>
> - mutex_lock(mutex);
> mas_for_each(&mas_free, area, ULONG_MAX) {
> area_size = mas_range_len(&mas_free);
>
> @@ -245,7 +236,7 @@ static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> }
>
> if (area_size < size)
> - goto out_unlock;
> + return NULL;
>
> addr = mas_free.index;
> last = mas_free.last;
> @@ -254,7 +245,7 @@ static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> mas_set_range(&mas_busy, addr, addr + size - 1);
> err = mas_store_gfp(&mas_busy, (void *)addr, GFP_KERNEL);
> if (err)
> - goto out_unlock;
> + return NULL;
>
> mas_store_gfp(&mas_free, NULL, GFP_KERNEL);
> if (area_size > size) {
> @@ -268,19 +259,25 @@ static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> err = mas_store_gfp(&mas_free, ptr, GFP_KERNEL);
> if (err) {
> mas_store_gfp(&mas_busy, NULL, GFP_KERNEL);
> - goto out_unlock;
> + return NULL;
> }
> }
> ptr = (void *)addr;
>
> -out_unlock:
> - mutex_unlock(mutex);
> return ptr;
> }
>
> -static int execmem_cache_populate(struct execmem_range *range, size_t size)
> +static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> +{
> + guard(mutex)(&execmem_cache.mutex);
> +
> + return __execmem_cache_alloc_locked(range, size);
> +}
> +
> +static void *__execmem_cache_populate_alloc(struct execmem_range *range, size_t size)

No need for leading underscores here as well.

> {
> unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
> + struct mutex *mutex = &execmem_cache.mutex;
> struct vm_struct *vm;
> size_t alloc_size;
> int err = -ENOMEM;
> @@ -294,7 +291,7 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
> }
>
> if (!p)
> - return err;
> + return NULL;
>
> vm = find_vm_area(p);
> if (!vm)
> @@ -307,33 +304,43 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
> if (err)
> goto err_free_mem;
>
> - err = execmem_cache_add(p, alloc_size, GFP_KERNEL);
> - if (err)
> + /*
> + * New memory blocks must be propagated and allocated as an atomic operation,
> + * otherwise it may be consumed by a parallel call to the execmem_cache_alloc
> + * function.
> + */

Please keep the comment lines under 80 characters.

> + mutex_lock(mutex);
> + err = execmem_cache_add_locked(p, alloc_size, GFP_KERNEL);
> + if (err) {
> + mutex_unlock(mutex);
> goto err_reset_direct_map;

Please add a new label that will unlock the mutex and goto to the new
label.

> + }
>
> - return 0;
> + p = __execmem_cache_alloc_locked(range, size);
> + if (!p) {
> + mutex_unlock(mutex);
> + return NULL;

This if is not needed, we anyway unlock the mutex and return p, if
__execmem_cache_alloc_locked() failed p would be NULL anyway.

> + }
> + mutex_unlock(mutex);
> +
> + return p;
>
> err_reset_direct_map:
> execmem_set_direct_map_valid(vm, true);
> err_free_mem:
> vfree(p);
> - return err;
> + return NULL;
> }
>
> static void *execmem_cache_alloc(struct execmem_range *range, size_t size)
> {
> void *p;
> - int err;
>
> p = __execmem_cache_alloc(range, size);
> if (p)
> return p;
>
> - err = execmem_cache_populate(range, size);
> - if (err)
> - return NULL;
> -
> - return __execmem_cache_alloc(range, size);
> + return __execmem_cache_populate_alloc(range, size);
> }
>
> static inline bool is_pending_free(void *ptr)
> --
> 2.53.0.851.ga537e3e6e9-goog
>

--
Sincerely yours,
Mike.