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

[Yongseok Koh <yskoh@mellanox.com>]

Hi,

The reason is because the upstream patch doesn't have Cc: stable@dpdk.org.
Adding the tag is at author's discretion. If a patch doesn't have it, we
regard the author doesn't want it to be included in stable releases.
We still urge developers to carefully put the tag for bug fixes. In my
experience, a fix having the tag sometimes had to be excluded. That is
another type of mistake.

>From the next stable release cycle, we will not include such patches which
have "fix" keyword or "Fixes:" tag but no Cc:stable tag at first. Instead,
we will look into such fixes one by one. And we will ask for author's help
if needed.

This was the reason for my email and I was waiting for replies.

As you say it is needed, it has been re-applied to stable/17.11


Thanks,
Yongseok

> On Jan 8, 2019, at 8:47 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
> 
> 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);
>