On Fri, 31 Mar 2023 10:02:32 +0000
"Liu, Jing2" <jing2.liu@xxxxxxxxx> wrote:
> Hi Reinette,
>
> > @@ -409,33 +416,62 @@ static int vfio_msi_set_vector_signal(struct
> > vfio_pci_core_device *vdev, {
> > struct pci_dev *pdev = vdev->pdev;
> > struct vfio_pci_irq_ctx *ctx;
> > + struct msi_map msix_map = {};
> > + bool allow_dyn_alloc = false;
> > struct eventfd_ctx *trigger;
> > + bool new_ctx = false;
> > int irq, ret;
> > u16 cmd;
> >
> > + /* Only MSI-X allows dynamic allocation. */
> > + if (msix && pci_msix_can_alloc_dyn(vdev->pdev))
> > + allow_dyn_alloc = true;
> > +
> > ctx = vfio_irq_ctx_get(vdev, vector);
> > - if (!ctx)
> > + if (!ctx && !allow_dyn_alloc)
> > return -EINVAL;
> > +
> > irq = pci_irq_vector(pdev, vector);
> > + /* Context and interrupt are always allocated together. */
> > + WARN_ON((ctx && irq == -EINVAL) || (!ctx && irq != -EINVAL));
> >
> > - if (ctx->trigger) {
> > + if (ctx && ctx->trigger) {
> > irq_bypass_unregister_producer(&ctx->producer);
> >
> > cmd = vfio_pci_memory_lock_and_enable(vdev);
> > free_irq(irq, ctx->trigger);
> > + if (allow_dyn_alloc) {
> > + msix_map.index = vector;
> > + msix_map.virq = irq;
> > + pci_msix_free_irq(pdev, msix_map);
> > + irq = -EINVAL;
> > + }
> > vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > kfree(ctx->name);
> > eventfd_ctx_put(ctx->trigger);
> > ctx->trigger = NULL;
> > + if (allow_dyn_alloc) {
> > + vfio_irq_ctx_free(vdev, ctx, vector);
> > + ctx = NULL;
> > + }
> > }
> >
> > if (fd < 0)
> > return 0;
> >
>
> While looking at this piece of code, one thought comes to me:
> It might be possible that userspace comes twice with the same valid fd for a specific
> vector, this vfio code will free the resource in if(ctx && ctx->trigger) for the second
> time, and then re-alloc again for the same fd given by userspace.
>
> Would that help if we firstly check e.g. ctx->trigger with the given valid fd, to see if
> the trigger is really changed or not?
It's rather a made-up situation, if userspace wants to avoid bouncing
the vector when the eventfd hasn't changed they can simply test this
before calling the ioctl. Thanks,
Alex
