Re: [PATCH 5/9] bpf/syscall: Route generic_map_update_batch key/value allocations to folio_scratchpad

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Batch map updates fail with -ENOMEM for maps with keys or values larger than ~4072 bytes due to order=0 chunk limits.
- [High] Missing folio_scratchpad_free on the key allocation error path causes a permanent kernel memory leak when map->key_size exceeds the chunk capacity.
- [Low] Incompatible pointer type passed to folio_scratchpad_init_key causes a compiler warning.
--

--- Patch [5]: [PATCH 5/9] bpf/syscall: Route generic_map_update_batch key/value allocations to folio_scratchpad ---
commit d64109fbe5280b8aca05d025f4ea39aceb7430e9
Author: Jim Cromie <[email protected]>

bpf/syscall: Route generic_map_update_batch key/value allocations to folio_scratchpad

Use struct folio_scratchpad to allocate transient key and value buffers
during map update batch operations, bypassing SLUB cache churn.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6db306d23b479..8ef48673b9339 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
[ ... ]
> @@ -2083,13 +2087,17 @@ int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
>  	if (put_user(0, &uattr->batch.count))
>  		return -EFAULT;
>  
> -	key = kvmalloc(map->key_size, GFP_USER | __GFP_NOWARN);
> +	folio_scratchpad_init_key(&batch_sp, 0, &bpf_batch_scratchpad_key);

[Severity: Critical]
Does this restrict the size of map elements, causing failures for large
elements?

By passing 0 for the chunk order in folio_scratchpad_init_key(), the
allocation limit is now restricted to a single page. Before this change, the
code used kvmalloc() which supported much larger sizes.

Since folio_scratchpad_alloc() returns NULL if the requested size exceeds the
remaining space in a single page:

lib/folio_pool.c:folio_scratchpad_alloc() {
    ...
    if (sp->remaining < aligned_size) {
        spin_unlock_irqrestore(&sp->lock, flags);
        return NULL;
    }
}

Will this cause legitimate bpf batch map updates with large keys or values to
unconditionally fail with -ENOMEM?

[Severity: Low]
Are we passing an incompatible pointer type here?

The variable bpf_batch_scratchpad_key is defined as struct static_key_true
via DEFINE_STATIC_KEY_TRUE, but folio_scratchpad_init_key() expects a
struct static_key *. Passing it directly might trigger strict aliasing
warnings and break builds.

> +
> +	key = folio_scratchpad_alloc(&batch_sp, map->key_size,
> +				     sizeof(void *), GFP_USER | __GFP_NOWARN);
>  	if (!key)
>  		return -ENOMEM;

[Severity: High]
Could this lead to a memory leak if the key allocation fails?

If map->key_size exceeds the capacity of the order-0 chunk,
folio_scratchpad_alloc() allocates a new folio and adds it to the chunks
list, but still returns NULL because the chunk is too small:

lib/folio_pool.c:folio_scratchpad_alloc() {
    ...
    list_add(&chunk->link, &sp->chunks);
    ...
    if (sp->remaining < aligned_size) {
        spin_unlock_irqrestore(&sp->lock, flags);
        return NULL;
    }
}

Returning -ENOMEM here in generic_map_update_batch() without calling
folio_scratchpad_free() leaves the newly allocated folio on the
sp->chunks list, leaking the memory.

>  
> -	value = kvmalloc(value_size, GFP_USER | __GFP_NOWARN);
> +	value = folio_scratchpad_alloc(&batch_sp, value_size,
> +				       sizeof(void *), GFP_USER | __GFP_NOWARN);
>  	if (!value) {
> -		kvfree(key);
> +		folio_scratchpad_free(&batch_sp);
>  		return -ENOMEM;
>  	}

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