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.
>> + * 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. If I use an #ifdef around this one, I also have
to add one around the function definition. 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
