On Tue, Feb 03, 2026 at 11:39:43AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 3, 2026, at 11:22, Michael S. Tsirkin wrote:
> > On Mon, Feb 02, 2026 at 11:48:08PM +0100, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <[email protected]>
> >>
> >> These two ioctls are incompatible on 32-bit x86 userspace, because
> >> the data structures are shorter than they are on 64-bit.
> >>
> >> Add a proper .compat_ioctl handler for x86 that reads the structures
> >> with the smaller padding before calling the internal handlers.
> >>
> >> Fixes: ad146355bfad ("vduse: Support querying information of IOVA regions")
> >> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> >> Signed-off-by: Arnd Bergmann <[email protected]>
> >> ---
> >> The code is directly copied from the native ioctl handler, but I
> >> did not test this with actual x86-32 userspace, so please review
> >> carefully.
> >
> > More importantly, I'm not applying this until it's tested)
>
> Sure
>
> > ifndef CONFIG_COMPAT around the structs will make it clearer
> > they are only for this purpose.
> >
> >> + * i386 has different alignment constraints than x86_64,
> >
> > why i386 specifically? many architectures have CONFIG_COMPAT
> > and it looks like all of them will have the issue.
>
> No, the weird alignment rules are only on arc, csky, m68k,
> microblaze, nios2, openrisc, sh and x86-32. Out of those,
> x86 is hte only one that currently has a 64-bit version
> (arc and micrblaze 64-bit support never made it upstream,
> sh64 was removed since there were no products).
>
> All the other architectures with compat support (arm,
> powerpc, mips, sparc, riscv) have the same alignment rules
> for 32-bit and 64-bit builds and align all integers naturally.
Oh interesting. But the code is compiled for
and generates useless code for all CONFIG_COMPAT right now.
The ifdef you need is COMPAT_FOR_U64_ALIGNMENT then, I think.
> >> + * so there are only 3 bytes of padding instead of 7.
> >> + */
> >> +struct compat_vduse_iotlb_entry {
> >> + compat_u64 offset;
> >> + compat_u64 start;
> >> + compat_u64 last;
> >> + __u8 perm;
> >> + __u8 padding[__alignof__(compat_u64) - 1];
> >
> > Was surprised to learn __alignof__ can be used to size
> > arrays. This is the first use of this capability in the kernel.
> >
> > I think the point of all this is that compat_vduse_iotlb_entry
> > will be 4 byte aligned now? Very well. But why do we bother
> > with specifying the hidden padding? compilers adds exactly
> > this amount anyway. Just plan compat_u64 will do the trick.
>
> Right, I could remove the padding field here, since this is
> just used to document the size of the otherwise implied
> padding.
>
> The patch I used to find the issue originally adds explicit
> padding to all uapi structures with implied padding, so I
> did the smae thing here.
>
> >> +#define COMPAT_VDUSE_IOTLB_GET_FD _IOWR(VDUSE_BASE, 0x10, struct
> >> compat_vduse_iotlb_entry)
> >> +
> >> +struct compat_vduse_vq_info {
> >> + __u32 index;
> >> + __u32 num;
> >> + compat_u64 desc_addr;
> >> + compat_u64 driver_addr;
> >> + compat_u64 device_addr;
> >> + union {
> >> + struct vduse_vq_state_split split;
> >> + struct vduse_vq_state_packed packed;
> >> + };
> >> + __u8 ready;
> >> + __u8 padding[__alignof__(compat_u64) - 1];
> >> +} __uapi_arch_align;
> >
> > it's a global variable? I'm not aware of this trick. What is this doing?
>
> My mistake, that should not have been here.
>
> >> @@ -1678,7 +1799,7 @@ static const struct file_operations vduse_dev_fops =
> >> {
> >> .write_iter = vduse_dev_write_iter,
> >> .poll = vduse_dev_poll,
> >> .unlocked_ioctl = vduse_dev_ioctl,
> >> - .compat_ioctl = compat_ptr_ioctl,
> >> + .compat_ioctl = PTR_IF(IS_ENABLED(CONFIG_COMPAT),
> >> vduse_dev_compat_ioctl),
> >
> > Too funky IMHO. Everyone uses ifdef around this, let's do the same.
>
> Sure. I only used this because you asked for fewer #ifdefs in
> my v1 patch.
It's less the amount of ifdefs more them being placed strategically.
sorry about being unclear.
> If I use an #ifdef around this one, I also have
> to add one around the function definition.
and the structs, preferably.
> In that case, I'd
> probably change it back to the x86 check there, and use
>
> #if defined(CONFIG_X86_64) && defined(CONFIG_COMPAT)
> .compat_ioctl = vduse_dev_compat_ioctl,
> #else
> .compat_ioctl = compat_ptr_ioctl.
> #endif
>
> Arnd
