[dpdk-dev,v1] ethdev: fix multi-process NULL dereference crashes

  Even though only primary processes should setup PMDs, secondary
processes were also blanket zeroing ethernet device memory. The
result was NULL dereference crashes in multi-process setups.

Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 doc/guides/rel_notes/release_17_02.rst | 6 ++++++
 lib/librte_ether/rte_ethdev.c          | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)
  

2017-01-11 02:42, Remy Horton:
> Even though only primary processes should setup PMDs, secondary
> processes were also blanket zeroing ethernet device memory. The
> result was NULL dereference crashes in multi-process setups.
> 
> Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

I think it can be fixed by this patch:

http://dpdk.org/ml/archives/dev/2017-January/054220.html
  

On 11/01/2017 14:22, Thomas Monjalon wrote:
> 2017-01-11 02:42, Remy Horton:
>> Even though only primary processes should setup PMDs, secondary
>> processes were also blanket zeroing ethernet device memory. The
>> result was NULL dereference crashes in multi-process setups.
>> Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")
>
> I think it can be fixed by this patch:
>
> http://dpdk.org/ml/archives/dev/2017-January/054220.html

Close call - really depends how the (likley) merge conflict is resolved...

..Remy
  

2017-01-11 02:42, Remy Horton:
> +* **ethdev: Fixed crash with multi-processing.**
> +
> +  Even though only primary processes should setup PMDs, secondary
> +  processes were also blanket zeroing ethernet device memory. The
> +  result was NULL dereference crashes in multi-process setups.
> +

3 comments here:
- it is in the wrong section (EAL instead of Drivers)
- secondary processes can setup a vdev PMD
- before Yuanhan's patch, even PCI PMD were blanking primary process data


> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -212,7 +212,8 @@ rte_eth_dev_allocate(const char *name)
>  
>  	eth_dev = &rte_eth_devices[port_id];
>  	eth_dev->data = &rte_eth_dev_data[port_id];
> -	memset(eth_dev->data, 0, sizeof(*eth_dev->data));
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +		memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>  	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>  	eth_dev->data->port_id = port_id;
>  	eth_dev->data->mtu = ETHER_MTU;
> 

I propose this rebase:

-       memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));
        eth_dev = eth_dev_get(port_id);
+       if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+               memset(eth_dev->data, 0, sizeof(*eth_dev->data));
        snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
        eth_dev->data->port_id = port_id;
        eth_dev->data->mtu = ETHER_MTU;
  

On 20/01/2017 18:37, Thomas Monjalon wrote:
[..]
> 3 comments here:
> - it is in the wrong section (EAL instead of Drivers)
> - secondary processes can setup a vdev PMD
> - before Yuanhan's patch, even PCI PMD were blanking primary process data

Since the code being changed is in rte_ether rather than drivers/net it 
seemed the logical place to me.. :)

> I propose this rebase:
>
> -       memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));
>         eth_dev = eth_dev_get(port_id);
> +       if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +               memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>         snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>         eth_dev->data->port_id = port_id;
>         eth_dev->data->mtu = ETHER_MTU;

Seems OK to me, assuming Yuanhan's patch is going in as-is.

..Remy
  

2017-01-24 08:16, Remy Horton:
> 
> On 20/01/2017 18:37, Thomas Monjalon wrote:
> [..]
> > 3 comments here:
> > - it is in the wrong section (EAL instead of Drivers)
> > - secondary processes can setup a vdev PMD
> > - before Yuanhan's patch, even PCI PMD were blanking primary process data
> 
> Since the code being changed is in rte_ether rather than drivers/net it 
> seemed the logical place to me.. :)

The change is in ethdev, and you put the release note in EAL.
So no, it is not logical, because ethdev is not EAL.

> > I propose this rebase:
> >
> > -       memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));
> >         eth_dev = eth_dev_get(port_id);
> > +       if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > +               memset(eth_dev->data, 0, sizeof(*eth_dev->data));
> >         snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> >         eth_dev->data->port_id = port_id;
> >         eth_dev->data->mtu = ETHER_MTU;
> 
> Seems OK to me, assuming Yuanhan's patch is going in as-is.

Yuanhan's patch is already part of RC1.
  

On 24/01/2017 10:49, Thomas Monjalon wrote:
[..]
>> Seems OK to me, assuming Yuanhan's patch is going in as-is.
>
> Yuanhan's patch is already part of RC1.

Ah ok. I'll rebase a v2 then..