On Tue, 6 Feb 2024 13:45:22 -0800
Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> Hi Alex,
>
> On 2/5/2024 2:35 PM, Alex Williamson wrote:
> > On Thu, 1 Feb 2024 20:57:01 -0800
> > Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
>
> ..
>
> >> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> >> index 31f73c70fcd2..7ca2b983b66e 100644
> >> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> >> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> >> @@ -427,7 +427,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >>
> >> ctx = vfio_irq_ctx_get(vdev, vector);
> >>
> >> - if (ctx) {
> >> + if (ctx && ctx->trigger) {
> >> irq_bypass_unregister_producer(&ctx->producer);
> >> irq = pci_irq_vector(pdev, vector);
> >> cmd = vfio_pci_memory_lock_and_enable(vdev);
> >> @@ -435,8 +435,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >> vfio_pci_memory_unlock_and_restore(vdev, cmd);
> >> /* Interrupt stays allocated, will be freed at MSI-X disable. */
> >> kfree(ctx->name);
> >> + ctx->name = NULL;
> >
> > Setting ctx->name = NULL is not strictly necessary and does not match
> > the INTx code that we're claiming to try to emulate. ctx->name is only
> > tested immediately after allocation below, otherwise it can be inferred
> > from ctx->trigger. Thanks,
>
> This all matches my understanding. I added ctx->name = NULL after every kfree(ctx->name)
> (see below for confirmation of other instance). You are correct that the flow
> infers validity of ctx->name from ctx->trigger. My motivation for
> adding ctx->name = NULL is that, since the interrupt context persists, this
> change ensures that there will be no pointer that points to freed memory. I
> am not comfortable leaving pointers to freed memory around.
Fair enough. Maybe note the change in the commit log. Thanks,
Alex
> >> eventfd_ctx_put(ctx->trigger);
> >> - vfio_irq_ctx_free(vdev, ctx, vector);
> >> + ctx->trigger = NULL;
> >> }
> >>
> >> if (fd < 0)
> >> @@ -449,16 +450,17 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >> return irq;
> >> }
> >>
> >> - ctx = vfio_irq_ctx_alloc(vdev, vector);
> >> - if (!ctx)
> >> - return -ENOMEM;
> >> + /* Per-interrupt context remain allocated. */
> >> + if (!ctx) {
> >> + ctx = vfio_irq_ctx_alloc(vdev, vector);
> >> + if (!ctx)
> >> + return -ENOMEM;
> >> + }
> >>
> >> ctx->name = kasprintf(GFP_KERNEL_ACCOUNT, "vfio-msi%s[%d](%s)",
> >> msix ? "x" : "", vector, pci_name(pdev));
> >> - if (!ctx->name) {
> >> - ret = -ENOMEM;
> >> - goto out_free_ctx;
> >> - }
> >> + if (!ctx->name)
> >> + return -ENOMEM;
> >>
> >> trigger = eventfd_ctx_fdget(fd);
> >> if (IS_ERR(trigger)) {
> >> @@ -502,8 +504,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >> eventfd_ctx_put(trigger);
> >> out_free_name:
> >> kfree(ctx->name);
> >> -out_free_ctx:
> >> - vfio_irq_ctx_free(vdev, ctx, vector);
> >> + ctx->name = NULL;
>
> Here is the other one.
>
> Reinette
>
