Message-ID: <20151001133114-mutt-send-email-mst@redhat.com>
Date:	Thu, 1 Oct 2015 13:37:12 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Stephen Hemminger <stephen@...workplumber.org>
Cc:	hjk@...sjkoch.de, gregkh@...ux-foundation.org,
	linux-kernel@...r.kernel.org, dev@...k.org
Subject: Re: [PATCH 2/2] uio: new driver to support PCI MSI-X

On Thu, Oct 01, 2015 at 11:33:06AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 30, 2015 at 03:28:58PM -0700, Stephen Hemminger wrote:
> > This driver allows using PCI device with Message Signalled Interrupt
> > from userspace. The API is similar to the igb_uio driver used by the DPDK.
> > Via ioctl it provides a mechanism to map MSI-X interrupts into event
> > file descriptors similar to VFIO.
> >
> > VFIO is a better choice if IOMMU is available, but often userspace drivers
> > have to work in environments where IOMMU support (real or emulated) is
> > not available.  All UIO drivers that support DMA are not secure against
> > rogue userspace applications programming DMA hardware to access
> > private memory; this driver is no less secure than existing code.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
> 
> I don't think copying the igb_uio interface is a good idea.
> What DPDK is doing with igb_uio (and indeed uio_pci_generic)
> is abusing the sysfs BAR access to provide unlimited
> access to hardware.
> 
> MSI messages are memory writes so any generic device capable
> of MSI is capable of corrupting kernel memory.
> This means that a bug in userspace will lead to kernel memory corruption
> and crashes.  This is something distributions can't support.
> 
> uio_pci_generic is already abused like that, mostly
> because when I wrote it, I didn't add enough protections
> against using it with DMA capable devices,
> and we can't go back and break working userspace.
> But at least it does not bind to VFs which all of
> them are capable of DMA.
> 
> The result of merging this driver will be userspace abusing the
> sysfs BAR access with VFs as well, and we do not want that.
> 
> 
> Just forwarding events is not enough to make a valid driver.
> What is missing is a way to access the device in a safe way.
> 
> On a more positive note:
> 
> What would be a reasonable interface? One that does the following
> in kernel:
> 
> 1. initializes device rings (can be in pinned userspace memory,
>    but can not be writeable by userspace), brings up interface link
> 2. pins userspace memory (unless using e.g. hugetlbfs)
> 3. gets request, make sure it's valid and belongs to
>    the correct task, put it in the ring
> 4. in the reverse direction, notify userspace when buffers
>    are available in the ring
> 5. notify userspace about MSI (what this driver does)
> 
> What userspace can be allowed to do:
> 
> 	format requests (e.g. transmit, receive) in userspace
> 	read ring contents
> 
> What userspace can't be allowed to do:
> 
> 	access BAR
> 	write rings

Thinking about it some more, many devices


Have separate rings for DMA: TX (device reads memory)
and RX (device writes memory).
With such devices, a mode where userspace can write TX ring
but not RX ring might make sense.

This will mean userspace might read kernel memory
through the device, but can not corrupt it.

That's already a big win!

And RX buffers do not have to be added one at a time.
If we assume 0.2usec per system call, batching some 100 buffers per
system call gives you 2 nano seconds overhead.  That seems quite
reasonable.





> 
> This means that the driver can not be a generic one,
> and there will be a system call overhead when you
> write the ring, but that's the price you have to
> pay for ability to run on systems without an IOMMU.
> 
> 
> 
> 
> > ---
> >  drivers/uio/Kconfig          |   9 ++
> >  drivers/uio/Makefile         |   1 +
> >  drivers/uio/uio_msi.c        | 378 +++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/Kbuild    |   1 +
> >  include/uapi/linux/uio_msi.h |  22 +++
> >  5 files changed, 411 insertions(+)
> >  create mode 100644 drivers/uio/uio_msi.c
> >  create mode 100644 include/uapi/linux/uio_msi.h
> > 
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index 52c98ce..04adfa0 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -93,6 +93,15 @@ config UIO_PCI_GENERIC
> >  	  primarily, for virtualization scenarios.
> >  	  If you compile this as a module, it will be called uio_pci_generic.
> >  
> > +config UIO_PCI_MSI
> > +       tristate "Generic driver supporting MSI-x on PCI Express cards"
> > +	depends on PCI
> > +	help
> > +	  Generic driver that provides Message Signalled IRQ events
> > +	  similar to VFIO. If IOMMMU is available please use VFIO
> > +	  instead since it provides more security.
> > +	  If you compile this as a module, it will be called uio_msi.
> > +
> >  config UIO_NETX
> >  	tristate "Hilscher NetX Card driver"
> >  	depends on PCI
> > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > index 8560dad..62fc44b 100644
> > --- a/drivers/uio/Makefile
> > +++ b/drivers/uio/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> >  obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> >  obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
> >  obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
> > +obj-$(CONFIG_UIO_PCI_MSI)	+= uio_msi.o
> > diff --git a/drivers/uio/uio_msi.c b/drivers/uio/uio_msi.c
> > new file mode 100644
> > index 0000000..802b5c4
> > --- /dev/null
> > +++ b/drivers/uio/uio_msi.c
> > @@ -0,0 +1,378 @@
> > +/*-
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * Author: Stephen Hemminger <stephen@...workplumber.org>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 only.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/msi.h>
> > +#include <linux/uio_msi.h>
> > +
> > +#define DRIVER_VERSION		"0.1.1"
> > +#define MAX_MSIX_VECTORS	64
> > +
> > +/* MSI-X vector information */
> > +struct uio_msi_pci_dev {
> > +	struct uio_info info;		/* UIO driver info */
> > +	struct pci_dev *pdev;		/* PCI device */
> > +	struct mutex	mutex;		/* open/release/ioctl mutex */
> > +	int		ref_cnt;	/* references to device */
> > +	unsigned int	max_vectors;	/* MSI-X slots available */
> > +	struct msix_entry *msix;	/* MSI-X vector table */
> > +	struct uio_msi_irq_ctx {
> > +		struct eventfd_ctx *trigger; /* vector to eventfd */
> > +		char *name;		/* name in /proc/interrupts */
> > +	} *ctx;
> > +};
> > +
> > +static irqreturn_t uio_intx_irqhandler(int irq, void *arg)
> > +{
> > +	struct uio_msi_pci_dev *udev = arg;
> > +
> > +	if (pci_check_and_mask_intx(udev->pdev)) {
> > +		eventfd_signal(udev->ctx->trigger, 1);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t uio_msi_irqhandler(int irq, void *arg)
> > +{
> > +	struct eventfd_ctx *trigger = arg;
> > +
> > +	eventfd_signal(trigger, 1);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/* set the mapping between vector # and existing eventfd. */
> > +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> > +{
> > +	struct eventfd_ctx *trigger;
> > +	int irq, err;
> > +
> > +	if (vec >= udev->max_vectors) {
> > +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> > +			   vec, udev->max_vectors);
> > +		return -ERANGE;
> > +	}
> > +
> > +	irq = udev->msix[vec].vector;
> > +	trigger = udev->ctx[vec].trigger;
> > +	if (trigger) {
> > +		/* Clearup existing irq mapping */
> > +		free_irq(irq, trigger);
> > +		eventfd_ctx_put(trigger);
> > +		udev->ctx[vec].trigger = NULL;
> > +	}
> > +
> > +	/* Passing -1 is used to disable interrupt */
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +	trigger = eventfd_ctx_fdget(fd);
> > +	if (IS_ERR(trigger)) {
> > +		err = PTR_ERR(trigger);
> > +		dev_notice(&udev->pdev->dev,
> > +			   "eventfd ctx get failed: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (udev->msix)
> > +		err = request_irq(irq, uio_msi_irqhandler, 0,
> > +				  udev->ctx[vec].name, trigger);
> > +	else
> > +		err = request_irq(irq, uio_intx_irqhandler, IRQF_SHARED,
> > +				  udev->ctx[vec].name, udev);
> > +
> > +	if (err) {
> > +		dev_notice(&udev->pdev->dev,
> > +			   "request irq failed: %d\n", err);
> > +		eventfd_ctx_put(trigger);
> > +		return err;
> > +	}
> > +
> > +	udev->ctx[vec].trigger = trigger;
> > +	return 0;
> > +}
> > +
> > +static int
> > +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	struct uio_msi_irq_set hdr;
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case UIO_MSI_IRQ_SET:
> > +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > +			return -EFAULT;
> > +
> > +		mutex_lock(&udev->mutex);
> > +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> > +		mutex_unlock(&udev->mutex);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +	}
> > +	return err;
> > +}
> > +
> > +/* Opening the UIO device for first time enables MSI-X */
> > +static int
> > +uio_msi_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int err = 0;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (udev->ref_cnt++ == 0) {
> > +		if (udev->msix)
> > +			err = pci_enable_msix(udev->pdev, udev->msix,
> > +					      udev->max_vectors);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return err;
> > +}
> > +
> > +/* Last close of the UIO device releases/disables all IRQ's */
> > +static int
> > +uio_msi_release(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	mutex_lock(&udev->mutex);
> > +	if (--udev->ref_cnt == 0) {
> > +		for (i = 0; i < udev->max_vectors; i++) {
> > +			int irq = udev->msix[i].vector;
> > +			struct eventfd_ctx *trigger = udev->ctx[i].trigger;
> > +
> > +			if (!trigger)
> > +				continue;
> > +
> > +			free_irq(irq, trigger);
> > +			eventfd_ctx_put(trigger);
> > +			udev->ctx[i].trigger = NULL;
> > +		}
> > +
> > +		if (udev->msix)
> > +			pci_disable_msix(udev->pdev);
> > +	}
> > +	mutex_unlock(&udev->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Unmap previously ioremap'd resources */
> > +static void
> > +release_iomaps(struct uio_mem *mem)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> > +		if (mem->internal_addr)
> > +			iounmap(mem->internal_addr);
> > +	}
> > +}
> > +
> > +static int
> > +setup_maps(struct pci_dev *pdev, struct uio_info *info)
> > +{
> > +	int i, m = 0, p = 0, err;
> > +	static const char * const bar_names[] = {
> > +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
> > +	};
> > +
> > +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
> > +		unsigned long start = pci_resource_start(pdev, i);
> > +		unsigned long flags = pci_resource_flags(pdev, i);
> > +		unsigned long len = pci_resource_len(pdev, i);
> > +
> > +		if (start == 0 || len == 0)
> > +			continue;
> > +
> > +		if (flags & IORESOURCE_MEM) {
> > +			void __iomem *addr;
> > +
> > +			if (m >= MAX_UIO_MAPS)
> > +				continue;
> > +
> > +			addr = ioremap(start, len);
> > +			if (addr == NULL) {
> > +				err = -EINVAL;
> > +				goto fail;
> > +			}
> > +
> > +			info->mem[m].name = bar_names[i];
> > +			info->mem[m].addr = start;
> > +			info->mem[m].internal_addr = addr;
> > +			info->mem[m].size = len;
> > +			info->mem[m].memtype = UIO_MEM_PHYS;
> > +			++m;
> > +		} else if (flags & IORESOURCE_IO) {
> > +			if (p >= MAX_UIO_PORT_REGIONS)
> > +				continue;
> > +
> > +			info->port[p].name = bar_names[i];
> > +			info->port[p].start = start;
> > +			info->port[p].size = len;
> > +			info->port[p].porttype = UIO_PORT_X86;
> > +			++p;
> > +		}
> > +	}
> > +
> > +	return 0;
> > + fail:
> > +	for (i = 0; i < m; i++)
> > +		iounmap(info->mem[i].internal_addr);
> > +	return err;
> > +}
> > +
> > +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct uio_msi_pci_dev *udev;
> > +	int i, err, vectors;
> > +
> > +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> > +	if (!udev)
> > +		return -ENOMEM;
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> > +		goto fail_free;
> > +	}
> > +
> > +	err = pci_request_regions(pdev, "uio_msi");
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "Cannot request regions\n");
> > +		goto fail_disable;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	/* remap resources */
> > +	err = setup_maps(pdev, &udev->info);
> > +	if (err)
> > +		goto fail_release_iomem;
> > +
> > +	/* fill uio infos */
> > +	udev->info.name = "uio_msi";
> > +	udev->info.version = DRIVER_VERSION;
> > +	udev->info.priv = udev;
> > +	udev->pdev = pdev;
> > +	udev->info.ioctl = uio_msi_ioctl;
> > +	udev->info.open = uio_msi_open;
> > +	udev->info.release = uio_msi_release;
> > +	udev->info.irq = UIO_IRQ_CUSTOM;
> > +	mutex_init(&udev->mutex);
> > +
> > +	vectors = pci_msix_vec_count(pdev);
> > +	if (vectors > 0) {
> > +		udev->max_vectors = min_t(u16, vectors, MAX_MSIX_VECTORS);
> > +		dev_info(&pdev->dev, "using up to %u MSI-X vectors\n",
> > +			udev->max_vectors);
> > +
> > +		err = -ENOMEM;
> > +		udev->msix = kcalloc(udev->max_vectors,
> > +				     sizeof(struct msix_entry), GFP_KERNEL);
> > +		if (!udev->msix)
> > +			goto fail_release_iomem;
> > +	} else if (!pci_intx_mask_supported(pdev)) {
> > +		dev_err(&pdev->dev,
> > +			"device does not support MSI-X or INTX\n");
> > +		err = -EINVAL;
> > +		goto fail_release_iomem;
> > +	} else {
> > +		dev_notice(&pdev->dev, "using INTX\n");
> > +		udev->info.irq_flags = IRQF_SHARED;
> > +		udev->max_vectors = 1;
> > +	}
> > +
> > +	udev->ctx = kcalloc(udev->max_vectors,
> > +			    sizeof(struct uio_msi_irq_ctx), GFP_KERNEL);
> > +	if (!udev->ctx)
> > +		goto fail_free_msix;
> > +
> > +	for (i = 0; i < udev->max_vectors; i++) {
> > +		udev->msix[i].entry = i;
> > +
> > +		udev->ctx[i].name = kasprintf(GFP_KERNEL,
> > +					      KBUILD_MODNAME "[%d](%s)",
> > +					      i, pci_name(pdev));
> > +		if (!udev->ctx[i].name)
> > +			goto fail_free_ctx;
> > +	}
> > +
> > +	/* register uio driver */
> > +	err = uio_register_device(&pdev->dev, &udev->info);
> > +	if (err != 0)
> > +		goto fail_free_ctx;
> > +
> > +	pci_set_drvdata(pdev, udev);
> > +	return 0;
> > +
> > +fail_free_ctx:
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +fail_free_msix:
> > +	kfree(udev->msix);
> > +fail_release_iomem:
> > +	release_iomaps(udev->info.mem);
> > +	pci_release_regions(pdev);
> > +fail_disable:
> > +	pci_disable_device(pdev);
> > +fail_free:
> > +	kfree(udev);
> > +
> > +	pr_notice("%s ret %d\n", __func__, err);
> > +	return err;
> > +}
> > +
> > +static void uio_msi_remove(struct pci_dev *pdev)
> > +{
> > +	struct uio_info *info = pci_get_drvdata(pdev);
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	int i;
> > +
> > +	uio_unregister_device(info);
> > +	release_iomaps(info->mem);
> > +
> > +	pci_release_regions(pdev);
> > +	for (i = 0; i < udev->max_vectors; i++)
> > +		kfree(udev->ctx[i].name);
> > +	kfree(udev->ctx);
> > +	kfree(udev->msix);
> > +	pci_disable_device(pdev);
> > +
> > +	pci_set_drvdata(pdev, NULL);
> > +	kfree(udev);
> > +}
> > +
> > +static struct pci_driver uio_msi_pci_driver = {
> > +	.name = "uio_msi",
> > +	.probe = uio_msi_probe,
> > +	.remove = uio_msi_remove,
> > +};
> > +
> > +module_pci_driver(uio_msi_pci_driver);
> > +MODULE_VERSION(DRIVER_VERSION);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Stephen Hemminger <stephen@...workplumber.org>");
> > +MODULE_DESCRIPTION("UIO driver for MSI PCI devices");
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index f7b2db4..d9497691 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -411,6 +411,7 @@ header-y += udp.h
> >  header-y += uhid.h
> >  header-y += uinput.h
> >  header-y += uio.h
> > +header-y += uio_msi.h
> >  header-y += ultrasound.h
> >  header-y += un.h
> >  header-y += unistd.h
> > diff --git a/include/uapi/linux/uio_msi.h b/include/uapi/linux/uio_msi.h
> > new file mode 100644
> > index 0000000..297de00
> > --- /dev/null
> > +++ b/include/uapi/linux/uio_msi.h
> > @@ -0,0 +1,22 @@
> > +/*
> > + * UIO_MSI API definition
> > + *
> > + * Copyright (c) 2015 by Brocade Communications Systems, Inc.
> > + * All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef _UIO_PCI_MSI_H
> > +#define _UIO_PCI_MSI_H
> > +
> > +struct uio_msi_irq_set {
> > +	u32 vec;
> > +	int fd;
> > +};
> > +
> > +#define UIO_MSI_BASE	0x86
> > +#define UIO_MSI_IRQ_SET	_IOW('I', UIO_MSI_BASE+1, struct uio_msi_irq_set)
> > +
> > +#endif
> > -- 
> > 2.1.4
> > 
> > --
> > 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/
--
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/

Powered by blists - more mailing lists