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

[Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk>]

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

Hi Ivan, agree. I think we can atleast currently guard all the known
crashes.

Sure, I will check the macro and get back to you.

Thank you!

On Wed, Jul 23, 2025, 18:19 Ivan Malov <ivan.malov@arknetworks.am> wrote:

> Hi Khadem,
>
> On Wed, 23 Jul 2025, Khadem Ullah wrote:
>
> > In secondary processes, directly accessing 'dev->data->dev_private' can
> > cause a segmentation fault if the primary process has exited or if the
> > shared memory is no longer accessible.
> >
> > Secondary application not only breaking on device closing,
> > but also getting segfault when we do "show device info all" from
> secondary
> > after primary closes.
> >
> > This patch adds safety checks while using rte_mem_virt2phy(), with an
> > unlikely() branch hint to minimize performance impact in the fast path.
> > This ensures 'dev_private' is still valid before accessing it.
> >
> > 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 | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index dd7c00bc94..343e156a4f 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -4079,6 +4079,13 @@ 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 &&
> > +             unlikely(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 */
> > @@ -4307,7 +4314,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct
> rte_ether_addr *mac_addr)
> >                       port_id);
> >               return -EINVAL;
> >       }
> > -
> > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> > +             (dev->data->mac_addrs == NULL)) {
> > +                     RTE_ETHDEV_LOG_LINE(ERR,
> > +                     "Secondary: dev_private not accessible (primary
> exited?)");
> > +                     rte_errno = ENODEV;
> > +                     return -rte_errno;
> > +     }
> >       rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
> >
> >       rte_eth_trace_macaddr_get(port_id, mac_addr);
>
> I see one more API has been augmented with the check. But community
> members may
> still argue this is not robust, as many other APIs will also fail. So,
> even if
> the task was to augment as many APIs as possible with the check, then the
> check
> would still be required to be factorised/generalised somehow. What do you
> think?
>
> Please also note that there are already macro invocations in many of these
> APIs,
> for example, RTE_ETH_VALID_PORTID_OR_ERR_RET. Could be convenient.
>
> Thank you.
>
> > --
> > 2.43.0
> >
> >
>

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