Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [Critical] Passing ZERO_SIZE_PTR to is_folio_pool_ptr() triggers a kernel Oops. - [Critical] Integer overflow in aligned_size calculation bypasses bounds checks and leads to out-of-bounds memset(). - [High] is_folio_pool_ptr() incorrectly identifies large kmalloc() allocations as pool pointers, causing memory leaks. - [High] folio_pool_realloc() allocates unmanaged kmalloc objects that escape O(1) bulk teardown, leaking memory. - [High] DEFINE_FREE wrapper for folio_scratchpad fails to guard against ERR_PTR, risking kernel panics. - [High] Fallback kvzalloc() allocations are not tracked in the scratchpad, breaking bulk teardown and leaking memory. - [High] Integer underflow when the requested alignment exceeds the fallback folio size causes an out-of-bounds memory write. - [High] Calling krealloc() on vmalloc pointers triggers a kernel panic. - [Medium] folio_scratchpad_reset() retains the newest (potentially smallest) chunk instead of the primary large chunk. - [Medium] Requesting an allocation larger than a folio's capacity permanently leaks a new folio on every attempt. - [Medium] TOCTOU race condition in folio_scratchpad_alloc() blindly overwrites pool state, abandoning free space. - [Low] folio_scratchpad_stats() calculates incorrect telemetry metrics when the allocator falls back to order-0 folios. --
--- Patch [1]: [PATCH 1/9] lib/folio_pool: Introduce Direct-Map Large Folio Pool & Scratchpad bump allocators --- commit affebf70d4f5ab8fbcb579b4c9e3058f34859616 Author: Jim Cromie <[email protected]> lib/folio_pool: Introduce Direct-Map Large Folio Pool & Scratchpad bump allocators This commit introduces region-based linear bump allocators backed by direct-map large folios for kernel subsystems. It provides a core engine (folio_scratchpad) and a wrapper (folio_pool), with static key controls for subsystem autonomy. > diff --git a/include/linux/folio_pool.h b/include/linux/folio_pool.h > new file mode 100644 > index 0000000000000..5bbca8818877c > --- /dev/null > +++ b/include/linux/folio_pool.h [ ... ] > +void folio_scratchpad_stats(struct folio_scratchpad *sp, unsigned int > *nr_chunks, > + size_t *chunk_size, size_t *tail_used); > + > +DEFINE_FREE(folio_scratchpad, struct folio_scratchpad *, if (_T) > folio_scratchpad_free(_T)) [Severity: High] Does this wrapper properly handle ERR_PTR returns? If an allocator returns an ERR_PTR, it evaluates as truthy in the if (_T) check. This would pass the bogus pointer to folio_scratchpad_free(), which might dereference it while acquiring the spinlock and cause a kernel crash. > + > +/** > + * is_folio_pool_ptr - Check whether an address resides in a folio > pool/scratchpad > + * @ptr: Object pointer to test > + * > + * Direct-map large folios allocated via folio_alloc() are not slab pages, > + * unlike objects returned by kmalloc/kzalloc. > + */ > +static inline bool is_folio_pool_ptr(const void *ptr) > +{ > + return ptr && !is_vmalloc_addr(ptr) && > !folio_test_slab(virt_to_folio(ptr)); [Severity: Critical] Can this panic if ptr is ZERO_SIZE_PTR? If a caller uses kvzalloc(0), it returns a ZERO_SIZE_PTR (typically (void *)16). This is truthy and not a vmalloc address, so it seems virt_to_folio((void *)16) would be called, which could index out of bounds in vmemmap and trigger an unhandled page fault. [Severity: High] Will this misidentify large kmalloc() allocations? Since kmalloc_large() allocations bypass the SLUB allocator and are served directly by the page allocator, they do not have the PG_slab flag set. It looks like they would be misidentified as folio pool pointers, causing kvfree() to be skipped in folio_pool_free_elem() and leading to a memory leak. > +} > + > +/** > + * folio_pool_free_elem - Safely release a pool object or SLUB fallback > element [ ... ] > + * in a direct-map large folio, allocates a fresh @new_size buffer from SLUB > + * and copies @old_size bytes; the original scratchpad slot remains abandoned > + * until the entire scratchpad is released or reset at batch boundary. > + */ > +static inline void *folio_pool_realloc(void *ptr, size_t old_size, > + size_t new_size, gfp_t gfp) > +{ > + void *new_ptr; > + > + if (!ptr) > + return kmalloc(new_size, gfp); > + > + if (!is_folio_pool_ptr(ptr)) > + return krealloc(ptr, new_size, gfp); [Severity: High] Can this trigger a kernel panic by passing a vmalloc pointer to krealloc()? If the fallback allocation was performed by kvzalloc(), it may return a vmalloc address. If passed here, is_folio_pool_ptr() would return false, and krealloc() would be called on a vmalloc pointer, which could cause a panic. > + > + new_ptr = kmalloc(new_size, gfp); [Severity: High] Does this allocate unmanaged kmalloc objects that escape bulk teardown? The newly allocated buffer does not appear to be added to the scratchpad's chunks list. Since it is invisible to folio_scratchpad_free(), subsystems using bulk teardown might permanently leak these reallocated SLUB objects. > + if (new_ptr) > + memcpy(new_ptr, ptr, min(old_size, new_size)); > + return new_ptr; > +} [ ... ] > diff --git a/lib/folio_pool.c b/lib/folio_pool.c > new file mode 100644 > index 0000000000000..07a290db6c506 > --- /dev/null > +++ b/lib/folio_pool.c [ ... ] > +noinline void *folio_scratchpad_alloc(struct folio_scratchpad *sp, size_t > size, > + size_t align, gfp_t gfp) > +{ > + struct folio_pool_chunk *chunk; > + struct folio *folio; > + void *elem, *base; > + size_t chunk_size, aligned_size, pad, header_offset; > + unsigned long flags; > + > + if (!folio_scratchpad_is_enabled(sp)) > + return kvzalloc(size, gfp); [Severity: High] Are these fallback allocations tracked for bulk teardown? It looks like they are never appended to sp->chunks. When folio_scratchpad_free() or folio_scratchpad_reset() executes, it seems they only iterate over sp->chunks, meaning all fallback allocations would be permanently leaked. > + > + if (unlikely(!size)) > + return NULL; > + > + align = max_t(size_t, sizeof(void *), align ? align : sizeof(void *)); > + > + spin_lock_irqsave(&sp->lock, flags); > + pad = (uintptr_t)sp->free_ptr & (align - 1); > + if (pad) > + pad = align - pad; > + aligned_size = size + pad; [Severity: Critical] Is it possible for aligned_size to overflow? If size is very large (near SIZE_MAX), aligned_size could wrap around to a small integer. The capacity check below might then incorrectly succeed, leading to an out-of-bounds memset(elem, 0, size) with the original massive size. > + > + if (sp->remaining < aligned_size) { > + spin_unlock_irqrestore(&sp->lock, flags); > + > + folio = folio_alloc(gfp, sp->chunk_order); > + if (!folio && sp->chunk_order > 0) > + folio = folio_alloc(gfp, 0); > + if (!folio) > + return NULL; > + > + base = folio_address(folio); > + chunk = (struct folio_pool_chunk *)base; > + chunk->folio = folio; > + chunk_size = folio_size(folio); > + header_offset = ALIGN(sizeof(*chunk), max_t(size_t, sizeof(void > *), align)); > + > + spin_lock_irqsave(&sp->lock, flags); > + list_add(&chunk->link, &sp->chunks); > + sp->free_ptr = base + header_offset; > + sp->remaining = chunk_size - header_offset; [Severity: Medium] Could this overwrite pool state unconditionally in a race condition? Since sp->lock was dropped to call folio_alloc(), concurrent threads could also allocate folios. Upon re-acquiring the lock, it looks like threads blindly overwrite sp->free_ptr and sp->remaining without checking if the pool was already replenished, potentially abandoning free space. [Severity: High] Can chunk_size - header_offset underflow? If the user requests an extremely large alignment (e.g., page-block alignment) on a small fallback order-0 folio, header_offset might exceed chunk_size. This subtraction would underflow, producing a massive positive integer, and returning an out-of-bounds pointer. > + > + pad = (uintptr_t)sp->free_ptr & (align - 1); > + if (pad) > + pad = align - pad; > + aligned_size = size + pad; > + } > + > + if (sp->remaining < aligned_size) { > + spin_unlock_irqrestore(&sp->lock, flags); > + return NULL; [Severity: Medium] Does this leak the newly allocated folio if the size is too large? The new folio was just added to sp->chunks. If the requested size is larger than the entire folio's capacity, this check fails and returns NULL, but the folio remains in sp->chunks. Repeatedly requesting an oversized allocation could rapidly leak folios. > + } > + > + elem = sp->free_ptr + pad; > + sp->free_ptr += aligned_size; > + sp->remaining -= aligned_size; > + spin_unlock_irqrestore(&sp->lock, flags); > + > + memset(elem, 0, size); > + return elem; > +} > +EXPORT_SYMBOL_GPL(folio_scratchpad_alloc); > + > +noinline void folio_scratchpad_reset(struct folio_scratchpad *sp) > +{ > + struct folio_pool_chunk *head, *chunk, *tmp; > + size_t header_offset; > + unsigned long flags; > + > + spin_lock_irqsave(&sp->lock, flags); > + if (list_empty(&sp->chunks)) { > + sp->free_ptr = NULL; > + sp->remaining = 0; > + spin_unlock_irqrestore(&sp->lock, flags); > + return; > + } > + > + /* Retain primary head chunk; free overflow chunks */ > + head = list_first_entry(&sp->chunks, struct folio_pool_chunk, link); [Severity: Medium] Does list_first_entry() retain the newest chunk instead of the primary large chunk? Since list_add() is used during allocation, new chunks are prepended in a LIFO order. If the final chunk was a small fallback order-0 chunk, it seems this would free the large capacity chunk and retain the tiny chunk for the next batch. > + list_for_each_entry_safe(chunk, tmp, &sp->chunks, link) { > + if (chunk == head) > + continue; > + list_del(&chunk->link); > + folio_put(chunk->folio); > + } [ ... ] > +void folio_scratchpad_stats(struct folio_scratchpad *sp, unsigned int > *nr_chunks, > + size_t *chunk_size, size_t *tail_used) > +{ > + unsigned long flags; > + size_t csz; > + > + csz = (PAGE_SIZE << sp->chunk_order); > + if (chunk_size) > + *chunk_size = csz; > + > + spin_lock_irqsave(&sp->lock, flags); > + if (nr_chunks) > + *nr_chunks = list_count_nodes(&sp->chunks); > + if (tail_used) > + *tail_used = csz > sp->remaining ? csz - sp->remaining : 0; [Severity: Low] Does this calculate incorrect telemetry when falling back to order-0 folios? If the allocator falls back to an order-0 folio, sp->remaining is relative to PAGE_SIZE. Subtracting the smaller order-0 remainder from the theoretical high-order total (csz) seems to compute an artificially inflated tail_used metric. > + spin_unlock_irqrestore(&sp->lock, flags); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
