nit: Per submitting-patches.rst, I think the subject could be:
- "slub: Add a way to handle freelist cycle in on_freelist()"
or more concisely,
- "slub: Handle freelist cycle in on_freelist()"
On Wed, Mar 05, 2025 at 05:48:39PM +0200, Lilith Gkini wrote:
> The on_freelist() doesn't have a way to handle the edgecase of having a
> full freelist that doesn't end in NULL and instead has another valid
> pointer in the slab as a result of a Use-After-Free or anything similar.
>
> This case won't get caught by check_valid_pointer() and it will result in
> nr incrementing to `slab->objects + 1`, corrupting the slab->inuse entry
> later in the code by setting it to -1.
>
> The Patch adds an if check to detect that case, notifies us and handles
> the freelist and slab appropriately, as is the standard process in these
> situations.
>
> Furthermore the Patch changes the return type of the function from
> int to bool as per codying style guidelines.
nit: codying -> coding
> It also moves the `break;` line inside the `if (object) {` to make it more
> obvious that the code breaks the while loop in that branch.
>
> Signed-off-by: Lilith Persefoni Gkini <lilithgkini@xxxxxxxxx>
> ---
Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@xxxxxxxxxx>
--
Cheers,
Harry
> mm/slub.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..95e54ffd5330 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1427,7 +1427,7 @@ static int check_slab(struct kmem_cache *s, struct slab *slab)
> * Determine if a certain object in a slab is on the freelist. Must hold the
> * slab lock to guarantee that the chains are in a consistent state.
> */
> -static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> +static bool on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> {
> int nr = 0;
> void *fp;
> @@ -1437,26 +1437,34 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> fp = slab->freelist;
> while (fp && nr <= slab->objects) {
> if (fp == search)
> - return 1;
> + return true;
> if (!check_valid_pointer(s, slab, fp)) {
> if (object) {
> object_err(s, slab, object,
> "Freechain corrupt");
> set_freepointer(s, object, NULL);
> + break;
> } else {
> slab_err(s, slab, "Freepointer corrupt");
> slab->freelist = NULL;
> slab->inuse = slab->objects;
> slab_fix(s, "Freelist cleared");
> - return 0;
> + return false;
> }
> - break;
> }
> object = fp;
> fp = get_freepointer(s, object);
> nr++;
> }
>
> + if (nr > slab->objects) {
> + slab_err(s, slab, "Freelist cycle detected");
> + slab->freelist = NULL;
> + slab->inuse = slab->objects;
> + slab_fix(s, "Freelist cleared");
> + return false;
> + }
> +
> max_objects = order_objects(slab_order(slab), s->size);
> if (max_objects > MAX_OBJS_PER_PAGE)
> max_objects = MAX_OBJS_PER_PAGE;
> --
> 2.48.1
>
