Received-SPF: pass (google.com: domain of linux-kernel+bounces-55664-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 6 Feb 2024 15:03:16 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: <jgg@nvidia.com>, <yishaih@nvidia.com>,
 <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
 <kvm@vger.kernel.org>, <dave.jiang@intel.com>, <ashok.raj@intel.com>,
 <linux-kernel@vger.kernel.org>, <patches@lists.linux.dev>
Subject: Re: [PATCH 07/17] vfio/pci: Preserve per-interrupt contexts
Message-ID: <20240206150316.66e24a8d.alex.williamson@redhat.com>
In-Reply-To: <6cef4f69-e19d-4741-9cff-a9485dd58d89@intel.com>
References: <cover.1706849424.git.reinette.chatre@intel.com>
	<d6e32e0e7adaf61da39fb6cd2863298b15a2663e.1706849424.git.reinette.chatre@intel.com>
	<20240205153509.333c2c95.alex.williamson@redhat.com>
	<6cef4f69-e19d-4741-9cff-a9485dd58d89@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 6 Feb 2024 13:45:22 -0800
Reinette Chatre <reinette.chatre@intel.com> 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@intel.com> 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
>