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@xxxxxxxxx>
>> ---
> 
> Looks good to me,
> Reviewed-by: Harry Yoo <harry.yoo@xxxxxxxxxx>
>