[dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process

[dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process

[Darek Stojaczyk]

Setting up a device that wasn't setup in the primary
process will possibly break the primary process. That's
because the IPC message to retrieve the group fd in the
primary will also *open* that group if it wasn't opened
before. Even though the secondary process closes that fd
soon after as a part of its error handling path, the
primary process leaks it.

What's worse, opening that fd on the primary will
increment the process-local counter of opened groups.
If it was 0 before, then the group will never be added
to the vfio container, nor dpdk memory will be ever
mapped.

This patch moves the proper error checks earlier in the
code to fuly prevent setting up devices in secondary
processes that weren't setup in the primary process.

Fixes: 2f4adfad0a69 ("vfio: add multiprocess support")
Cc: anatoly.burakov@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index ffd26f195..54a4c959e 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -794,11 +794,6 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
-	ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
-					&vfio_dev_fd, &device_info);
-	if (ret)
-		return ret;
-
 	/* if we're in a secondary process, just find our tailq entry */
 	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
 		if (rte_pci_addr_cmp(&vfio_res->pci_addr,
@@ -810,9 +805,14 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	if (vfio_res == NULL) {
 		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI device!\n",
 				pci_addr);
-		goto err_vfio_dev_fd;
+		return -1;
 	}
 
+	ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
+					&vfio_dev_fd, &device_info);
+	if (ret)
+		return ret;
+
 	/* map BARs */
 	maps = vfio_res->maps;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process

[Burakov, Anatoly]

On 21-Nov-18 6:41 PM, Darek Stojaczyk wrote:
> Setting up a device that wasn't setup in the primary
> process will possibly break the primary process. That's
> because the IPC message to retrieve the group fd in the
> primary will also *open* that group if it wasn't opened
> before. Even though the secondary process closes that fd
> soon after as a part of its error handling path, the
> primary process leaks it.
> 
> What's worse, opening that fd on the primary will
> increment the process-local counter of opened groups.
> If it was 0 before, then the group will never be added
> to the vfio container, nor dpdk memory will be ever
> mapped.
> 
> This patch moves the proper error checks earlier in the
> code to fuly prevent setting up devices in secondary
> processes that weren't setup in the primary process.
> 
> Fixes: 2f4adfad0a69 ("vfio: add multiprocess support")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process

[Thomas Monjalon]

23/11/2018 10:12, Burakov, Anatoly:
> On 21-Nov-18 6:41 PM, Darek Stojaczyk wrote:
> > Setting up a device that wasn't setup in the primary
> > process will possibly break the primary process. That's
> > because the IPC message to retrieve the group fd in the
> > primary will also *open* that group if it wasn't opened
> > before. Even though the secondary process closes that fd
> > soon after as a part of its error handling path, the
> > primary process leaks it.
> > 
> > What's worse, opening that fd on the primary will
> > increment the process-local counter of opened groups.
> > If it was 0 before, then the group will never be added
> > to the vfio container, nor dpdk memory will be ever
> > mapped.
> > 
> > This patch moves the proper error checks earlier in the
> > code to fuly prevent setting up devices in secondary
> > processes that weren't setup in the primary process.
> > 
> > Fixes: 2f4adfad0a69 ("vfio: add multiprocess support")
> > Cc: anatoly.burakov@intel.com
> > 
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks

Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process

[Maxime Coquelin]

On 11/21/18 7:41 PM, Darek Stojaczyk wrote:
> Setting up a device that wasn't setup in the primary
> process will possibly break the primary process. That's
> because the IPC message to retrieve the group fd in the
> primary will also *open* that group if it wasn't opened
> before. Even though the secondary process closes that fd
> soon after as a part of its error handling path, the
> primary process leaks it.
> 
> What's worse, opening that fd on the primary will
> increment the process-local counter of opened groups.
> If it was 0 before, then the group will never be added
> to the vfio container, nor dpdk memory will be ever
> mapped.
> 
> This patch moves the proper error checks earlier in the
> code to fuly prevent setting up devices in secondary

s/fuly/fully/

> processes that weren't setup in the primary process.
> 
> Fixes: 2f4adfad0a69 ("vfio: add multiprocess support")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index ffd26f195..54a4c959e 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -794,11 +794,6 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>   			loc->domain, loc->bus, loc->devid, loc->function);
>   
> -	ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> -					&vfio_dev_fd, &device_info);
> -	if (ret)
> -		return ret;
> -
>   	/* if we're in a secondary process, just find our tailq entry */
>   	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
>   		if (rte_pci_addr_cmp(&vfio_res->pci_addr,
> @@ -810,9 +805,14 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   	if (vfio_res == NULL) {
>   		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI device!\n",
>   				pci_addr);
> -		goto err_vfio_dev_fd;
> +		return -1;
>   	}
>   
> +	ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> +					&vfio_dev_fd, &device_info);
> +	if (ret)
> +		return ret;
> +
>   	/* map BARs */
>   	maps = vfio_res->maps;
>   
> 

Other than commit message typo:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime