[dpdk-dev] [PATCH] eal: fix secondary process segfault on multipe virtio devices

[dpdk-dev] [PATCH] eal: fix secondary process segfault on multipe virtio devices

[Jianfeng Tan]

Suppose we have 2 virtio devices for a VM, with only the first one,
virtio0, binding to igb_uio. Start a primary DPDK process, driving
only virtio0. Then start a secondary DPDK process, it encounters
segfault at eth_virtio_dev_init() because hw is NULL, when trying
to initialize the 2nd virtio devices.
    1539                    if (!hw->virtio_user_dev) {

We could add a precheck to return error when hw is NULL. But the
root cause is that virtio devices which are not driven by the primary
process are not exluded by secondary eal probe function.

To support legacy virtio devices bound to none kernel driver, we
removed RTE_PCI_DRV_NEED_MAPPING in
commit 962cf902e6eb ("pci: export device mapping functions").
At the boot of primary process, ether dev is allocated in rte_eth_devices
array, rte_eth_dev_data is also allocated in rte_eth_dev_data array; then
probe function fails; and ether dev is released. However, the entry in
rte_eth_dev_data array is not cleared. Then we start secondary process,
and try to attach the virtio device that not used in primary process,
the field, dev_private (or hw), in rte_eth_dev_data, is NULL.

To fail the dev attach, we need to clear the field, name, when we
release any ether devices in primary, so that below loop in
rte_eth_dev_attach_secondary() will not find any matched names.
        for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
                if (strcmp(rte_eth_dev_data[i].name, name) == 0)
                        break;
        }

Fixes: 6d890f8ab512 ("Fixes: net/virtio: fix multiple process support")

Reported-by: Reshma Pattan <reshma.pattan@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_ether/rte_ethdev_pci.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
index d3bc03c..025700d 100644
--- a/lib/librte_ether/rte_ethdev_pci.h
+++ b/lib/librte_ether/rte_ethdev_pci.h
@@ -134,6 +134,11 @@ rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
 
 	eth_dev->data->dev_private = NULL;
 
+	/* Secondary process will use this field, name, for secondary attach,
+	 * clear this field to avoid attaching any released ports in primary.
+	 */
+	memset(eth_dev->data->name, 0, RTE_ETH_NAME_MAX_LEN);
+
 	eth_dev->device = NULL;
 	eth_dev->intr_handle = NULL;
 }
-- 
2.7.4

Re: [dpdk-dev] [PATCH] eal: fix secondary process segfault on multipe virtio devices

[Thomas Monjalon]

02/06/2017 18:20, Jianfeng Tan:
> Suppose we have 2 virtio devices for a VM, with only the first one,
> virtio0, binding to igb_uio. Start a primary DPDK process, driving
> only virtio0. Then start a secondary DPDK process, it encounters
> segfault at eth_virtio_dev_init() because hw is NULL, when trying
> to initialize the 2nd virtio devices.
>     1539                    if (!hw->virtio_user_dev) {
> 
> We could add a precheck to return error when hw is NULL. But the
> root cause is that virtio devices which are not driven by the primary
> process are not exluded by secondary eal probe function.
> 
> To support legacy virtio devices bound to none kernel driver, we
> removed RTE_PCI_DRV_NEED_MAPPING in
> commit 962cf902e6eb ("pci: export device mapping functions").
> At the boot of primary process, ether dev is allocated in rte_eth_devices
> array, rte_eth_dev_data is also allocated in rte_eth_dev_data array; then
> probe function fails; and ether dev is released. However, the entry in
> rte_eth_dev_data array is not cleared. Then we start secondary process,
> and try to attach the virtio device that not used in primary process,
> the field, dev_private (or hw), in rte_eth_dev_data, is NULL.
> 
> To fail the dev attach, we need to clear the field, name, when we
> release any ether devices in primary, so that below loop in
> rte_eth_dev_attach_secondary() will not find any matched names.
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>                 if (strcmp(rte_eth_dev_data[i].name, name) == 0)
>                         break;
>         }
> 
> Fixes: 6d890f8ab512 ("Fixes: net/virtio: fix multiple process support")

It probably needs to be Cc: stable@dpdk.org

> Reported-by: Reshma Pattan <reshma.pattan@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
[...]
>  	eth_dev->data->dev_private = NULL;
>  
> +	/* Secondary process will use this field, name, for secondary attach,
> +	 * clear this field to avoid attaching any released ports in primary.
> +	 */
> +	memset(eth_dev->data->name, 0, RTE_ETH_NAME_MAX_LEN);

I think it may be sufficient to set an empty string:
eth_dev->data->name = '\0';

[dpdk-dev] [PATCH v2] eal: fix secondary process segfault on multipe virtio devices

[Jianfeng Tan]

Suppose we have 2 virtio devices for a VM, with only the first one,
virtio0, binding to igb_uio. Start a primary DPDK process, driving
only virtio0. Then start a secondary DPDK process, it encounters
segfault at eth_virtio_dev_init() because hw is NULL, when trying
to initialize the 2nd virtio devices.
    1539                    if (!hw->virtio_user_dev) {

We could add a precheck to return error when hw is NULL. But the
root cause is that virtio devices which are not driven by the primary
process are not exluded by secondary eal probe function.

To support legacy virtio devices bound to none kernel driver, we
removed RTE_PCI_DRV_NEED_MAPPING in
commit 962cf902e6eb ("pci: export device mapping functions").
At the boot of primary process, ether dev is allocated in rte_eth_devices
array, rte_eth_dev_data is also allocated in rte_eth_dev_data array; then
probe function fails; and ether dev is released. However, the entry in
rte_eth_dev_data array is not cleared. Then we start secondary process,
and try to attach the virtio device that not used in primary process,
the field, dev_private (or hw), in rte_eth_dev_data, is NULL.

To fail the dev attach, we need to clear the field, name, when we
release any ether devices in primary, so that below loop in
rte_eth_dev_attach_secondary() will not find any matched names.
        for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
                if (strcmp(rte_eth_dev_data[i].name, name) == 0)
                        break;
        }

Fixes: 6d890f8ab512 ("Fixes: net/virtio: fix multiple process support")
Cc: stable@dpdk.org

Reported-by: Reshma Pattan <reshma.pattan@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
v2:
  - Assign '\0' to first char of name instead of memset as per Thomas's advice.
 lib/librte_ether/rte_ethdev_pci.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
index 69aab03..0e7d7a9 100644
--- a/lib/librte_ether/rte_ethdev_pci.h
+++ b/lib/librte_ether/rte_ethdev_pci.h
@@ -133,6 +133,12 @@ rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
 
 	eth_dev->data->dev_private = NULL;
 
+	/* Secondary process will use the field, name, for secondary
+	 * attach, clear this field to avoid attaching any released
+	 * ports in secondary processes.
+	 */
+	eth_dev->data->name[0] = '\0';
+
 	eth_dev->device = NULL;
 	eth_dev->intr_handle = NULL;
 }
-- 
2.7.4

Re: [dpdk-dev] [PATCH] eal: fix secondary process segfault on multipe virtio devices

[Tan, Jianfeng]

HI Thomas,


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Saturday, July 1, 2017 10:10 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com;
> maxime.coquelin@redhat.com
> Subject: Re: [dpdk-dev] [PATCH] eal: fix secondary process segfault on
> multipe virtio devices
> 
> 02/06/2017 18:20, Jianfeng Tan:
> > Suppose we have 2 virtio devices for a VM, with only the first one,
> > virtio0, binding to igb_uio. Start a primary DPDK process, driving
> > only virtio0. Then start a secondary DPDK process, it encounters
> > segfault at eth_virtio_dev_init() because hw is NULL, when trying
> > to initialize the 2nd virtio devices.
> >     1539                    if (!hw->virtio_user_dev) {
> >
> > We could add a precheck to return error when hw is NULL. But the
> > root cause is that virtio devices which are not driven by the primary
> > process are not exluded by secondary eal probe function.
> >
> > To support legacy virtio devices bound to none kernel driver, we
> > removed RTE_PCI_DRV_NEED_MAPPING in
> > commit 962cf902e6eb ("pci: export device mapping functions").
> > At the boot of primary process, ether dev is allocated in rte_eth_devices
> > array, rte_eth_dev_data is also allocated in rte_eth_dev_data array; then
> > probe function fails; and ether dev is released. However, the entry in
> > rte_eth_dev_data array is not cleared. Then we start secondary process,
> > and try to attach the virtio device that not used in primary process,
> > the field, dev_private (or hw), in rte_eth_dev_data, is NULL.
> >
> > To fail the dev attach, we need to clear the field, name, when we
> > release any ether devices in primary, so that below loop in
> > rte_eth_dev_attach_secondary() will not find any matched names.
> >         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> >                 if (strcmp(rte_eth_dev_data[i].name, name) == 0)
> >                         break;
> >         }
> >
> > Fixes: 6d890f8ab512 ("Fixes: net/virtio: fix multiple process support")
> 
> It probably needs to be Cc: stable@dpdk.org
> 
> > Reported-by: Reshma Pattan <reshma.pattan@intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> [...]
> >  	eth_dev->data->dev_private = NULL;
> >
> > +	/* Secondary process will use this field, name, for secondary attach,
> > +	 * clear this field to avoid attaching any released ports in primary.
> > +	 */
> > +	memset(eth_dev->data->name, 0, RTE_ETH_NAME_MAX_LEN);
> 
> I think it may be sufficient to set an empty string:
> eth_dev->data->name = '\0';

Thank you for the advice. And a new version has been sent.

Thanks,
Jianfeng

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] eal: fix secondary process segfault on multipe virtio devices

[Thomas Monjalon]

03/07/2017 08:37, Jianfeng Tan:
> Suppose we have 2 virtio devices for a VM, with only the first one,
> virtio0, binding to igb_uio. Start a primary DPDK process, driving
> only virtio0. Then start a secondary DPDK process, it encounters
> segfault at eth_virtio_dev_init() because hw is NULL, when trying
> to initialize the 2nd virtio devices.
>     1539                    if (!hw->virtio_user_dev) {
> 
> We could add a precheck to return error when hw is NULL. But the
> root cause is that virtio devices which are not driven by the primary
> process are not exluded by secondary eal probe function.
> 
> To support legacy virtio devices bound to none kernel driver, we
> removed RTE_PCI_DRV_NEED_MAPPING in
> commit 962cf902e6eb ("pci: export device mapping functions").
> At the boot of primary process, ether dev is allocated in rte_eth_devices
> array, rte_eth_dev_data is also allocated in rte_eth_dev_data array; then
> probe function fails; and ether dev is released. However, the entry in
> rte_eth_dev_data array is not cleared. Then we start secondary process,
> and try to attach the virtio device that not used in primary process,
> the field, dev_private (or hw), in rte_eth_dev_data, is NULL.
> 
> To fail the dev attach, we need to clear the field, name, when we
> release any ether devices in primary, so that below loop in
> rte_eth_dev_attach_secondary() will not find any matched names.
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>                 if (strcmp(rte_eth_dev_data[i].name, name) == 0)
>                         break;
>         }
> 
> Fixes: 6d890f8ab512 ("Fixes: net/virtio: fix multiple process support")
> Cc: stable@dpdk.org
> 
> Reported-by: Reshma Pattan <reshma.pattan@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Reworded the comment and applied, thanks