[PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Persefoni Gkini]

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.

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

Signed-off-by: Lilith Persefoni Gkini <lilithgkini@proton.me>
---
 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;
 }
 
 static void trace(struct kmem_cache *s, struct slab *slab, void *object,
-- 
2.48.1

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka]

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

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Gkini]

On Mon, Mar 03, 2025 at 12:06:58PM +0100, Vlastimil Babka wrote:
> On 3/2/25 19:01, Lilith Persefoni Gkini wrote:
> > 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 :)
> 

Alright, If you want to be more technical it's
`1 (true), otherwise 0 (false).`
Its just easier to communicate with the true or false concepts, but in C
we usually don't use bools cause its just 1s or 0s.

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

True! We could either add an if check after the while as you said to
replicate the "Freepointer corrupt" behavior...
Or...

I hate to say it, or we could leave the while condition with the equal
sign intact, as it was, and change that `if` check from
`if (!check_valid_pointer(s, slab, fp)) {`
to
`if (!check_valid_pointer(s, slab, fp) || nr == slab->objects) {`

When it reaches nr == slab->objects and we are still in the while loop
it means that fp != NULL and therefore the freelist is corrupted (note
that nr starts from 0).

This would add fewer lines of code and there won't be any repeating
code.
It will enter in the "Freechain corrupt" branch and set the tail of 
the freelist to NULL, inform us of the error and it won't get a chance
to do the nr++ part, leaving nr == slab->objects in that particular 
case, because it breaks of the loop afterwards.

But it will not Null-out the freelist and set inuse to objects like you
suggested. If that is the desired behavior instead then we could do
something like you suggested.

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Christoph Lameter (Ampere)]

In Mon, 3 Mar 2025, Lilith Gkini wrote:

> Alright, If you want to be more technical it's
> `1 (true), otherwise 0 (false).`
> Its just easier to communicate with the true or false concepts, but in C
> we usually don't use bools cause its just 1s or 0s.

Its not about "technicalities". Please use the bool return type as
provided for kernel code.

Compare f.e.


static inline bool kmem_cache_has_cpu_partial(struct kmem_cache *s)
{
#ifdef CONFIG_SLUB_CPU_PARTIAL
        return !kmem_cache_debug(s);
#else
        return false;
#endif
}

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka]

On 3/3/25 17:41, Lilith Gkini wrote:
> On Mon, Mar 03, 2025 at 12:06:58PM +0100, Vlastimil Babka wrote:
>> On 3/2/25 19:01, Lilith Persefoni Gkini wrote:
>> > 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 :)
>> 
> 
> Alright, If you want to be more technical it's
> `1 (true), otherwise 0 (false).`
> Its just easier to communicate with the true or false concepts, but in C
> we usually don't use bools cause its just 1s or 0s.

Yeah, I think bools were not used initially int the kernel, but we're fine
with them now and changing a function for other reasons is a good
opportunity to modernize. There are some guidelines in
Documentation/process/coding-style.rst about this (paragraphs 16 and 17).
int is recommended if 0 means success and -EXXX for error, bool for simple
true/false which is the case here.

>> 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?
> 
> True! We could either add an if check after the while as you said to
> replicate the "Freepointer corrupt" behavior...
> Or...
> 
> I hate to say it, or we could leave the while condition with the equal
> sign intact, as it was, and change that `if` check from
> `if (!check_valid_pointer(s, slab, fp)) {`
> to
> `if (!check_valid_pointer(s, slab, fp) || nr == slab->objects) {`

You're right!

> When it reaches nr == slab->objects and we are still in the while loop
> it means that fp != NULL and therefore the freelist is corrupted (note
> that nr starts from 0).
> 
> This would add fewer lines of code and there won't be any repeating
> code.
> It will enter in the "Freechain corrupt" branch and set the tail of 
> the freelist to NULL, inform us of the error and it won't get a chance
> to do the nr++ part, leaving nr == slab->objects in that particular 
> case, because it breaks of the loop afterwards.
> 
> But it will not Null-out the freelist and set inuse to objects like you
> suggested. If that is the desired behavior instead then we could do
> something like you suggested.

We could change if (object) to if (object && nr != slab->objects) to force
it into the "Freepointer corrupt" variant which is better. But then the
message should be also adjusted depending on nr... it should really report
"Freelist cycle detected", but that's adding too many conditions just to
reuse the cleanup code so maybe it's more readable to check that outside of
the while loop after all.

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Gkini]

On Mon, Mar 03, 2025 at 08:06:32PM +0100, Vlastimil Babka wrote:
> On 3/3/25 17:41, Lilith Gkini wrote:
> > On Mon, Mar 03, 2025 at 12:06:58PM +0100, Vlastimil Babka wrote:
> >> On 3/2/25 19:01, Lilith Persefoni Gkini wrote:
> >> > 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 :)
> >> 
> > 
> > Alright, If you want to be more technical it's
> > `1 (true), otherwise 0 (false).`
> > Its just easier to communicate with the true or false concepts, but in C
> > we usually don't use bools cause its just 1s or 0s.
> 
> Yeah, I think bools were not used initially int the kernel, but we're fine
> with them now and changing a function for other reasons is a good
> opportunity to modernize. There are some guidelines in
> Documentation/process/coding-style.rst about this (paragraphs 16 and 17).
> int is recommended if 0 means success and -EXXX for error, bool for simple
> true/false which is the case here.

Oh! because of the emote I thought you were being sarcastic that I didnt
report it properly.
Thank you for clarifying! That should be an easy fix!

> >> 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?
> > 
> > True! We could either add an if check after the while as you said to
> > replicate the "Freepointer corrupt" behavior...
> > Or...
> > 
> > I hate to say it, or we could leave the while condition with the equal
> > sign intact, as it was, and change that `if` check from
> > `if (!check_valid_pointer(s, slab, fp)) {`
> > to
> > `if (!check_valid_pointer(s, slab, fp) || nr == slab->objects) {`
> 
> You're right!
> 
> > When it reaches nr == slab->objects and we are still in the while loop
> > it means that fp != NULL and therefore the freelist is corrupted (note
> > that nr starts from 0).
> > 
> > This would add fewer lines of code and there won't be any repeating
> > code.
> > It will enter in the "Freechain corrupt" branch and set the tail of 
> > the freelist to NULL, inform us of the error and it won't get a chance
> > to do the nr++ part, leaving nr == slab->objects in that particular 
> > case, because it breaks of the loop afterwards.
> > 
> > But it will not Null-out the freelist and set inuse to objects like you
> > suggested. If that is the desired behavior instead then we could do
> > something like you suggested.
> 
> We could change if (object) to if (object && nr != slab->objects) to force
> it into the "Freepointer corrupt" variant which is better. But then the

We could add a ternary operator in addition to you suggestion.
Changing this:
`slab_err(s, slab, "Freepointer corrupt");`

to this (needs adjusting for the proper formating ofc...):
`slab_err(s, slab, (nr == slab->objects) ? "Freelist cycle detected" : "Freepointer corrupt");`

But this might be too much voodoo...

> message should be also adjusted depending on nr... it should really report

I m not sure what you have in mind about the adjusting the message on
nr. Do we really need to report the nr in the error? Do we need to
mention anything besides "Freelist cycle detected" like you mentioned?

> "Freelist cycle detected", but that's adding too many conditions just to
> reuse the cleanup code so maybe it's more readable to check that outside of
> the while loop after all.

If the ternary operator is too unreadable we could do something like you
suggested

```
if (fp != NULL && nr == slab->objects) {
	slab_err(s, slab, "Freelist cycle detected");
	slab->freelist = NULL;
	slab->inuse = slab->objects;
	slab_fix(s, "Freelist cleared");
	return false;
}
```

What more would you like to add in the error message?

In a previous email you mentioned this

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

If nuking it is how we should hangle corrupted freelists shouldn't we
also do the same in the "Freechain corrupt" branch? Otherwise it
wouldn't be consistent. Instead the code now just sets the tail to NULL.

In that case we'll need to do a lot more rewriting, but it might help
out with avoiding the reuse of cleanup code.

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka]

On 3/4/25 09:24, Lilith Gkini wrote:
> On Mon, Mar 03, 2025 at 08:06:32PM +0100, Vlastimil Babka wrote:
>> Yeah, I think bools were not used initially int the kernel, but we're fine
>> with them now and changing a function for other reasons is a good
>> opportunity to modernize. There are some guidelines in
>> Documentation/process/coding-style.rst about this (paragraphs 16 and 17).
>> int is recommended if 0 means success and -EXXX for error, bool for simple
>> true/false which is the case here.
> 
> Oh! because of the emote I thought you were being sarcastic that I didnt
> report it properly.

Ah, sorry about that misunderstanding.

> Thank you for clarifying! That should be an easy fix!

Great!

>> > When it reaches nr == slab->objects and we are still in the while loop
>> > it means that fp != NULL and therefore the freelist is corrupted (note
>> > that nr starts from 0).
>> > 
>> > This would add fewer lines of code and there won't be any repeating
>> > code.
>> > It will enter in the "Freechain corrupt" branch and set the tail of 
>> > the freelist to NULL, inform us of the error and it won't get a chance
>> > to do the nr++ part, leaving nr == slab->objects in that particular 
>> > case, because it breaks of the loop afterwards.
>> > 
>> > But it will not Null-out the freelist and set inuse to objects like you
>> > suggested. If that is the desired behavior instead then we could do
>> > something like you suggested.
>> 
>> We could change if (object) to if (object && nr != slab->objects) to force
>> it into the "Freepointer corrupt" variant which is better. But then the
> 
> We could add a ternary operator in addition to you suggestion.
> Changing this:
> `slab_err(s, slab, "Freepointer corrupt");`
> 
> to this (needs adjusting for the proper formating ofc...):
> `slab_err(s, slab, (nr == slab->objects) ? "Freelist cycle detected" : "Freepointer corrupt");`
> 
> But this might be too much voodoo...

Yeah it means 3 places where we check (nr == slab->objects) so it's not very
readable.

>> message should be also adjusted depending on nr... it should really report
> 
> I m not sure what you have in mind about the adjusting the message on
> nr. Do we really need to report the nr in the error? Do we need to
> mention anything besides "Freelist cycle detected" like you mentioned?

I meant just the Freelist cycle detected" message. As "nr" equals
slab->objects it's not so interesting.

>> "Freelist cycle detected", but that's adding too many conditions just to
>> reuse the cleanup code so maybe it's more readable to check that outside of
>> the while loop after all.
> 
> If the ternary operator is too unreadable we could do something like you
> suggested
> 
> ```
> if (fp != NULL && nr == slab->objects) {
> 	slab_err(s, slab, "Freelist cycle detected");
> 	slab->freelist = NULL;
> 	slab->inuse = slab->objects;
> 	slab_fix(s, "Freelist cleared");
> 	return false;
> }
> ```

Yeah looks good.
> 
> What more would you like to add in the error message?
> 
> In a previous email you mentioned this
> 
>> >> 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.
> 
> If nuking it is how we should hangle corrupted freelists shouldn't we
> also do the same in the "Freechain corrupt" branch? Otherwise it
> wouldn't be consistent. Instead the code now just sets the tail to NULL.

It sets the tail to NULL but then also breaks out of the loop (btw that
break; could be moved to the if (object) branch to make it more obvious) to
the code below, which should also set slab->inuse properly. So the result
should be consistent? In that case we're able to salvage at least the
uncorrupted part of the freelist. It's likely corrupted by a use-after-free
of a single object overwriting the freepointer.

In case of the cycle we could also in theory replace the freepointer causing
the cycle to NULL. But we'd have to spend more effort to determine which it
was. Cycle is also probably due to a more serious issue than single object
overwrite - it's unlikely a random overwrite would corrupt a freepointer in
a way that it's valid but causing a cycle. So throwing out everything seems
the easiest.

> In that case we'll need to do a lot more rewriting, but it might help
> out with avoiding the reuse of cleanup code.

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Gkini]

On Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote:
> On 3/4/25 09:24, Lilith Gkini wrote:
> > On Mon, Mar 03, 2025 at 08:06:32PM +0100, Vlastimil Babka wrote:
> >> Yeah, I think bools were not used initially int the kernel, but we're fine
> >> with them now and changing a function for other reasons is a good
> >> opportunity to modernize. There are some guidelines in
> >> Documentation/process/coding-style.rst about this (paragraphs 16 and 17).
> >> int is recommended if 0 means success and -EXXX for error, bool for simple
> >> true/false which is the case here.
> > 
> > Oh! because of the emote I thought you were being sarcastic that I didnt
> > report it properly.
> 
> Ah, sorry about that misunderstanding.
> 
> > Thank you for clarifying! That should be an easy fix!
> 
> Great!
> 
> >> > When it reaches nr == slab->objects and we are still in the while loop
> >> > it means that fp != NULL and therefore the freelist is corrupted (note
> >> > that nr starts from 0).
> >> > 
> >> > This would add fewer lines of code and there won't be any repeating
> >> > code.
> >> > It will enter in the "Freechain corrupt" branch and set the tail of 
> >> > the freelist to NULL, inform us of the error and it won't get a chance
> >> > to do the nr++ part, leaving nr == slab->objects in that particular 
> >> > case, because it breaks of the loop afterwards.
> >> > 
> >> > But it will not Null-out the freelist and set inuse to objects like you
> >> > suggested. If that is the desired behavior instead then we could do
> >> > something like you suggested.
> >> 
> >> We could change if (object) to if (object && nr != slab->objects) to force
> >> it into the "Freepointer corrupt" variant which is better. But then the
> > 
> > We could add a ternary operator in addition to you suggestion.
> > Changing this:
> > `slab_err(s, slab, "Freepointer corrupt");`
> > 
> > to this (needs adjusting for the proper formating ofc...):
> > `slab_err(s, slab, (nr == slab->objects) ? "Freelist cycle detected" : "Freepointer corrupt");`
> > 
> > But this might be too much voodoo...
> 
> Yeah it means 3 places where we check (nr == slab->objects) so it's not very
> readable.
> 
> >> message should be also adjusted depending on nr... it should really report
> > 
> > I m not sure what you have in mind about the adjusting the message on
> > nr. Do we really need to report the nr in the error? Do we need to
> > mention anything besides "Freelist cycle detected" like you mentioned?
> 
> I meant just the Freelist cycle detected" message. As "nr" equals
> slab->objects it's not so interesting.
> 
> >> "Freelist cycle detected", but that's adding too many conditions just to
> >> reuse the cleanup code so maybe it's more readable to check that outside of
> >> the while loop after all.
> > 
> > If the ternary operator is too unreadable we could do something like you
> > suggested
> > 
> > ```
> > if (fp != NULL && nr == slab->objects) {
> > 	slab_err(s, slab, "Freelist cycle detected");
> > 	slab->freelist = NULL;
> > 	slab->inuse = slab->objects;
> > 	slab_fix(s, "Freelist cleared");
> > 	return false;
> > }
> > ```
> 
> Yeah looks good.
> > 
> > What more would you like to add in the error message?
> > 
> > In a previous email you mentioned this
> > 
> >> >> 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.
> > 
> > If nuking it is how we should hangle corrupted freelists shouldn't we
> > also do the same in the "Freechain corrupt" branch? Otherwise it
> > wouldn't be consistent. Instead the code now just sets the tail to NULL.
> 
> It sets the tail to NULL but then also breaks out of the loop (btw that
> break; could be moved to the if (object) branch to make it more obvious) to
> the code below, which should also set slab->inuse properly. So the result
> should be consistent? In that case we're able to salvage at least the
> uncorrupted part of the freelist. It's likely corrupted by a use-after-free
> of a single object overwriting the freepointer.

Yes! You are right!

I also just tested this. The "Freelist cycle detected" will get
triggered even if there is an invalid address at the tail in the case
of a full freelist, which is a bit... inacurate, right? It's technically
not a cycle in that case since the freepointer is invalid and it doesn't
point back to the slab.

- We could avoid this by nulling the fp in that case (as I suggested in v1
in previous emails) inside the "Freechain corrupt" branch, but also
reverting the while condition back to it's equal sign like it was and
then changing the new if check to:
	if (fp != NULL && nr > slab->objects) {
but it feels a bit messy.

- Or we could just change the "Freelist cycle detected" message to
something else.

- Or we could leave it as "Freelist cycle detected".

This is only a problem if the freelist is full and the tail is junk.

If the freelist is not full the code will act as you suggested.


If this is becoming too hard to follow I'll include the two diffs.

For the case were we are fine with the "Freelist cycle detected"
message, even in the case of a junk tail:

---
 mm/slub.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 1f50129dcfb3..25e972a3b914 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;
@@ -1435,29 +1435,37 @@ 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;
+			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++;
 	}
 
-	max_objects = order_objects(slab_order(slab), s->size);
+	if (fp != NULL && 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_or0der(slab), s->size);
 	if (max_objects > MAX_OBJS_PER_PAGE)
 		max_objects = MAX_OBJS_PER_PAGE;
 
-- 

and in the case where we want the code to not display "Freelist cycle
detected" we could do something like this:

---
 mm/slub.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 1f50129dcfb3..eef879d4feb1 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,27 +1437,36 @@ 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);
+				fp = 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++;
 	}
 
-	max_objects = order_objects(slab_order(slab), s->size);
+	if (fp != NULL && 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_or0der(slab), s->size);
 	if (max_objects > MAX_OBJS_PER_PAGE)
 		max_objects = MAX_OBJS_PER_PAGE;
 
-- 

Let me know what you think!

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka]

On 3/4/25 12:06, Lilith Gkini wrote:
> On Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote:
>> It sets the tail to NULL but then also breaks out of the loop (btw that
>> break; could be moved to the if (object) branch to make it more obvious) to
>> the code below, which should also set slab->inuse properly. So the result
>> should be consistent? In that case we're able to salvage at least the
>> uncorrupted part of the freelist. It's likely corrupted by a use-after-free
>> of a single object overwriting the freepointer.
> 
> Yes! You are right!
> 
> I also just tested this. The "Freelist cycle detected" will get
> triggered even if there is an invalid address at the tail in the case
> of a full freelist, which is a bit... inacurate, right? It's technically

Yes. But see my comments on the code below. I wonder why you got it triggered.

> not a cycle in that case since the freepointer is invalid and it doesn't
> point back to the slab.
> 
> - We could avoid this by nulling the fp in that case (as I suggested in v1
> in previous emails) inside the "Freechain corrupt" branch, but also
> reverting the while condition back to it's equal sign like it was and
> then changing the new if check to:
> 	if (fp != NULL && nr > slab->objects) {
> but it feels a bit messy.

I think it's not so bad.

> - Or we could just change the "Freelist cycle detected" message to
> something else.
> 
> - Or we could leave it as "Freelist cycle detected".

I'd prefer that.

> This is only a problem if the freelist is full and the tail is junk.

If the tail is junk it would be better to just fix it to NULL and not report
wrongly a cycle.

> If the freelist is not full the code will act as you suggested.
> 
> 
> If this is becoming too hard to follow I'll include the two diffs.
> 
> For the case were we are fine with the "Freelist cycle detected"
> message, even in the case of a junk tail:

<snip>

> 
> -- 
> 
> and in the case where we want the code to not display "Freelist cycle
> detected" we could do something like this:
> 
> ---
>  mm/slub.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..eef879d4feb1 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,27 +1437,36 @@ 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);
> +				fp = NULL;
> +				break;

Since we break, nr is not incremented to slab->objects + 1.

>  			} 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++;
>  	}
>  
> -	max_objects = order_objects(slab_order(slab), s->size);
> +	if (fp != NULL && nr > slab->objects) {

And thus we should not need to set fp to NULL and test it here? Am I missing
something?

> +		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_or0der(slab), s->size);
>  	if (max_objects > MAX_OBJS_PER_PAGE)
>  		max_objects = MAX_OBJS_PER_PAGE;
>  
> -- 
> 
> Let me know what you think!

The latter would be better, thanks!

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Gkini]

On Tue, Mar 04, 2025 at 12:20:03PM +0100, Vlastimil Babka wrote:
> On 3/4/25 12:06, Lilith Gkini wrote:
> > On Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote:
> > -- 
> > 
> > and in the case where we want the code to not display "Freelist cycle
> > detected" we could do something like this:
> > 
> > ---
> >  mm/slub.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 1f50129dcfb3..eef879d4feb1 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,27 +1437,36 @@ 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);
> > +				fp = NULL;
> > +				break;
> 
> Since we break, nr is not incremented to slab->objects + 1.
> 
> >  			} 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++;
> >  	}
> >  
> > -	max_objects = order_objects(slab_order(slab), s->size);
> > +	if (fp != NULL && nr > slab->objects) {
> 
> And thus we should not need to set fp to NULL and test it here? Am I missing
> something?

Thats true. I still had the return fp == search; in my mind, but with all
these changes we can just leave it as return search == NULL; as it was,
because we are handing the edge cases.

By the time it reaches that return line it should be fine.

I was also thinking of fixing two lines to adhere to the "Breaking long
lines and strings" (2) from the coding-style.

---
 mm/slub.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 1f50129dcfb3..e06b88137705 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,38 +1437,48 @@ 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++;
 	}
 
-	max_objects = order_objects(slab_order(slab), s->size);
+	if (fp != NULL && 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_or0der(slab), s->size);
 	if (max_objects > MAX_OBJS_PER_PAGE)
 		max_objects = MAX_OBJS_PER_PAGE;
 
 	if (slab->objects != max_objects) {
-		slab_err(s, slab, "Wrong number of objects. Found %d but should be %d",
+		slab_err(s, slab,
+			 "Wrong number of objects. Found %d but should be %d",
 			 slab->objects, max_objects);
 		slab->objects = max_objects;
 		slab_fix(s, "Number of objects adjusted");
 	}
 	if (slab->inuse != slab->objects - nr) {
-		slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d",
+		slab_err(s, slab,
+			 "Wrong object count. Counter is %d but counted were %d",
 			 slab->inuse, slab->objects - nr);
 		slab->inuse = slab->objects - nr;
 		slab_fix(s, "Object count adjusted");
-- 

I do have to note that the last slab_err is of length 81 with my change,
but it looks fine. If that one extra character is unacceptable let me
know so I can change it to something else.

Or if you think it's completely unnecessary I could leave it as it was
in the first place.
I just thought since we are trying to modernaze I should fix the length
as well.

Also the CHECKPATCH is complaining about the `fp != NULL` that we can
just check fp on it's own, which is technically true, but wouldn't make
readability worse?
I think its better as it's in my diff cause it's more obvious, but if
you prefer the singular fp I can change it.

If these changes are acceptable and we don't have anything further to
change or add I can send it as a proper commit again, But I should
probably break it into multiple patches.

Maybe one patch for the lines and another for the rest? Or should I
break the bool change in it's own patch?

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka]

On 3/4/25 13:18, Lilith Gkini wrote:
> On Tue, Mar 04, 2025 at 12:20:03PM +0100, Vlastimil Babka wrote:
> Thats true. I still had the return fp == search; in my mind, but with all

Ah, right.

> these changes we can just leave it as return search == NULL; as it was,
> because we are handing the edge cases.
> 
> By the time it reaches that return line it should be fine.

True.

> I was also thinking of fixing two lines to adhere to the "Breaking long
> lines and strings" (2) from the coding-style.

Hm AFAIK checkpatch was adjusted to only warn at 100 lines. While the style
document wasn't updated, we can leave such a small excess with no change.

> ---
>  mm/slub.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..e06b88137705 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,38 +1437,48 @@ 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++;
>  	}
>  
> -	max_objects = order_objects(slab_order(slab), s->size);
> +	if (fp != NULL && nr > slab->objects) {

In case nr > slab->objects we already know fp can't be NULL, no? So we don't
have to test it?

> +		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_or0der(slab), s->size);
>  	if (max_objects > MAX_OBJS_PER_PAGE)
>  		max_objects = MAX_OBJS_PER_PAGE;
>  
>  	if (slab->objects != max_objects) {
> -		slab_err(s, slab, "Wrong number of objects. Found %d but should be %d",
> +		slab_err(s, slab,
> +			 "Wrong number of objects. Found %d but should be %d",
>  			 slab->objects, max_objects);
>  		slab->objects = max_objects;
>  		slab_fix(s, "Number of objects adjusted");
>  	}
>  	if (slab->inuse != slab->objects - nr) {
> -		slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d",
> +		slab_err(s, slab,
> +			 "Wrong object count. Counter is %d but counted were %d",
>  			 slab->inuse, slab->objects - nr);
>  		slab->inuse = slab->objects - nr;
>  		slab_fix(s, "Object count adjusted");
> 
> I do have to note that the last slab_err is of length 81 with my change,
> but it looks fine. If that one extra character is unacceptable let me
> know so I can change it to something else.
> Or if you think it's completely unnecessary I could leave it as it was
> in the first place.

Yeah can leave it.

> I just thought since we are trying to modernaze I should fix the length
> as well.
> 
> Also the CHECKPATCH is complaining about the `fp != NULL` that we can
> just check fp on it's own, which is technically true, but wouldn't make
> readability worse?
> I think its better as it's in my diff cause it's more obvious, but if
> you prefer the singular fp I can change it.

I think it's not necessary to test at all but in case I'm wrong, we can do
what checkpatch suggests to be consistent with the while() condition.

> If these changes are acceptable and we don't have anything further to
> change or add I can send it as a proper commit again, But I should
> probably break it into multiple patches.

It's fine as a single patch. Thanks!

> Maybe one patch for the lines and another for the rest? Or should I
> break the bool change in it's own patch?

Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Lilith Gkini]

On Tue, Mar 04, 2025 at 03:25:26PM +0100, Vlastimil Babka wrote:
> On 3/4/25 13:18, Lilith Gkini wrote:
> > On Tue, Mar 04, 2025 at 12:20:03PM +0100, Vlastimil Babka wrote:
> > I was also thinking of fixing two lines to adhere to the "Breaking long
> > lines and strings" (2) from the coding-style.
> 
> Hm AFAIK checkpatch was adjusted to only warn at 100 lines. While the style
> document wasn't updated, we can leave such a small excess with no change.

Yeah, it didn't complain about it, I noticed it while having multiple
windows open with the diffs and all.

> > ---
> >  mm/slub.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 1f50129dcfb3..e06b88137705 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,38 +1437,48 @@ 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++;
> >  	}
> >  
> > -	max_objects = order_objects(slab_order(slab), s->size);
> > +	if (fp != NULL && nr > slab->objects) {
> 
> In case nr > slab->objects we already know fp can't be NULL, no? So we don't
> have to test it?

...Yeah. All these different diffs got me confused. What a mess.

I just tested it in a debugger. That fp null check isn't necessary.

I'll send the full patch tomorrow or something, when I check it again
with a clear head. I dont want to do any mistakes in the actual patch.

> > I do have to note that the last slab_err is of length 81 with my change,
> > but it looks fine. If that one extra character is unacceptable let me
> > know so I can change it to something else.
> > Or if you think it's completely unnecessary I could leave it as it was
> > in the first place.
> 
> Yeah can leave it.
> 

Alright, I wont include the line breaks in the patch then! I'll leave it as it
was.

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

[Lilith Gkini]

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.

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

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

[Harry Yoo]

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
> 

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

[Vlastimil Babka]

On 3/6/25 09:34, Harry Yoo wrote:
> 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()"

Right, some of the sentences later too. I've adjusted it a bit to avoid the
need to resubmit, hope it's ok. The result is here in slab/for-next

https://web.git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.15/fixes-cleanups&id=747e2cf137f44058a093d3226bf83974d9d117e7

Thanks a lot!

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