Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in secondary process' has been queued to LTS release 17.11.5

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

Hi,

What's the reason for "mistakenly" merging this patch? This patch fixes a real bug. Why revert?

Thanks,
Anatoly


> -----Original Message-----
> From: Yongseok Koh [mailto:yskoh@mellanox.com]
> Sent: Thursday, December 20, 2018 12:22 AM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; dpdk stable <stable@dpdk.org>
> Subject: Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in
> secondary process' has been queued to LTS release 17.11.5
> 
> Hi,
> 
> This patch is being removed from stable/17.11 as it was mistakenly merged.
> Patches having 'fix' keyword in the title were merged even though those
> don't have "Cc: stable@dpdk.org" tag in the commit message.
> 
> If you think this patch is still needed for stable/17.11, please let me know.
> Then I'll take it back.
> 
> 
> Thanks,
> Yongseok
> 
> 
> > On Nov 29, 2018, at 3:12 PM, Yongseok Koh <yskoh@mellanox.com>
> wrote:
> >
> > Hi,
> >
> > FYI, your patch has been queued to LTS release 17.11.5
> >
> > Note it hasn't been pushed to
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> k.org%2Fbrowse%2Fdpdk-
> stable&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C984a5148ee3f4d
> 0911c508d65650a735%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7
> C636791301732433081&amp;sdata=xSYO3rfdcp080xZsdFughJwuHl10CW%2B6
> njLe8SLNR5o%3D&amp;reserved=0 yet.
> > It will be pushed if I get no objections before 12/01/18. So please
> > shout if anyone has objections.
> >
> > Also note that after the patch there's a diff of the upstream commit
> > vs the patch applied to the branch. If the code is different (ie: not
> > only metadata diffs), due for example to a change in context or macro
> names, please double check it.
> >
> > Thanks.
> >
> > Yongseok
> >
> > ---
> > From ff44ba8002e2db8c02eb769d547bfd7659af14e1 Mon Sep 17 00:00:00
> 2001
> > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > Date: Wed, 21 Nov 2018 19:41:32 +0100
> > Subject: [PATCH] vfio: do not needlessly setup device in secondary
> > process
> >
> > [ upstream commit 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 ]
> >
> > 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 fully
> > prevent setting up devices in secondary processes that weren't setup
> > in the primary process.
> >
> > Fixes: 2f4adfad0a69 ("vfio: add multiprocess support")
> >
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.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 745db260c..654897284 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -580,11 +580,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,
> > @@ -596,9 +591,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.11.0
> >
> > ---
> >  Diff of the applied patch vs upstream commit (please double-check if non-
> empty:
> > ---
> > --- -	2018-11-29 15:01:50.745484864 -0800
> > +++ 0127-vfio-do-not-needlessly-setup-device-in-secondary-pro.patch
> 	2018-11-29 15:01:45.335961000 -0800
> > @@ -1,8 +1,10 @@
> > -From 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 Mon Sep 17 00:00:00
> > 2001
> > +From ff44ba8002e2db8c02eb769d547bfd7659af14e1 Mon Sep 17 00:00:00
> > +2001
> > From: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > Date: Wed, 21 Nov 2018 19:41:32 +0100
> > Subject: [PATCH] vfio: do not needlessly setup device in secondary
> > process
> >
> > +[ upstream commit 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 ]
> > +
> > 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 @@ -31,10 +33,10 @@
> >  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
> > +index 745db260c..654897284 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)
> > +@@ -580,11 +580,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);
> >
> > @@ -46,7 +48,7 @@
> >  	/* 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)
> > +@@ -596,9 +591,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);