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
