linux-kernel - Re: [REVIEW] NVM Express driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <201103131924.25032.arnd@arndb.de>
Date:	Sun, 13 Mar 2011 19:24:25 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Matthew Wilcox <willy@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [REVIEW] NVM Express driver

On Thursday 03 March 2011 21:47:49 Matthew Wilcox wrote:

> You may have seen the recent announcement of the NVM Express spec;
> here's the driver.  I've put a git tree up on kernel.org with
> all the history that's fit to be released (earlier versions
> were full of Intel codenames and are not suited to public
> consumption).  If you'd rather review the history, it's at
> http://git.kernel.org/?p=linux/kernel/git/willy/nvme.git;a=summary

Looks really nice overall, I only have a few comments about
the ioctl interface. As Greg already commented, it would be
good to reduce the number of ioctl commands to a minimum.

> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
> +                                                       unsigned long arg)
> +{
> +       struct nvme_ns *ns = bdev->bd_disk->private_data;
> +
> +       switch (cmd) {
> +       case NVME_IOCTL_IDENTIFY_NS:
> +               return nvme_identify(ns, arg, 0);
> +       case NVME_IOCTL_IDENTIFY_CTRL:
> +               return nvme_identify(ns, arg, 1);
> +       case NVME_IOCTL_GET_RANGE_TYPE:
> +               return nvme_get_range_type(ns, arg);
> +       case NVME_IOCTL_SUBMIT_IO:
> +               return nvme_submit_io(ns, (void __user *)arg);
> +       case NVME_IOCTL_DOWNLOAD_FW:
> +               return nvme_download_firmware(ns, (void __user *)arg);
> +       case NVME_IOCTL_ACTIVATE_FW:
> +               return nvme_activate_firmware(ns, arg);
> +       default:
> +               return -ENOTTY;
> +       }
> +}

The argument is a pointer for all of these, as far as I can tell,
so you could do the typecast here once instead of doing it for
two of them.

> +static const struct block_device_operations nvme_fops = {
> +       .owner          = THIS_MODULE,
> +       .ioctl          = nvme_ioctl,
> +};

This is missing a .compat_ioctl pointer to work with 32 bit user
space. The data structures all seem compatible, so that should
be a trivial change.

> +#define NVME_IOCTL_IDENTIFY_NS _IOW('N', 0x40, struct nvme_id_ns)
> +#define NVME_IOCTL_IDENTIFY_CTRL _IOW('N', 0x41, struct nvme_id_ctrl)
> +#define NVME_IOCTL_GET_RANGE_TYPE _IOW('N', 0x42, struct nvme_lba_range_type)
> +#define NVME_IOCTL_SUBMIT_IO   _IOWR('N', 0x43, struct nvme_rw_command)
> +#define NVME_IOCTL_DOWNLOAD_FW _IOR('N', 0x44, struct nvme_dlfw)
> +#define NVME_IOCTL_ACTIVATE_FW _IO('N', 0x45)

I believe you have confused read and write here. IIRC, _IOW means
copy_from_user and _IOR means copy_to_user.

The code for NVME_IOCTL_SUBMIT_IO takes a nvme_user_io argument,
not nvme_rw_command.

You might want to have an upper bound on the amount of user data
that one can pass to SUBMIT_IO and others.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/