On Thu, Feb 20, 2025 at 06:21:14PM +0900, Harry Yoo wrote:
> On Thu, Feb 20, 2025 at 05:20:00PM +0900, Harry Yoo wrote:
> > On Sat, Feb 15, 2025 at 06:57:01PM +0200, Lilitha Persefoni Gkini wrote:
> > > The condition `nr <= slab->objects` in the `on_freelist()` serves as
> > > bound while walking through the `freelist` linked list because we can't
> > > have more free objects than the maximum amount of objects in the slab.
> > > But the `=` can result in an extra unnecessary iteration.
> > >
> > > The patch changes it to `nr < slab->objects` to ensure it iterates
> > > at most `slab->objects` number of times.
> > >
> > > Signed-off-by: Lilitha Persefoni Gkini <lilithpgkini@xxxxxxxxx>
> > > ---
> > > mm/slub.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index 1f50129dcfb3..ad42450d4b0f 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) {
> >
> > Hi, this makes sense to me.
> >
> > But based on what the name of the variable suggests (nr of objects),
> > I think it makes clearer to initialize it to 1 instead?
>
> Oh, actually iterating at most (slab->objects + 1) times allows it to catch
> cases where the freelist does not end with NULL (see how validate_slab()
> calls on_freelist(), passing search = NULL).
>
> It's very subtle. A comment like this would help:
>
> /*
> * Iterate at most slab->objects + 1 times to handle cases
> * where the freelist does not end with NULL.
> */
>
> --
> Cheers,
> Harry
nr is the number of "free objects" in the freelist, so it should start
from zero for the case when there are no free objects.
Oh, you think its on purpose to catch edgecases, like a defensive
programing sort of way? Huh, thats interesting!
In that case it would prevent a case where every object in the slab is
freed and the tail of the freelist isnt NULL like it should be, maybe because
of some Out-Of-Bounds write from another object, or a Use-After-Free.
If that pointer is some gibberish then the chech_valid_pointer() check
on line 1441 will catch it, set it as NULL in line 1445 with
set_freepointer() and then break from the While and continue with the
rest of the program. nr will correctly remain as the number of freed
objects and the freelist will have a NULL in its tail, as it should!
But if the pointer isn't some random address and instead is an address in
the slab, lets say as an example the address of a free object in the
linked list (making the freelist cicrular) it wont get caught by the
check_valid_pointer() since technically it is a valid pointer, it will
increment nr to slab->objects + 1 and then exit the While loop because
it will fail the conditional nr <= slab->objects.
Then later on, in line 1470 slab->objects - nr will be -1 which is not
equals to slab->inuse and enter the If case where it will set the
slab->inuse to -1, but because slab-inuse is an unsinged short it will
be stored as 0xFFFF, ie 65535, corrupting the slab struct with an
unreasonably large "inuse" value.
You mentioned validate_slab(), I assume you are refering to how it
searches for NULL when it calls on_freelist() and if it does find NULL
in the freelist it will return 1 (basicaly TRUE).
In the example where every object is freed it will return TRUE
regardless if NULL is in the freelist or not, because on_freelist()
returns search == NULL if it doesnt find the search in the freelist. In
this case it would be NULL == NULL which is TRUE again.
This will have the same behavior even if we remove the equals sign from
the While, like the Patch suggests.
I am still pretty new to this so I apologize for any mistakes. I
appreciate the feedback!
Is it ok to refer to lines of code, or should I copy paste the entire line?
I understand that even small changes could have a huge effect to some
other function or subsystem in ways that might not be obvious to someone
not as familiar with the codebase. I hope I am not coming off to
strong or anything.
