[PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

In secondary processes, accessing 'dev->data->dev_private' directly can
cause a segmentation fault if the primary process has exited or the
shared memory is unavailable.

This patch adds a check for dev/data/dev_private and uses
rte_mem_virt2phy to ensure the pointer is still valid.

Fixes: bdad90d12ec8 ('ethdev: change device info get callback to return int')
Cc: stable@dpdk.org

Signed-off-by: Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk>
---
 lib/ethdev/rte_ethdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index dd7c00bc94..03ef446a96 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4079,6 +4079,15 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 
 	if (dev->dev_ops->dev_infos_get == NULL)
 		return -ENOTSUP;
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
+		(dev == NULL || dev->data == NULL ||
+		dev->data->dev_private == NULL ||
+		rte_mem_virt2phy(dev->data->dev_private) == RTE_BAD_PHYS_ADDR)) {
+			RTE_ETHDEV_LOG_LINE(ERR,
+			"Secondary: dev_private not accessible (primary exited?)");
+			rte_errno = ENODEV;
+			return -rte_errno;
+	}
 	diag = dev->dev_ops->dev_infos_get(dev, dev_info);
 	if (diag != 0) {
 		/* Cleanup already filled in device information */
-- 
2.43.0

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Stephen Hemminger]

On Tue, 22 Jul 2025 07:54:39 -0400
Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:

> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> +		(dev == NULL || dev->data == NULL ||
> +		dev->data->dev_private == NULL ||

dev can't be NULL and checking it here will cause a Coverity warning.

There are many other ethdev calls that will fail if primary dies.
stats, xstats, rx/tx burst, ...

I don't think it is good idea to add checks here.

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

Hi Stephen,
Can we add only the check that fixes the segfault, or do you mean that it
should be fixed at the PMD level?

Best regards,
Khadem

On Tue, Jul 22, 2025, 18:39 Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Tue, 22 Jul 2025 07:54:39 -0400
> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>
> > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> > +             (dev == NULL || dev->data == NULL ||
> > +             dev->data->dev_private == NULL ||
>
> dev can't be NULL and checking it here will cause a Coverity warning.
>
> There are many other ethdev calls that will fail if primary dies.
> stats, xstats, rx/tx burst, ...
>
> I don't think it is good idea to add checks here.
>

[-- Attachment #2: Type: text/html, Size: 1290 bytes --]

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Stephen Hemminger]

On Tue, 22 Jul 2025 19:30:41 +0500
Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:

> Hi Stephen,
> Can we add only the check that fixes the segfault, or do you mean that it
> should be fixed at the PMD level?
> 
> Best regards,
> Khadem
> 
> On Tue, Jul 22, 2025, 18:39 Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
> > On Tue, 22 Jul 2025 07:54:39 -0400
> > Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
> >  
> > > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> > > +             (dev == NULL || dev->data == NULL ||
> > > +             dev->data->dev_private == NULL ||  
> >
> > dev can't be NULL and checking it here will cause a Coverity warning.
> >
> > There are many other ethdev calls that will fail if primary dies.
> > stats, xstats, rx/tx burst, ...
> >
> > I don't think it is good idea to add checks here.
> >  

It needs to be fixed at the documentation level.
Make sure and document what applications need to do. Rather than adding
more checks in ethdev.

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

Thanks for the follow up.
Understood. That makes sense. However, I’d like to highlight that
applications should ideally be robust and interactive enough to handle all
edge cases where a segfault or unexpected error might occur. While clear
documentation is certainly important, relying solely on it may not be
sufficient. In my view, potential segfaults should be handled explicitly in
code to ensure stability and to prevent silent failures, especially in
production environments.


On Tue, Jul 22, 2025, 20:42 Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Tue, 22 Jul 2025 19:30:41 +0500
> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>
> > Hi Stephen,
> > Can we add only the check that fixes the segfault, or do you mean that it
> > should be fixed at the PMD level?
> >
> > Best regards,
> > Khadem
> >
> > On Tue, Jul 22, 2025, 18:39 Stephen Hemminger <
> stephen@networkplumber.org>
> > wrote:
> >
> > > On Tue, 22 Jul 2025 07:54:39 -0400
> > > Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
> > >
> > > > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> > > > +             (dev == NULL || dev->data == NULL ||
> > > > +             dev->data->dev_private == NULL ||
> > >
> > > dev can't be NULL and checking it here will cause a Coverity warning.
> > >
> > > There are many other ethdev calls that will fail if primary dies.
> > > stats, xstats, rx/tx burst, ...
> > >
> > > I don't think it is good idea to add checks here.
> > >
>
> It needs to be fixed at the documentation level.
> Make sure and document what applications need to do. Rather than adding
> more checks in ethdev.
>

[-- Attachment #2: Type: text/html, Size: 2510 bytes --]

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Bruce Richardson]

On Tue, Jul 22, 2025 at 09:01:42PM +0500, Khadem Ullah wrote:
>    Thanks for the follow up.
>    Understood. That makes sense. However, I’d like to highlight that
>    applications should ideally be robust and interactive enough to handle
>    all edge cases where a segfault or unexpected error might occur. While
>    clear documentation is certainly important, relying solely on it may
>    not be sufficient. In my view, potential segfaults should be handled
>    explicitly in code to ensure stability and to prevent silent failures,
>    especially in production environments.
> 
In fairness, where stability is the main concern, I'd generally recommend
avoiding multi-process entirely. Or, if it has to be used, the primary
process should be a minimal slim one, that sets up the ports and memory and
thereafter sleeps so that it should never crash unexpectedly!

Even with that, if any secondary process dies, you'll still have all the
buffers in use by that secondary process leaked, so for any multiprocess
system the only safe behaviour for the system is to restart all processes
if any process unexpectedly terminate.

/Bruce

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Agree, but I think it's also a good practice to guard against known cases
that are prone to crashes.

On Tue, Jul 22, 2025 at 9:14 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> On Tue, Jul 22, 2025 at 09:01:42PM +0500, Khadem Ullah wrote:
> >    Thanks for the follow up.
> >    Understood. That makes sense. However, I’d like to highlight that
> >    applications should ideally be robust and interactive enough to handle
> >    all edge cases where a segfault or unexpected error might occur. While
> >    clear documentation is certainly important, relying solely on it may
> >    not be sufficient. In my view, potential segfaults should be handled
> >    explicitly in code to ensure stability and to prevent silent failures,
> >    especially in production environments.
> >
> In fairness, where stability is the main concern, I'd generally recommend
> avoiding multi-process entirely. Or, if it has to be used, the primary
> process should be a minimal slim one, that sets up the ports and memory and
> thereafter sleeps so that it should never crash unexpectedly!
>
> Even with that, if any secondary process dies, you'll still have all the
> buffers in use by that secondary process leaked, so for any multiprocess
> system the only safe behaviour for the system is to restart all processes
> if any process unexpectedly terminate.
>
> /Bruce
>


-- 
Engr. Khadem Ullah,
Software Engineer,
Dreambig Semiconductor Inc
https://dreambigsemi.com/

[-- Attachment #2: Type: text/html, Size: 2532 bytes --]

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Stephen Hemminger]

On Tue, 22 Jul 2025 22:04:32 +0500
Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:

> Agree, but I think it's also a good practice to guard against known cases
> that are prone to crashes.


Right but DPDK chooses performance over API safety.
For example rx/tx burst doesn't check args.

The point is that as a library, if application is doing something wrong
returning error doesn't always help.

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

Right, but performance and reliability are both important. While DPDK
rightly prioritizes performance, some level of reliability should still be
ensured, especially to catch known issues that could lead to instability.

On Tue, Jul 22, 2025, 22:38 Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Tue, 22 Jul 2025 22:04:32 +0500
> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>
> > Agree, but I think it's also a good practice to guard against known cases
> > that are prone to crashes.
>
>
> Right but DPDK chooses performance over API safety.
> For example rx/tx burst doesn't check args.
>
> The point is that as a library, if application is doing something wrong
> returning error doesn't always help.
>

[-- Attachment #2: Type: text/html, Size: 1164 bytes --]

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Stephen Hemminger]

On Tue, 22 Jul 2025 22:53:08 +0500
Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:

> Right, but performance and reliability are both important. While DPDK
> rightly prioritizes performance, some level of reliability should still be
> ensured, especially to catch known issues that could lead to instability.
> 
> On Tue, Jul 22, 2025, 22:38 Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
> > On Tue, 22 Jul 2025 22:04:32 +0500
> > Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
> >  
> > > Agree, but I think it's also a good practice to guard against known cases
> > > that are prone to crashes.  
> >
> >
> > Right but DPDK chooses performance over API safety.
> > For example rx/tx burst doesn't check args.
> >
> > The point is that as a library, if application is doing something wrong
> > returning error doesn't always help.
> >  

The problem is that all those values dev->data and private are shared
between processes without any locking. If the API's are going to MP safe
then they would require locking. The DPDK has made an explicit decision
to not use locking in ethdev control or data path.

You can get away with checking for dev->data being NULL on x86 where
there is data consistency. But on weakly ordered platforms that is not going
to work.

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Khadem Ullah]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

Right, those structures are accessed within each PMD, and I believe this
shouldn't pose issues even on weakly ordered systems. As long as the checks
are implemented using proper DPDK APIs, we can ensure correctness by
enforcing appropriate memory ordering where needed, without introducing
full locking overhead.

On Tue, Jul 22, 2025, 23:21 Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Tue, 22 Jul 2025 22:53:08 +0500
> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>
> > Right, but performance and reliability are both important. While DPDK
> > rightly prioritizes performance, some level of reliability should still
> be
> > ensured, especially to catch known issues that could lead to instability.
> >
> > On Tue, Jul 22, 2025, 22:38 Stephen Hemminger <
> stephen@networkplumber.org>
> > wrote:
> >
> > > On Tue, 22 Jul 2025 22:04:32 +0500
> > > Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
> > >
> > > > Agree, but I think it's also a good practice to guard against known
> cases
> > > > that are prone to crashes.
> > >
> > >
> > > Right but DPDK chooses performance over API safety.
> > > For example rx/tx burst doesn't check args.
> > >
> > > The point is that as a library, if application is doing something wrong
> > > returning error doesn't always help.
> > >
>
> The problem is that all those values dev->data and private are shared
> between processes without any locking. If the API's are going to MP safe
> then they would require locking. The DPDK has made an explicit decision
> to not use locking in ethdev control or data path.
>
> You can get away with checking for dev->data being NULL on x86 where
> there is data consistency. But on weakly ordered platforms that is not
> going
> to work.
>

[-- Attachment #2: Type: text/html, Size: 2546 bytes --]

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Ivan Malov]

There is a difference between control path and data path. Always has been. Yes,
on data path, DPDK has historically sought better performance, but on the slow
path, checks have typically been implemented, even in the flow API, with the
only exception being "asynchronous flow" APIs, which are meant to be fast-path.

Yes, the idea to have a "secondary process reference counter" in 'rte_device'
to be either guarded with its own lock or accessed atomically by 'rte_dev_probe'
and 'rte_dev_remove' (to increment and decrement/check respectively) as well as
by 'rte_eth_dev_close' and 'rte_eth_dev_reset' (to decrement/check) may not be
a hill to die on, to be honest, and might be wrong, so I have no strong opinion.

What scares me most in this idea is that, one may still end up with certain
entry points overlooked, rendering the whole effort worthless.

Thank you.

On Tue, 22 Jul 2025, Stephen Hemminger wrote:

> On Tue, 22 Jul 2025 22:53:08 +0500
> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>
>> Right, but performance and reliability are both important. While DPDK
>> rightly prioritizes performance, some level of reliability should still be
>> ensured, especially to catch known issues that could lead to instability.
>>
>> On Tue, Jul 22, 2025, 22:38 Stephen Hemminger <stephen@networkplumber.org>
>> wrote:
>>
>>> On Tue, 22 Jul 2025 22:04:32 +0500
>>> Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> wrote:
>>>
>>>> Agree, but I think it's also a good practice to guard against known cases
>>>> that are prone to crashes.
>>>
>>>
>>> Right but DPDK chooses performance over API safety.
>>> For example rx/tx burst doesn't check args.
>>>
>>> The point is that as a library, if application is doing something wrong
>>> returning error doesn't always help.
>>>
>
> The problem is that all those values dev->data and private are shared
> between processes without any locking. If the API's are going to MP safe
> then they would require locking. The DPDK has made an explicit decision
> to not use locking in ethdev control or data path.
>
> You can get away with checking for dev->data being NULL on x86 where
> there is data consistency. But on weakly ordered platforms that is not going
> to work.
>

Re: [PATCH] lib/ethdev: fix segfault in secondary process by validating dev_private pointer

[Stephen Hemminger]

On Tue, 22 Jul 2025 23:05:06 +0400 (+04)
Ivan Malov <ivan.malov@arknetworks.am> wrote:

> There is a difference between control path and data path. Always has been. Yes,
> on data path, DPDK has historically sought better performance, but on the slow
> path, checks have typically been implemented, even in the flow API, with the
> only exception being "asynchronous flow" APIs, which are meant to be fast-path.
> 
> Yes, the idea to have a "secondary process reference counter" in 'rte_device'
> to be either guarded with its own lock or accessed atomically by 'rte_dev_probe'
> and 'rte_dev_remove' (to increment and decrement/check respectively) as well as
> by 'rte_eth_dev_close' and 'rte_eth_dev_reset' (to decrement/check) may not be
> a hill to die on, to be honest, and might be wrong, so I have no strong opinion.
> 
> What scares me most in this idea is that, one may still end up with certain
> entry points overlooked, rendering the whole effort worthless.
> 

Please don't top post.

The DPDK control has (up to now) assumed that control operations are only
done from a single thread on each port. There is also the issue of hotplug
but that is separate. For example, if two threads start and stop the
same port bad thing happen and NIC driver's break. 

This is not well documented and a section needs to go into programmer's guide
thread safety. The whole thread safety section is out of date, and doesn't
reference RCU when it should. It also doesn't cover hot plug or weird secondary
processes that fork.

There is also the issue of how primary/secondary monitoring work.
Right now the secondary monitors primary by periodically polling a lock file.
This inherently a racy method and leads to problems. It needs to be redesigned
to use a blocking method something like spawning a thread in secondary
that uses some part of the existing Unix domain IPC to get notification
when primary crashes or wants to exit. Ideally it would support synchronous
handshake with all primaries and asynchronous case when primary crashes.

The point is that bandaid's in the ethdev layer won't fix it well
enough.