[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 --]