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 8F44441D9D; Tue, 28 Feb 2023 14:17:49 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 62F98410D4; Tue, 28 Feb 2023 14:17:49 +0100 (CET) Received: from mail-vs1-f41.google.com (mail-vs1-f41.google.com [209.85.217.41]) by mails.dpdk.org (Postfix) with ESMTP id BFBAC4021F for ; Tue, 28 Feb 2023 14:17:47 +0100 (CET) Received: by mail-vs1-f41.google.com with SMTP id s1so15759662vsk.5 for ; Tue, 28 Feb 2023 05:17:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Kgr8LN/oA75rcyEaFQk/gbMWUCCep4gT1LchHsinkcc=; b=HRhmdBSplJC0tTuAfXBoAp8WssnGxdTzDGeBsOfmF/BXxoCTe0bftS+Yv7CeX6T2SG ecZox2P5gW55fJ+Jzb7yvNlzciyFrKve4FEI2xShSK4xqLscZz7PE32rfAdCi1WdfGep JW9pWzoUG8c0+JpznlRkoqsfzR3BmUkgVf1b/+3pNvgf1sktsT6pei8TArDeZxMqUI3j +s0jSFAht4W4g+95S7p/3/x4EUaBC3cy448/Ijjne/qi7bUTv6jMMYciluYX8sIEBgRF C1WyilgsEI5MxGYRgu88XQS8S4XcX8x7C9JCHIkUqtf2fguyMOVJkpCCC9LmK4UjvJev IZYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding: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=Kgr8LN/oA75rcyEaFQk/gbMWUCCep4gT1LchHsinkcc=; b=CbNmIDx8PU3eUls5FiTvHQM4vtaBLmf+j9uks6cgDzp4BrU2CaxM6t9aCA/vMa/vCI MX1K4USGQlsk4uK1JzsMQ8esFPrlEBW8fdAWMMVxVHz6ivELdec4KYaaL7P4GDLH+vv9 MGxfjhFkG3uq4KiYc1RDxeI2dUPG5KDig1EnX1ccrFOtxL/wQutTOFpqb2LDqNxss7Gq DIo/cjVepOzdEkiide2WL9tqJmeClKvVx5EW4kYjhMiWVLs9oOooonITjmDLymyj9MHV zBuEKjtiWdFTxC4P3hagen21quUVLcG9fGYsaiZkysxFROk7FfoQnsjsBhaNI6SqiXKe TejQ== X-Gm-Message-State: AO0yUKVgjwxdOeN8EU0O6zONM1rl5XirDi4hU+Ip3p4CyxtTNJSyebrT XfJ7rCA6NDKfCG/LKPrXFFD4NKGzOZZmTEIFXf0= X-Google-Smtp-Source: AK7set9Cq3HCgisJmUtpJgY0hr8n2hUaXQghMUYLerPLf9+e6c/i20mBqPPebHKm0D8+uhQQAu8fTjQLscHQSK+WmHg= X-Received: by 2002:a05:6102:802:b0:411:fff6:3cc4 with SMTP id g2-20020a056102080200b00411fff63cc4mr2046326vsb.3.1677590267060; Tue, 28 Feb 2023 05:17:47 -0800 (PST) MIME-Version: 1.0 References: <20230223123029.2117781-1-adwivedi@marvell.com> <20230223123029.2117781-2-adwivedi@marvell.com> In-Reply-To: From: Jerin Jacob Date: Tue, 28 Feb 2023 18:47:20 +0530 Message-ID: Subject: Re: [PATCH v1 1/2] ethdev: fix null pointer dereference To: Ferruh Yigit Cc: David Marchand , Ankur Dwivedi , dev@dpdk.org, Thomas Monjalon , jerinj@marvell.com, Ali Alnubani , "Li, WeiyuanX" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Feb 28, 2023 at 6:16=E2=80=AFPM Ferruh Yigit = wrote: > > On 2/28/2023 11:29 AM, David Marchand wrote: > > On Tue, Feb 28, 2023 at 12:05 PM Ferruh Yigit wr= ote: > >> > >> On 2/23/2023 12:30 PM, Ankur Dwivedi wrote: > >>> The speed_fec_capa pointer can be null. So dereferencing the pointer = is > >>> removed and only the pointer is captured in trace function. > >>> Fixed few more trace functions in which null pointer can be dereferen= ced. > >>> > >>> Coverity issue: 383238 > >>> Bugzilla ID: 1162 > >>> Fixes: 6679cf21d608 ("ethdev: add trace points") > >>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") > >>> > >>> Signed-off-by: Ankur Dwivedi > >> > >> Hi Ankur, > >> > >> There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=3D1= 167 > >> > >> > >> As far as I can see that is caused by '__rte_trace_point_register()' i= s > >> calling 'register_fn()' [1]. > >> > >> At registering trace point stage, most of the pointers can be invalid, > >> and this can crash other locations too. > > > > I remember hitting this issue when running with UBsan. > > > >> > >> Why 'register_fn()' called withing the trace point register? Can we > >> remove it? > > > > IIRC, this is used to evaluate the size of the trace point event. > > > > > > Yes, as checked with Jerin, it is used to evaluate size and some sanity > checks fro size. > > We need either find a way to calculate size without really reading the > pointer content during register phase, all convert all pointer tracing > to emit_ptr(). > > I prefer first option if we can. Looking at the root of the issue. rte_trace_point_emit_* has two variant a)trace point version - Which will emit the trace b)trace register version - Which will find the size of trace automatically with single definition of trace point to make life easy for defining the trace point In this case, conf value is junk, as we are referencing the value at registration time. Looks like in PPC arch, the stack content comes as junk at this point and got this issue. And other arch or other environment that adders is OK and since we're just _reading_ the value. It is not making any issue. But it is a bug. RTE_TRACE_POINT was designed to just use rte_trace_point_emit_u16(conf->my_value) so that in registration scope "conf->my_value" will be omitted by compiler. But there as issue in using bitfiled[1] as trace is not supporting bitfield. Ankur added a local variable to work around the bitfiled tracing. So couple of options, I can think of 1)Remove the bitfiled trace point( There are only some trace point uses that, Just we need to remove bitfield items from that) 2)It is possible to have anonymous union of type like u16, u32 for tracing the the bitfields[2], but that would need change in public structure 3) Another option is to have a #define to define the scope of registration. Something like [3] which is noisy. I think, we can just do 1 now for rc2 and (2) or (3) or some other ideas we can think in next release. [1] ../lib/ethdev/ethdev_trace.h: In function =E2=80=98rte_eth_trace_tx_hairpin_queue_setup=E2=80=99: ../lib/eal/include/rte_trace_point.h:378:21: error: cannot take address of bit-field =E2=80=98peer_count=E2=80=99 378 | memcpy(mem, &(in), sizeof(in)); \ | ^ [2] struct abc { union { uint32_t val; struct { val_5_bit:5 } } } [3] [main]dell[dpdk.org] $ git diff diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h index a9682d3f22..266350b29c 100644 --- a/lib/eal/include/rte_trace_point_register.h +++ b/lib/eal/include/rte_trace_point_register.h @@ -16,6 +16,8 @@ extern "C" { #include #include +#define RTE_TRACE_SCOPE_IS_REGISTRATION + RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz); #define RTE_TRACE_POINT_REGISTER(trace, name) \ diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h index 53d1a71ff0..ba42c3d10a 100644 --- a/lib/ethdev/ethdev_trace.h +++ b/lib/ethdev/ethdev_trace.h @@ -237,13 +237,23 @@ RTE_TRACE_POINT( RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id, uint16_t nb_rx_desc, const struct rte_eth_hairpin_conf *con= f, int ret), - uint16_t peer_count =3D conf->peer_count; - uint8_t tx_explicit =3D conf->tx_explicit; - uint8_t manual_bind =3D conf->manual_bind; - uint8_t use_locked_device_memory =3D conf->use_locked_device_memory= ; - uint8_t use_rte_memory =3D conf->use_rte_memory; - uint8_t force_memory =3D conf->force_memory; + uint16_t peer_count; + uint8_t tx_explicit; + uint8_t manual_bind; + uint8_t use_locked_device_memory; + uint8_t use_rte_memory; + uint8_t force_memory; +#ifdef RTE_TRACE_SCOPE_IS_REGISTRATION + RTE_SET_USED(conf); +#else + peer_count =3D conf->peer_count; + tx_explicit =3D conf->tx_explicit; + manual_bind =3D conf->manual_bind; + use_locked_device_memory =3D conf->use_locked_device_memory; + use_rte_memory =3D conf->use_rte_memory; + force_memory =3D conf->force_memory; +#endif rte_trace_point_emit_u16(port_id); rte_trace_point_emit_u16(rx_queue_id); rte_trace_point_emit_u16(nb_rx_desc); [main]dell[dpdk.org] $