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!
