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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Vlastimil,

On Wed, Nov 20, 2024 at 10:37 AM Vlastimil Babka <vbabka@suse.cz> 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@linux-m68k.org

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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