Re: [PATCH 03/14] io_uring: specify freeptr usage for SLAB_TYPESAFE_BY_RCU io_kiocb cache

[Vlastimil Babka <vbabka@suse.cz>]

On 11/20/24 10:07, Geert Uytterhoeven wrote:
> Hi Vlastimil,
> 
>> >>
>> >> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>> >> index 593c10a02144..8ed9c6923668 100644
>> >> --- a/include/linux/io_uring_types.h
>> >> +++ b/include/linux/io_uring_types.h
>> >> @@ -674,7 +674,11 @@ struct io_kiocb {
>> >>         struct io_kiocb                 *link;
>> >>         /* custom credentials, valid IFF REQ_F_CREDS is set */
>> >>         const struct cred               *creds;
>> >> -       struct io_wq_work               work;
>> >> +
>> >> +       union {
>> >> +               struct io_wq_work       work;
>> >> +               freeptr_t               freeptr __aligned(sizeof(freeptr_t));
>> >
>> > I'd rather add the __aligned() to the definition of freeptr_t, so it
>> > applies to all (future) users.
>> >
>> > But my main question stays: why is the slab code checking
>> > IS_ALIGNED(args->freeptr_offset, sizeof(freeptr_t)?
>>
>> I believe it's to match how SLUB normally calculates the offset if no
>> explicit one is given, in calculate_sizes():
>>
>> s->offset = ALIGN_DOWN(s->object_size / 2, sizeof(void *));
>>
>> Yes there's a sizeof(void *) because freepointer used to be just that and we
>> forgot to update this place when freepointer_t was introduced (by Jann in
>> 44f6a42d49350) for handling CONFIG_SLAB_FREELIST_HARDENED. In
>> get_freepointer() you can see how there's a cast to a pointer eventually.
>>
>> Does m68k have different alignment for pointer and unsigned long or both are
>> 2 bytes? Or any other arch, i.e. should get_freepointer be a union with
>> unsigned long and void * instead? (or it doesn't matter?)
> 
> The default alignment for int, long, and pointer is 2 on m68k.
> On CRIS (no longer supported by Linux), it was 1, IIRC.
> So the union won't make a difference.
> 
>> > Perhaps that was just intended to be __alignof__ instead of sizeof()?
>>
>> Would it do the right thing everywhere, given the explanation above?
> 
> It depends. Does anything rely on the offset being a multiple of (at
> least) 4?
> E.g. does anything counts in multiples of longs (hi BCPL! ;-), or are
> the 2 LSB used for a special purpose? (cfr. maple_tree, which uses
> bit 0 (https://elixir.bootlin.com/linux/v6.12/source/include/linux/maple_tree.h#L46)?

AFAIK no, the goal was just to prevent misaligned accesses. Kees added the:

s->offset = ALIGN_DOWN(s->object_size / 2, sizeof(void *));

so maybe he had something else in mind. But I suspect it was just because
the code already used it elsewhere.

So we might want something like this? But that would be safer for 6.14 so
I'd suggest the io_uring specific fix meanwhile. Or maybe just add the union
with freeptr_t but without __aligned plus the part below that changes
mm/slab_common.c only, as the 6.13 io_uring fix?

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 893d32059915..477fa471da18 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -230,7 +230,7 @@ static struct kmem_cache *create_cache(const char *name,
 	if (args->use_freeptr_offset &&
 	    (args->freeptr_offset >= object_size ||
 	     !(flags & SLAB_TYPESAFE_BY_RCU) ||
-	     !IS_ALIGNED(args->freeptr_offset, sizeof(freeptr_t))))
+	     !IS_ALIGNED(args->freeptr_offset, __alignof__(freeptr_t))))
 		goto out;
 
 	err = -ENOMEM;
diff --git a/mm/slub.c b/mm/slub.c
index 5b832512044e..6ad904be7700 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5287,11 +5287,7 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
 	unsigned int size = s->object_size;
 	unsigned int order;
 
-	/*
-	 * Round up object size to the next word boundary. We can only
-	 * place the free pointer at word boundaries and this determines
-	 * the possible location of the free pointer.
-	 */
+	/* Round up object size to the next word boundary. */
 	size = ALIGN(size, sizeof(void *));
 
 #ifdef CONFIG_SLUB_DEBUG
@@ -5325,7 +5321,7 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
 	if (((flags & SLAB_TYPESAFE_BY_RCU) && !args->use_freeptr_offset) ||
 	    (flags & SLAB_POISON) || s->ctor ||
 	    ((flags & SLAB_RED_ZONE) &&
-	     (s->object_size < sizeof(void *) || slub_debug_orig_size(s)))) {
+	     (s->object_size < sizeof(freeptr_t) || slub_debug_orig_size(s)))) {
 		/*
 		 * Relocate free pointer after the object if it is not
 		 * permitted to overwrite the first word of the object on
@@ -5343,7 +5339,7 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
 		 * longer true, the function needs to be modified.
 		 */
 		s->offset = size;
-		size += sizeof(void *);
+		size += sizeof(freeptr_t);
 	} else if ((flags & SLAB_TYPESAFE_BY_RCU) && args->use_freeptr_offset) {
 		s->offset = args->freeptr_offset;
 	} else {
@@ -5352,7 +5348,7 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
 		 * it away from the edges of the object to avoid small
 		 * sized over/underflows from neighboring allocations.
 		 */
-		s->offset = ALIGN_DOWN(s->object_size / 2, sizeof(void *));
+		s->offset = ALIGN_DOWN(s->object_size / 2, __alignof__(freeptr_t));
 	}
 
 #ifdef CONFIG_SLUB_DEBUG