From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3303D46BF1 for ; Wed, 23 Jul 2025 15:34:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 292EB40EE6; Wed, 23 Jul 2025 15:34:20 +0200 (CEST) Received: from mail-qv1-f46.google.com (mail-qv1-f46.google.com [209.85.219.46]) by mails.dpdk.org (Postfix) with ESMTP id 6804940E50 for ; Wed, 23 Jul 2025 15:34:18 +0200 (CEST) Received: by mail-qv1-f46.google.com with SMTP id 6a1803df08f44-6faf66905baso77760066d6.2 for ; Wed, 23 Jul 2025 06:34:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=uetpeshawar-edu-pk.20230601.gappssmtp.com; s=20230601; t=1753277658; x=1753882458; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=b0Uf5rxMLSIET9GLGr7yGPmnX2G4OYgh+xb0+BYbA04=; b=yERBo+wsL0lnmuKnbZgL6eS7asZgwr+piA+3csA/f/jti2PlakMsBU9OnelLGukccT ZIfOzqvmAkxyqMDJ/GUiohvvkTvMm4rNfPP2/kv0ng+P/qK/31IjHh6Y3kd+18L/kVWp qyJSwiZOaT5eLBPr/M3mjVhkLbqaRH/i1xEkzXDIAnULhuRDJjyGczdKkOFT7JISGcOI jLgONWadhWh4wG3iZFns/Hzi3gjmT/CSltWnH9XcHhsPMpDY3psMO5tenwh9QZBT/2b9 b6ZonpoyRr/Tj8QcXxAZvrJA0oOwmq7k3KoRXhQkl01vmlyWcyPHRp9B0OH86up8s8Ag lftw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1753277658; x=1753882458; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=b0Uf5rxMLSIET9GLGr7yGPmnX2G4OYgh+xb0+BYbA04=; b=npeZ3IBWRBRyb6Dz0C+pf5I906vw2bv5prekyI353gGzueNV/+52n/r5hJy+QITuCS gNUSdMruTE0K1p8g2WKtEaYBokrDFWC2ooLHo5rEI/NUiYF0w4+P1wKe39eygug2aEtQ qTSViFK6GuY2tEOBHYX3/tT78zcWc3hFZRm5f6uc7fXLk9aE+kd7PqS68Gck4V/sX6/e ldVgWXRL2dbR5Q0cDwbU5Clwz3tIrHY2W74cbFrcHCXlBs9mUHzRXVzihtsmZJetAQqS uTpl96WTED2aFhQ3cBNE4TJQnPqd9Fh7ESYehAf7/olojCR5r6Iq2q5SdOcyKanvnPtH AQXA== X-Forwarded-Encrypted: i=1; AJvYcCWnJtcDxdbo6BRzh4/pFIyR48/Vr5304gp5eWohDWPCe1L4XT0ff6ngW+Wpqm1qmPczRAPTjY0=@dpdk.org X-Gm-Message-State: AOJu0Yx4JRWfgBfU3pqhtLI0KHZJN77oZWaYDf4y4aX4dTjanUZsX2Ve N+XXP8KELLcBA1KFqJGaFDbTAuJ/AoB3+HL98/HHram4u4pcpkfSJm9eXXMnaZzImMUN73oQpSS WYjL+r3S+B3yIH168El3enHYc0jLCfujBq7yKBZgtMQ== X-Gm-Gg: ASbGncuFeX8wTvLzIa+/NRixW3UkRMzXccpuq8D1+/aY6ntDV065XGgLZpUhzN6K6FF MeKSWQ0e5e7S3tuj5JS/RJNsFLV6oLOGaIiw5DXa003a9+tqWivtHcoK24TNnOf4M3LideO0COr Hi6+vKkdm8Ghjl2YQ3XXhxYby4nH2Meyi+d4Rqn8/88PGqxKhAY6/WW+cJSBngO8wIt6yYNX3PE QMLBEB5UzoWsaG++21CyNcV3F6Ff1UkzI4xLdpjjw== X-Google-Smtp-Source: AGHT+IHAkv8uuiQMfK7kJy5nBUDgCz2cwT7/w6pynL3hRRBWX9G0De/aQYSzZYjNpy3Bxt2wGM0rNeFEGLIut74/0dU= X-Received: by 2002:ad4:5aa8:0:b0:706:f3f9:8a52 with SMTP id 6a1803df08f44-7070062e461mr33417166d6.29.1753277657651; Wed, 23 Jul 2025 06:34:17 -0700 (PDT) MIME-Version: 1.0 References: <20250723045022.1580829-1-14pwcse1224@uetpeshawar.edu.pk> <20250723131049.1703172-1-14pwcse1224@uetpeshawar.edu.pk> <0792a0e5-dbbf-f3bc-527b-2fe7d33e8822@arknetworks.am> In-Reply-To: <0792a0e5-dbbf-f3bc-527b-2fe7d33e8822@arknetworks.am> From: Khadem Ullah <14pwcse1224@uetpeshawar.edu.pk> Date: Wed, 23 Jul 2025 18:34:04 +0500 X-Gm-Features: Ac12FXz162K9WKiEe1tGLRn3yP1mmAZ-XnIr_G93vZ-uHcUB7WmYq6rfQbDUflo Message-ID: Subject: Re: [PATCH] [PATCH v3] lib/ethdev: fix segfault in secondary process by validating dev_private pointer To: Ivan Malov Cc: Stephen Hemminger , Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , dev@dpdk.org, dpdk stable Content-Type: multipart/alternative; boundary="000000000000fa37ab063a98c33c" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org --000000000000fa37ab063a98c33c Content-Type: text/plain; charset="UTF-8" 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 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 > > > > > --000000000000fa37ab063a98c33c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ivan, agree. I think we can atleast currently guard al= l the known crashes.

Sure, I w= ill check the macro and get back=C2=A0to you.=C2=A0
=
Thank you!

On Wed, J= ul 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_p= rivate' 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<= br> > 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.ed= u.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 r= te_eth_dev_info *dev_info)
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (dev->dev_ops->dev_infos_get =3D=3D= NULL)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOTSUP;=
> +=C2=A0 =C2=A0 =C2=A0if (rte_eal_process_type() =3D=3D RTE_PROC_SECOND= ARY &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unlikely(rte_mem_virt= 2phy(dev->data->dev_private) =3D=3D RTE_BAD_PHYS_ADDR)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0RTE_ETHDEV_LOG_LINE(ERR,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0"Secondary: dev_private not accessible (primary exited?)")= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0rte_errno =3D ENODEV;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -rte_errno;
> +=C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0diag =3D dev->dev_ops->dev_infos_get(d= ev, dev_info);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (diag !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Cleanup alrea= dy filled in device information */
> @@ -4307,7 +4314,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct rt= e_ether_addr *mac_addr)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0port_id);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -
> +=C2=A0 =C2=A0 =C2=A0if (rte_eal_process_type() =3D=3D RTE_PROC_SECOND= ARY &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(dev->data->mac= _addrs =3D=3D NULL)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0RTE_ETHDEV_LOG_LINE(ERR,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0"Secondary: dev_private not accessible (primary exited?)")= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0rte_errno =3D ENODEV;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -rte_errno;
> +=C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rte_ether_addr_copy(&dev->data->ma= c_addrs[0], mac_addr);
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rte_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 c= heck
would still be required to be factorised/generalised somehow. What do you t= hink?

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
>
>
--000000000000fa37ab063a98c33c--