Hi Vlastimil,
On Wed, Nov 20, 2024 at 10:37 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
On 11/20/24 10:07, Geert Uytterhoeven wrote:
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?
As it seems to work fine with s/sizeof/__alignof/, I have submitted
a patch to just make that change
https://lore.kernel.org/80c767a5d5927c099aea5178fbf2c897b459fa90.1732106544.git.geert@xxxxxxxxxxxxxx
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
