| 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
| ||
|
Message-ID: <f0e493a7-f7bf-4da5-be4f-b41f2a5821dc@redhat.com>
Date: Tue, 17 Oct 2023 16:17:48 +0200
From: Cédric Le Goater <clg@...hat.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] vfio/mtty: Overhaul mtty interrupt handling
On 10/17/23 00:47, Alex Williamson wrote:
> The mtty driver does not currently conform to the vfio SET_IRQS uAPI.
> For example, it claims to support mask and unmask of INTx, but actually
> does nothing. It claims to support AUTOMASK for INTx, but doesn't. It
> fails to teardown eventfds under the full semantics specified by the
> SET_IRQS ioctl. It also fails to teardown eventfds when the device is
> closed, leading to memory leaks. It claims to support the request IRQ,
> but doesn't.
>
> Fix all these.
>
> A side effect of this is that QEMU will now report a warning:
>
> vfio <uuid>: Failed to set up UNMASK eventfd signaling for interrupt \
> INTX-0: VFIO_DEVICE_SET_IRQS failure: Inappropriate ioctl for device
>
> The fact is that the unmask eventfd was never supported but quietly
> failed. mtty never honored the AUTOMASK behavior, therefore there
> was nothing to unmask. QEMU is verbose about the failure, but
> properly falls back to userspace unmasking.
We can add a -ENOTTY test in QEMU for the failures.
Reviewed-by: Cédric Le Goater <clg@...hat.com>
Thanks,
C.
> Fixes: 9d1a546c53b4 ("docs: Sample driver to demonstrate how to use Mediated device framework.")
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
> samples/vfio-mdev/mtty.c | 239 +++++++++++++++++++++++++++------------
> 1 file changed, 166 insertions(+), 73 deletions(-)
>
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index 5af00387c519..245db52bedf2 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -127,7 +127,6 @@ struct serial_port {
> /* State of each mdev device */
> struct mdev_state {
> struct vfio_device vdev;
> - int irq_fd;
> struct eventfd_ctx *intx_evtfd;
> struct eventfd_ctx *msi_evtfd;
> int irq_index;
> @@ -141,6 +140,7 @@ struct mdev_state {
> struct mutex rxtx_lock;
> struct vfio_device_info dev_info;
> int nr_ports;
> + u8 intx_mask:1;
> };
>
> static struct mtty_type {
> @@ -166,10 +166,6 @@ static const struct file_operations vd_fops = {
>
> static const struct vfio_device_ops mtty_dev_ops;
>
> -/* function prototypes */
> -
> -static int mtty_trigger_interrupt(struct mdev_state *mdev_state);
> -
> /* Helper functions */
>
> static void dump_buffer(u8 *buf, uint32_t count)
> @@ -186,6 +182,36 @@ static void dump_buffer(u8 *buf, uint32_t count)
> #endif
> }
>
> +static bool is_intx(struct mdev_state *mdev_state)
> +{
> + return mdev_state->irq_index == VFIO_PCI_INTX_IRQ_INDEX;
> +}
> +
> +static bool is_msi(struct mdev_state *mdev_state)
> +{
> + return mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX;
> +}
> +
> +static bool is_noirq(struct mdev_state *mdev_state)
> +{
> + return !is_intx(mdev_state) && !is_msi(mdev_state);
> +}
> +
> +static void mtty_trigger_interrupt(struct mdev_state *mdev_state)
> +{
> + lockdep_assert_held(&mdev_state->ops_lock);
> +
> + if (is_msi(mdev_state)) {
> + if (mdev_state->msi_evtfd)
> + eventfd_signal(mdev_state->msi_evtfd, 1);
> + } else if (is_intx(mdev_state)) {
> + if (mdev_state->intx_evtfd && !mdev_state->intx_mask) {
> + eventfd_signal(mdev_state->intx_evtfd, 1);
> + mdev_state->intx_mask = true;
> + }
> + }
> +}
> +
> static void mtty_create_config_space(struct mdev_state *mdev_state)
> {
> /* PCI dev ID */
> @@ -921,6 +947,25 @@ static ssize_t mtty_write(struct vfio_device *vdev, const char __user *buf,
> return -EFAULT;
> }
>
> +static void mtty_disable_intx(struct mdev_state *mdev_state)
> +{
> + if (mdev_state->intx_evtfd) {
> + eventfd_ctx_put(mdev_state->intx_evtfd);
> + mdev_state->intx_evtfd = NULL;
> + mdev_state->intx_mask = false;
> + mdev_state->irq_index = -1;
> + }
> +}
> +
> +static void mtty_disable_msi(struct mdev_state *mdev_state)
> +{
> + if (mdev_state->msi_evtfd) {
> + eventfd_ctx_put(mdev_state->msi_evtfd);
> + mdev_state->msi_evtfd = NULL;
> + mdev_state->irq_index = -1;
> + }
> +}
> +
> static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
> unsigned int index, unsigned int start,
> unsigned int count, void *data)
> @@ -932,59 +977,113 @@ static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
> case VFIO_PCI_INTX_IRQ_INDEX:
> switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> case VFIO_IRQ_SET_ACTION_MASK:
> + if (!is_intx(mdev_state) || start != 0 || count != 1) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + mdev_state->intx_mask = true;
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + uint8_t mask = *(uint8_t *)data;
> +
> + if (mask)
> + mdev_state->intx_mask = true;
> + } else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + ret = -ENOTTY; /* No support for mask fd */
> + }
> + break;
> case VFIO_IRQ_SET_ACTION_UNMASK:
> + if (!is_intx(mdev_state) || start != 0 || count != 1) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + mdev_state->intx_mask = false;
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + uint8_t mask = *(uint8_t *)data;
> +
> + if (mask)
> + mdev_state->intx_mask = false;
> + } else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + ret = -ENOTTY; /* No support for unmask fd */
> + }
> break;
> case VFIO_IRQ_SET_ACTION_TRIGGER:
> - {
> - if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - pr_info("%s: disable INTx\n", __func__);
> - if (mdev_state->intx_evtfd)
> - eventfd_ctx_put(mdev_state->intx_evtfd);
> + if (is_intx(mdev_state) && !count &&
> + (flags & VFIO_IRQ_SET_DATA_NONE)) {
> + mtty_disable_intx(mdev_state);
> + break;
> + }
> +
> + if (!(is_intx(mdev_state) || is_noirq(mdev_state)) ||
> + start != 0 || count != 1) {
> + ret = -EINVAL;
> break;
> }
>
> if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> int fd = *(int *)data;
> + struct eventfd_ctx *evt;
> +
> + mtty_disable_intx(mdev_state);
> +
> + if (fd < 0)
> + break;
>
> - if (fd > 0) {
> - struct eventfd_ctx *evt;
> -
> - evt = eventfd_ctx_fdget(fd);
> - if (IS_ERR(evt)) {
> - ret = PTR_ERR(evt);
> - break;
> - }
> - mdev_state->intx_evtfd = evt;
> - mdev_state->irq_fd = fd;
> - mdev_state->irq_index = index;
> + evt = eventfd_ctx_fdget(fd);
> + if (IS_ERR(evt)) {
> + ret = PTR_ERR(evt);
> break;
> }
> + mdev_state->intx_evtfd = evt;
> + mdev_state->irq_index = index;
> + break;
> + }
> +
> + if (!is_intx(mdev_state)) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + mtty_trigger_interrupt(mdev_state);
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + uint8_t trigger = *(uint8_t *)data;
> +
> + if (trigger)
> + mtty_trigger_interrupt(mdev_state);
> }
> break;
> }
> - }
> break;
> case VFIO_PCI_MSI_IRQ_INDEX:
> switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> case VFIO_IRQ_SET_ACTION_MASK:
> case VFIO_IRQ_SET_ACTION_UNMASK:
> + ret = -ENOTTY;
> break;
> case VFIO_IRQ_SET_ACTION_TRIGGER:
> - if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - if (mdev_state->msi_evtfd)
> - eventfd_ctx_put(mdev_state->msi_evtfd);
> - pr_info("%s: disable MSI\n", __func__);
> - mdev_state->irq_index = VFIO_PCI_INTX_IRQ_INDEX;
> + if (is_msi(mdev_state) && !count &&
> + (flags & VFIO_IRQ_SET_DATA_NONE)) {
> + mtty_disable_msi(mdev_state);
> break;
> }
> +
> + if (!(is_msi(mdev_state) || is_noirq(mdev_state)) ||
> + start != 0 || count != 1) {
> + ret = -EINVAL;
> + break;
> + }
> +
> if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> int fd = *(int *)data;
> struct eventfd_ctx *evt;
>
> - if (fd <= 0)
> - break;
> + mtty_disable_msi(mdev_state);
>
> - if (mdev_state->msi_evtfd)
> + if (fd < 0)
> break;
>
> evt = eventfd_ctx_fdget(fd);
> @@ -993,20 +1092,37 @@ static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
> break;
> }
> mdev_state->msi_evtfd = evt;
> - mdev_state->irq_fd = fd;
> mdev_state->irq_index = index;
> + break;
> + }
> +
> + if (!is_msi(mdev_state)) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + mtty_trigger_interrupt(mdev_state);
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + uint8_t trigger = *(uint8_t *)data;
> +
> + if (trigger)
> + mtty_trigger_interrupt(mdev_state);
> }
> break;
> - }
> - break;
> + }
> + break;
> case VFIO_PCI_MSIX_IRQ_INDEX:
> - pr_info("%s: MSIX_IRQ\n", __func__);
> + dev_dbg(mdev_state->vdev.dev, "%s: MSIX_IRQ\n", __func__);
> + ret = -ENOTTY;
> break;
> case VFIO_PCI_ERR_IRQ_INDEX:
> - pr_info("%s: ERR_IRQ\n", __func__);
> + dev_dbg(mdev_state->vdev.dev, "%s: ERR_IRQ\n", __func__);
> + ret = -ENOTTY;
> break;
> case VFIO_PCI_REQ_IRQ_INDEX:
> - pr_info("%s: REQ_IRQ\n", __func__);
> + dev_dbg(mdev_state->vdev.dev, "%s: REQ_IRQ\n", __func__);
> + ret = -ENOTTY;
> break;
> }
>
> @@ -1014,33 +1130,6 @@ static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
> return ret;
> }
>
> -static int mtty_trigger_interrupt(struct mdev_state *mdev_state)
> -{
> - int ret = -1;
> -
> - if ((mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX) &&
> - (!mdev_state->msi_evtfd))
> - return -EINVAL;
> - else if ((mdev_state->irq_index == VFIO_PCI_INTX_IRQ_INDEX) &&
> - (!mdev_state->intx_evtfd)) {
> - pr_info("%s: Intr eventfd not found\n", __func__);
> - return -EINVAL;
> - }
> -
> - if (mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX)
> - ret = eventfd_signal(mdev_state->msi_evtfd, 1);
> - else
> - ret = eventfd_signal(mdev_state->intx_evtfd, 1);
> -
> -#if defined(DEBUG_INTR)
> - pr_info("Intx triggered\n");
> -#endif
> - if (ret != 1)
> - pr_err("%s: eventfd signal failed (%d)\n", __func__, ret);
> -
> - return ret;
> -}
> -
> static int mtty_get_region_info(struct mdev_state *mdev_state,
> struct vfio_region_info *region_info,
> u16 *cap_type_id, void **cap_type)
> @@ -1084,22 +1173,16 @@ static int mtty_get_region_info(struct mdev_state *mdev_state,
>
> static int mtty_get_irq_info(struct vfio_irq_info *irq_info)
> {
> - switch (irq_info->index) {
> - case VFIO_PCI_INTX_IRQ_INDEX:
> - case VFIO_PCI_MSI_IRQ_INDEX:
> - case VFIO_PCI_REQ_IRQ_INDEX:
> - break;
> -
> - default:
> + if (irq_info->index != VFIO_PCI_INTX_IRQ_INDEX &&
> + irq_info->index != VFIO_PCI_MSI_IRQ_INDEX)
> return -EINVAL;
> - }
>
> irq_info->flags = VFIO_IRQ_INFO_EVENTFD;
> irq_info->count = 1;
>
> if (irq_info->index == VFIO_PCI_INTX_IRQ_INDEX)
> - irq_info->flags |= (VFIO_IRQ_INFO_MASKABLE |
> - VFIO_IRQ_INFO_AUTOMASKED);
> + irq_info->flags |= VFIO_IRQ_INFO_MASKABLE |
> + VFIO_IRQ_INFO_AUTOMASKED;
> else
> irq_info->flags |= VFIO_IRQ_INFO_NORESIZE;
>
> @@ -1262,6 +1345,15 @@ static unsigned int mtty_get_available(struct mdev_type *mtype)
> return atomic_read(&mdev_avail_ports) / type->nr_ports;
> }
>
> +static void mtty_close(struct vfio_device *vdev)
> +{
> + struct mdev_state *mdev_state =
> + container_of(vdev, struct mdev_state, vdev);
> +
> + mtty_disable_intx(mdev_state);
> + mtty_disable_msi(mdev_state);
> +}
> +
> static const struct vfio_device_ops mtty_dev_ops = {
> .name = "vfio-mtty",
> .init = mtty_init_dev,
> @@ -1273,6 +1365,7 @@ static const struct vfio_device_ops mtty_dev_ops = {
> .unbind_iommufd = vfio_iommufd_emulated_unbind,
> .attach_ioas = vfio_iommufd_emulated_attach_ioas,
> .detach_ioas = vfio_iommufd_emulated_detach_ioas,
> + .close_device = mtty_close,
> };
>
> static struct mdev_driver mtty_driver = {
Powered by blists - more mailing lists