| Message ID | Z8hyV4sROVDtwRDE@Arch (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slub: Adds a way to handle freelist cycle in on_freelist() | expand |
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>
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> >
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;
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(-)