On 3/2/25 19:01, Lilith Persefoni Gkini wrote:
> The on_freelist() uses a while loop to walk through the linked list
> freelist of a particular slab until it finds the `search` pattern and
> breaks if there is a freepointer in the list that is NULL, or invalid
> (fails the check_valid_pointer() check), or the number of objects (nr)
> in the freelist is more than `slab->objects + 1`
>
> No valid freelist should have more than slab->objects non NULL pointers,
> therefore the while conditional should check until slab->objects amount
> of times, not more.
In v1 discussion you explained how this can later lead to setting
slab->inuse to -1. I think it's useful to say it here in the changelog
because it's fixing a more serious problem than just doing an unnecessary
loop iteration.
> If the `search` pattern is not found in the freelist then the function
> should return `fp == search` where fp is the last freepointer from the
> while loop.
>
> If the caller of the function was searching for NULL and the freelist is
> valid it should return True (1), otherwise False (0).
This suggests we should change the function return value to bool :)
> Signed-off-by: Lilith Persefoni Gkini <lilithgkini@xxxxxxxxx>
> ---
> mm/slub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..0d3dd429b095 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1435,7 +1435,7 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> int max_objects;
>
> fp = slab->freelist;
> - while (fp && nr <= slab->objects) {
> + while (fp && nr < slab->objects) {
> if (fp == search)
> return 1;
> if (!check_valid_pointer(s, slab, fp)) {
> @@ -1473,7 +1473,7 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> slab->inuse = slab->objects - nr;
> slab_fix(s, "Object count adjusted");
> }
> - return search == NULL;
> + return fp == search;
> }
This seems ok and fixes the issue with setting inuse to -1, while
on_freelist(..., NULL) keeps working, AFAICS.
But I'm wondering if we still have some gap in case there's a cycle in the
freelist as such that we finish the loop with nr == slab->objects and fp is
not NULL. check_valid_pointer() will not catch it as every pointer seems
valid on its own.
This will happen either
- from free_consistency_checks() when searching for an object that's not on
the freelist. Notet his check should avoid the freelist cycle happening in
the first place, by preventing a double free. But it could also happen due
to some other (deliberate?) corruption.
- the call validate_slab() searching for NULL will be returning false
I think there's a problem that none of this will fix or even report the
situation properly. Even worse we'll set slab->inuse to 0 and thus pretend
all objects are free. This goes contrary to the other places that respond to
slab corruption by setting all objects to used and trying not to touch the
slab again at all.
So I think after the while loop we could determine there was a cycle if (nr
== slab->objects && fp != NULL), right? In that case we could perform the
same report and fix as in the "Freepointer corrupt" case?
