| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z8bvfiyLelfXskNw@Arch>
Date: Tue, 4 Mar 2025 14:18:06 +0200
From: Lilith Gkini <lilithpgkini@...il.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, harry.yoo@...cle.com
Subject: Re: [PATCH] slub: Fix Off-By-One in the While condition in
on_freelist()
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?
Powered by blists - more mailing lists