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
