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

Message ID 1484073764-15001-1-git-send-email-remy.horton@intel.com
State Superseded, archived
Headers show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Remy Horton Jan. 10, 2017, 6:42 p.m. UTC
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(-)

Comments

Thomas Monjalon Jan. 11, 2017, 2:22 p.m. UTC | #1
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
Remy Horton Jan. 11, 2017, 3:01 p.m. UTC | #2
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
Thomas Monjalon Jan. 20, 2017, 6:37 p.m. UTC | #3
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;
Remy Horton Jan. 24, 2017, 8:16 a.m. UTC | #4
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
Thomas Monjalon Jan. 24, 2017, 10:49 a.m. UTC | #5
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.
Remy Horton Jan. 24, 2017, 11:03 a.m. UTC | #6
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..

Patch
diff mbox

diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
index 180af82..20a4ced 100644
--- a/doc/guides/rel_notes/release_17_02.rst
+++ b/doc/guides/rel_notes/release_17_02.rst
@@ -72,6 +72,12 @@  Resolved Issues
 EAL
 ~~~
 
+* **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.
+
 
 Drivers
 ~~~~~~~
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 9dea1f1..a681982 100644
--- 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;