Re: [PATCH 1/2] vduse: avoid adding implicit padding

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Feb 2, 2026 at 1:07 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Feb 2, 2026, at 12:50, Eugenio Perez Martin wrote:
> > On Mon, Feb 2, 2026 at 12:28 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >> >                 ret = -EFAULT;
> >> > -               if (cmd == VDUSE_IOTLB_GET_FD2) {
> >> > -                       if (copy_from_user(&entry, argp, sizeof(entry)))
> >> > -                               break;
> >> > -               } else {
> >> > -                       if (copy_from_user(&entry.v1, argp,
> >> > -                                          sizeof(entry.v1)))
> >> > -                               break;
> >> > -               }
> >> > +               if (copy_from_user(&entry, argp, _IOC_SIZE(cmd)))
> >>
> >> I did not know about _IOC_SIZE and I like how it reduces the complexity, thanks!
> >>
> >> As a proposal, maybe we can add MIN(_IOC_SIZE, sizeof(entry)) ? Not
> >> sure if it is too much boilerplate for nothing as the compiler should
> >> make the code identical and the uapi ioctl part should never change.
> >> But it seems to me future changes to the code are better tied with the
> >> MIN.
> >> I'm ok with not including MIN() either way.
>
> I think it's more readable without the MIN(), but I don't mind
> adding it either.
>

Yep, I see how MIN() adds a little bit of noise. I'm happy either way :).

> >> >   */
> >> >  struct vduse_iotlb_entry_v2 {
> >> > -       struct vduse_iotlb_entry v1;
> >> > +       __u64 offset;
> >> > +       __u64 start;
> >> > +       __u64 last;
> >> > +       __u8 perm;
> >> > +       __u8 padding[7];
> >> >         __u32 asid;
> >> > -       __u32 reserved[12];
> >> > +       __u32 reserved[11];
> >
> > (I hit "Send" too early).
> >
> > We could make this padding[3] so reserved keeps being [12]. This way
> > the struct members keep the same alignment between the commits. Not
> > super important as there should not be a lot of users of this right
> > now, we're just introducing it.
>
> I think that is too risky, as it would overlay 'asid' on top of
> previously uninitialized padding fields coming from userspace
> on most architectures. Since there was previously no is_mem_zero()
> check for the padding, I don't think it should be reused at all.
>

Ok fair point. Yes, this would need something in the code like:

if (cmd == VDUSE_IOTLB_GET_FD)
    /* Ignoring whatever came in padding as it could be uninitialized due to
     * not having this member in the struct definition
     */
    memset(entry.padding, 0, sizeof(entry.padding);
else if (!mem_is_zero(entry.padding, sizeof(entry.padding,
sizeof(entry.padding))
    /* Return error following the style of the rest of the code */
    return -EINVAL
---

So we make sure we can use the padding in the future. Would that work?