Re: nvme_passthru_cmd64 has a 4 byte hole

[Charles Machalow <csm10495@gmail.com>]

I think something like this might do it:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fa7ba09dc..80b17730b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1453,11 +1453,11 @@ static int nvme_user_cmd64(struct nvme_ctrl
*ctrl, struct nvme_ns *ns,
        status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
                        (void __user *)(uintptr_t)cmd.addr, cmd.data_len,
                        (void __user *)(uintptr_t)cmd.metadata,
cmd.metadata_len,
-                       0, &cmd.result, timeout);
+                       0, (u64*)&cmd.result, timeout);
        nvme_passthru_end(ctrl, effects);

        if (status >= 0) {
-               if (put_user(cmd.result, &ucmd->result))
+               if (put_user(*(u64*)&cmd.result, (u64*)&ucmd->result))
                        return -EFAULT;
        }

diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index e168dc59e..4cb07bd6d 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -63,7 +63,7 @@ struct nvme_passthru_cmd64 {
        __u32   cdw14;
        __u32   cdw15;
        __u32   timeout_ms;
-       __u64   result;
+       __u32   result[2];
 };

Need to do a couple manual checks to make sure the old/new ioclts both
work. If they do, does this seem like a valid patch?

- Charlie Scott Machalow


On Wed, Oct 30, 2019 at 7:08 AM Charles Machalow <csm10495@gmail.com> wrote:
>
> Ah yes. I'd greatly prefer that. That way the new structure can likely
> even be used with the old Ioctls and the 2nd u32 should just stay not
> get set if used in the old flow
>
> - Charlie Scott Machalow
>
> On Tue, Oct 29, 2019 at 11:32 PM Keith Busch <kbusch@kernel.org> wrote:
> >
> > On Tue, Oct 29, 2019 at 09:43:20PM -0700, Charles Machalow wrote:
> > > Hey all,
> > >
> > > I noticed (via some debug) that nvme_passthru_cmd64 seems to have a 4
> > > byte hole that isn't currently accounted for in the struct:
> > > From pahole:
> > >
> > > struct nvme_passthru_cmd64 {
> > >         __u8                       opcode;               /*     0     1 */
> > >         __u8                       flags;                /*     1     1 */
> > >         __u16                      rsvd1;                /*     2     2 */
> > >         __u32                      nsid;                 /*     4     4 */
> > >         __u32                      cdw2;                 /*     8     4 */
> > >         __u32                      cdw3;                 /*    12     4 */
> > >         __u64                      metadata;             /*    16     8 */
> > >         __u64                      addr;                 /*    24     8 */
> > >         __u32                      metadata_len;         /*    32     4 */
> > >         __u32                      data_len;             /*    36     4 */
> > >         __u32                      cdw10;                /*    40     4 */
> > >         __u32                      cdw11;                /*    44     4 */
> > >         __u32                      cdw12;                /*    48     4 */
> > >         __u32                      cdw13;                /*    52     4 */
> > >         __u32                      cdw14;                /*    56     4 */
> > >         __u32                      cdw15;                /*    60     4 */
> > >         /* --- cacheline 1 boundary (64 bytes) --- */
> > >         __u32                      timeout_ms;           /*    64     4 */
> > >
> > >         /* XXX 4 bytes hole, try to pack */
> > >
> > >         __u64                      result;               /*    72     8 */
> > >
> > >         /* size: 80, cachelines: 2, members: 18 */
> > >         /* sum members: 76, holes: 1, sum holes: 4 */
> > >         /* last cacheline: 16 bytes */
> > > };
> > >
> > > (Since I doubt we can functionally change the structure at this
> > > point...)
> >
> > Well, there is no official kernel release using this structure,
> > so I suppose even this late in the rc cycle, we can change it with
> > appropriate justification. I think an alternative to padding is to make
> > this a __u32[2] field, which should also be backward compatible with
> > the 4 byte result passthrough.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme