On Fri, Mar 26, 2021 at 12:19:42AM +0900, Keith Busch wrote: > On Thu, Mar 25, 2021 at 09:48:37AM +0000, Niklas Cassel wrote: > > From: Niklas Cassel <[email protected]> > > > > When a passthru command targets a specific namespace, the ns parameter to > > nvme_user_cmd()/nvme_user_cmd64() is set. However, there is currently no > > validation that the nsid specified in the passthru command targets the > > namespace/nsid represented by the block device that the ioctl was > > performed on. > > > > Add a check that validates that the nsid in the passthru command matches > > that of the supplied namespace. > > > > Signed-off-by: Niklas Cassel <[email protected]> > > --- > > Currently, if doing NVME_IOCTL_IO_CMD on the controller char device, > > if and only if there is a single namespace in the ctrl->namespaces list, > > nvme_dev_user_cmd() will call nvme_user_cmd() with ns parameter set. > > While it might be good that we validate the nsid even in this case, > > perhaps we want to allow a nsid value in the passthru command to be > > 0x0 and/or the NSID broadcast value? (Only when NVME_IOCTL_IO_CMD was > > done on the controller char device though.) > > There are no IO commands accepting a 0 NSID, so rejecting those from the > driver should be okay. > > FLUSH is the only IO command that takes a broadcast NSID. I suspect at > least some entities were unfortunately sending broadcast flush through > this interface, so it's possible we'll hear of breakage, but I'd agree > your patch is still the right thing to do.
I don't think this should be a problem. You shouldn't be sending a broadcast NSID via the per namespace block device. It just seems silly to specify a specific namespace block device, and then use the broadcast NSID. (This obviously should never have been allowed.) If you wanted to send a broadcast NSID, you should have used the controller character device. However, nvme_dev_user_cmd() currently rejects any NVME_IOCTL_IO_CMD when there is more than one namespace in ctrl->namespaces list, so they could never have used the controller character device to send a flush to more than one namespace. So here we don't change anything. Kind regards, Niklas
