Re: [PATCH] slub: Adds a way to handle freelist cycle in on_freelist()

[Harry Yoo <harry.yoo@oracle.com>]

Hi Lilith, the patch looks good, and it's great to see the improvements
over the revisions! I've added my Reviewed-by: tag after the '---' line.

A few nit comments are inlined below.

From Documentation/process/submitting-patches.rst:
> Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
> instead of “[This patch] makes xyzzy do frotz” or
> “[I] changed xyzzy to do frotz”, as if you are giving orders to the codebase
> to change its behaviour.

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@proton.me>
> ---

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
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
>