DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process
@ 2018-11-21 18:41 Darek Stojaczyk
  2018-11-23  9:12 ` Burakov, Anatoly
  2018-11-23 13:20 ` Maxime Coquelin
  0 siblings, 2 replies; 4+ messages in thread
From: Darek Stojaczyk @ 2018-11-21 18:41 UTC (permalink / raw)
  To: dev; +Cc: Darek Stojaczyk, anatoly.burakov

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process
  2018-11-21 18:41 [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process Darek Stojaczyk
@ 2018-11-23  9:12 ` Burakov, Anatoly
  2018-11-25 12:03   ` Thomas Monjalon
  2018-11-23 13:20 ` Maxime Coquelin
  1 sibling, 1 reply; 4+ messages in thread
From: Burakov, Anatoly @ 2018-11-23  9:12 UTC (permalink / raw)
  To: Darek Stojaczyk, dev

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process
  2018-11-21 18:41 [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process Darek Stojaczyk
  2018-11-23  9:12 ` Burakov, Anatoly
@ 2018-11-23 13:20 ` Maxime Coquelin
  1 sibling, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2018-11-23 13:20 UTC (permalink / raw)
  To: Darek Stojaczyk, dev; +Cc: anatoly.burakov



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process
  2018-11-23  9:12 ` Burakov, Anatoly
@ 2018-11-25 12:03   ` Thomas Monjalon
  0 siblings, 0 replies; 4+ messages in thread
From: Thomas Monjalon @ 2018-11-25 12:03 UTC (permalink / raw)
  To: Darek Stojaczyk; +Cc: dev, Burakov, Anatoly

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-11-25 12:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 18:41 [dpdk-dev] [PATCH] vfio: don't needlessly setup devices in secondary process Darek Stojaczyk
2018-11-23  9:12 ` Burakov, Anatoly
2018-11-25 12:03   ` Thomas Monjalon
2018-11-23 13:20 ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).