* [PATCH v1 0/2] bug fix in ethdev trace
@ 2023-02-23 12:30 Ankur Dwivedi
2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Ankur Dwivedi @ 2023-02-23 12:30 UTC (permalink / raw)
To: dev; +Cc: jerinj, Ankur Dwivedi
The first patch in this series adds fix for bug and coverity.
The second patch makes change to pass structure pointer instead of the
structure value, to avoid 64 bytes copy in function call stack for
rte_eth_trace_xstats_get_names.
Ankur Dwivedi (2):
ethdev: fix null pointer dereference
ethdev: pass structure pointer
lib/ethdev/ethdev_trace.h | 33 +++++++++------------------------
lib/ethdev/rte_ethdev.c | 2 +-
2 files changed, 10 insertions(+), 25 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-23 12:30 [PATCH v1 0/2] bug fix in ethdev trace Ankur Dwivedi @ 2023-02-23 12:30 ` Ankur Dwivedi 2023-02-28 11:04 ` Ferruh Yigit 2023-02-28 15:01 ` Ferruh Yigit 2023-02-23 12:30 ` [PATCH v1 2/2] ethdev: pass structure pointer Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi 2 siblings, 2 replies; 20+ messages in thread From: Ankur Dwivedi @ 2023-02-23 12:30 UTC (permalink / raw) To: dev; +Cc: jerinj, Ankur Dwivedi 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 dereferenced. 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 <adwivedi@marvell.com> --- lib/ethdev/ethdev_trace.h | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h index 53d1a71ff0..a13e33fe64 100644 --- a/lib/ethdev/ethdev_trace.h +++ b/lib/ethdev/ethdev_trace.h @@ -123,8 +123,7 @@ RTE_TRACE_POINT( RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct rte_eth_dev_owner *owner, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u64(owner->id); - rte_trace_point_emit_string(owner->name); + rte_trace_point_emit_ptr(owner); rte_trace_point_emit_int(ret); ) @@ -375,9 +374,7 @@ RTE_TRACE_POINT( rte_eth_trace_find_next_of, RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct rte_device *parent), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_string(parent->name); - rte_trace_point_emit_string(parent->bus_info); - rte_trace_point_emit_int(parent->numa_node); + rte_trace_point_emit_ptr(parent); ) RTE_TRACE_POINT( @@ -869,8 +866,7 @@ RTE_TRACE_POINT( const struct rte_eth_fec_capa *speed_fec_capa, unsigned int num, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u32(speed_fec_capa->speed); - rte_trace_point_emit_u32(speed_fec_capa->capa); + rte_trace_point_emit_ptr(speed_fec_capa); rte_trace_point_emit_u32(num); rte_trace_point_emit_int(ret); ) @@ -1416,8 +1412,7 @@ RTE_TRACE_POINT( const struct rte_flow_item *pattern, const struct rte_flow_action *actions, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u32(attr->group); - rte_trace_point_emit_u32(attr->priority); + rte_trace_point_emit_ptr(attr); rte_trace_point_emit_ptr(pattern); rte_trace_point_emit_ptr(actions); rte_trace_point_emit_int(ret); @@ -1510,10 +1505,7 @@ RTE_TRACE_POINT( const struct rte_flow_item_flex_conf *conf, const struct rte_flow_item_flex_handle *handle), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_int(conf->tunnel); - rte_trace_point_emit_int(conf->nb_samples); - rte_trace_point_emit_int(conf->nb_inputs); - rte_trace_point_emit_int(conf->nb_outputs); + rte_trace_point_emit_ptr(conf); rte_trace_point_emit_ptr(handle); ) @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( int ret), rte_trace_point_emit_u16(port_id); rte_trace_point_emit_ptr(flow); - rte_trace_point_emit_int(action->type); - rte_trace_point_emit_ptr(action->conf); + rte_trace_point_emit_ptr(action); rte_trace_point_emit_ptr(data); rte_trace_point_emit_int(ret); ) @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( const struct rte_flow_indir_action_conf *conf, const struct rte_flow_action *action, const struct rte_flow_action_handle *handle), - uint8_t ingress = conf->ingress; - uint8_t egress = conf->egress; - uint8_t transfer = conf->transfer; - rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u8(ingress); - rte_trace_point_emit_u8(egress); - rte_trace_point_emit_u8(transfer); + rte_trace_point_emit_ptr(conf); rte_trace_point_emit_ptr(action); rte_trace_point_emit_ptr(handle); ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi @ 2023-02-28 11:04 ` Ferruh Yigit 2023-02-28 11:29 ` David Marchand 2023-02-28 15:01 ` Ferruh Yigit 1 sibling, 1 reply; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 11:04 UTC (permalink / raw) To: Ankur Dwivedi, dev, Thomas Monjalon Cc: jerinj, David Marchand, Ali Alnubani, Li, WeiyuanX 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 dereferenced. > > 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 <adwivedi@marvell.com> Hi Ankur, There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=1167 As far as I can see that is caused by '__rte_trace_point_register()' is calling 'register_fn()' [1]. At registering trace point stage, most of the pointers can be invalid, and this can crash other locations too. Why 'register_fn()' called withing the trace point register? Can we remove it? [1] #define RTE_TRACE_POINT_REGISTER(trace, name) RTE_INIT(trace##_init) __rte_trace_point_register(..., (void (*)(void)) trace); __rte_trace_point_register(handle, name, void (*register_fn)(void)) { ... register_fn(); ... } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 11:04 ` Ferruh Yigit @ 2023-02-28 11:29 ` David Marchand 2023-02-28 12:46 ` Ferruh Yigit 0 siblings, 1 reply; 20+ messages in thread From: David Marchand @ 2023-02-28 11:29 UTC (permalink / raw) To: Ferruh Yigit Cc: Ankur Dwivedi, dev, Thomas Monjalon, jerinj, Ali Alnubani, Li, WeiyuanX On Tue, Feb 28, 2023 at 12:05 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: > > 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 dereferenced. > > > > 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 <adwivedi@marvell.com> > > Hi Ankur, > > There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=1167 > > > As far as I can see that is caused by '__rte_trace_point_register()' is > 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. -- David Marchand ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 11:29 ` David Marchand @ 2023-02-28 12:46 ` Ferruh Yigit 2023-02-28 13:17 ` Jerin Jacob 0 siblings, 1 reply; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 12:46 UTC (permalink / raw) To: David Marchand Cc: Ankur Dwivedi, dev, Thomas Monjalon, jerinj, Ali Alnubani, Li, WeiyuanX On 2/28/2023 11:29 AM, David Marchand wrote: > On Tue, Feb 28, 2023 at 12:05 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: >> >> 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 dereferenced. >>> >>> 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 <adwivedi@marvell.com> >> >> Hi Ankur, >> >> There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=1167 >> >> >> As far as I can see that is caused by '__rte_trace_point_register()' is >> 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 12:46 ` Ferruh Yigit @ 2023-02-28 13:17 ` Jerin Jacob 2023-02-28 13:39 ` Ferruh Yigit 0 siblings, 1 reply; 20+ messages in thread From: Jerin Jacob @ 2023-02-28 13:17 UTC (permalink / raw) To: Ferruh Yigit Cc: David Marchand, Ankur Dwivedi, dev, Thomas Monjalon, jerinj, Ali Alnubani, Li, WeiyuanX On Tue, Feb 28, 2023 at 6:16 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: > > On 2/28/2023 11:29 AM, David Marchand wrote: > > On Tue, Feb 28, 2023 at 12:05 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: > >> > >> 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 dereferenced. > >>> > >>> 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 <adwivedi@marvell.com> > >> > >> Hi Ankur, > >> > >> There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=1167 > >> > >> > >> As far as I can see that is caused by '__rte_trace_point_register()' is > >> 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 ‘rte_eth_trace_tx_hairpin_queue_setup’: ../lib/eal/include/rte_trace_point.h:378:21: error: cannot take address of bit-field ‘peer_count’ 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 <rte_per_lcore.h> #include <rte_trace_point.h> +#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 *conf, int ret), - uint16_t peer_count = conf->peer_count; - uint8_t tx_explicit = conf->tx_explicit; - uint8_t manual_bind = conf->manual_bind; - uint8_t use_locked_device_memory = conf->use_locked_device_memory; - uint8_t use_rte_memory = conf->use_rte_memory; - uint8_t force_memory = 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 = conf->peer_count; + tx_explicit = conf->tx_explicit; + manual_bind = conf->manual_bind; + use_locked_device_memory = conf->use_locked_device_memory; + use_rte_memory = conf->use_rte_memory; + force_memory = 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] $ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 13:17 ` Jerin Jacob @ 2023-02-28 13:39 ` Ferruh Yigit 0 siblings, 0 replies; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 13:39 UTC (permalink / raw) To: Jerin Jacob Cc: David Marchand, Ankur Dwivedi, dev, Thomas Monjalon, jerinj, Ali Alnubani, Li, WeiyuanX On 2/28/2023 1:17 PM, Jerin Jacob wrote: > On Tue, Feb 28, 2023 at 6:16 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: >> >> On 2/28/2023 11:29 AM, David Marchand wrote: >>> On Tue, Feb 28, 2023 at 12:05 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote: >>>> >>>> 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 dereferenced. >>>>> >>>>> 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 <adwivedi@marvell.com> >>>> >>>> Hi Ankur, >>>> >>>> There is another bug report: https://bugs.dpdk.org/show_bug.cgi?id=1167 >>>> >>>> >>>> As far as I can see that is caused by '__rte_trace_point_register()' is >>>> 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 to go for option 1, specially at this phase of the release. Only limited number of traces are affected by this bitfield related issue anyway. btw, this is for the Bug 1167, I thought 1167 & 1162 share same root cause but they don't, so 1162 is still open. > > [1] > ../lib/ethdev/ethdev_trace.h: In function > ‘rte_eth_trace_tx_hairpin_queue_setup’: > ../lib/eal/include/rte_trace_point.h:378:21: error: cannot take > address of bit-field ‘peer_count’ > 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 <rte_per_lcore.h> > #include <rte_trace_point.h> > > +#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 *conf, > int ret), > - uint16_t peer_count = conf->peer_count; > - uint8_t tx_explicit = conf->tx_explicit; > - uint8_t manual_bind = conf->manual_bind; > - uint8_t use_locked_device_memory = conf->use_locked_device_memory; > - uint8_t use_rte_memory = conf->use_rte_memory; > - uint8_t force_memory = 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 = conf->peer_count; > + tx_explicit = conf->tx_explicit; > + manual_bind = conf->manual_bind; > + use_locked_device_memory = conf->use_locked_device_memory; > + use_rte_memory = conf->use_rte_memory; > + force_memory = 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] $ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi 2023-02-28 11:04 ` Ferruh Yigit @ 2023-02-28 15:01 ` Ferruh Yigit 2023-02-28 15:40 ` [EXT] " Ankur Dwivedi 1 sibling, 1 reply; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 15:01 UTC (permalink / raw) To: Ankur Dwivedi, dev; +Cc: jerinj 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 dereferenced. > > Coverity issue: 383238 > Bugzilla ID: 1162 > Fixes: 6679cf21d608 ("ethdev: add trace points") > Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") > In below changes, pointers can be NULL at runtime, so agree on to the change, with minor comments please see below. But I don't think this is a fix for Bug 1162, which is an ASan reported error, can you please drop that tag unless it is verified. <...> > @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( > int ret), > rte_trace_point_emit_u16(port_id); > rte_trace_point_emit_ptr(flow); > - rte_trace_point_emit_int(action->type); > - rte_trace_point_emit_ptr(action->conf); > + rte_trace_point_emit_ptr(action); > rte_trace_point_emit_ptr(data); > rte_trace_point_emit_int(ret); I think 'rte_flow_trace_create()' is missed, rest looks OK. Can you please double check 'rte_flow_trace_create()' too? > ) > @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( > const struct rte_flow_indir_action_conf *conf, > const struct rte_flow_action *action, > const struct rte_flow_action_handle *handle), > - uint8_t ingress = conf->ingress; > - uint8_t egress = conf->egress; > - uint8_t transfer = conf->transfer; > - > rte_trace_point_emit_u16(port_id); > - rte_trace_point_emit_u8(ingress); > - rte_trace_point_emit_u8(egress); > - rte_trace_point_emit_u8(transfer); > + rte_trace_point_emit_ptr(conf); > rte_trace_point_emit_ptr(action); > rte_trace_point_emit_ptr(handle); > ) This change is different than others, this is removing bitfield related local variable assignment. According to bug 1167 that is causing a crash. So we need a separate patch to either remove or fix bitfield related issues, for now I am OK to remove (as done above). But can you please make another patch for bitfield issue and move above change to that patch? Thanks, ferruh ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 15:01 ` Ferruh Yigit @ 2023-02-28 15:40 ` Ankur Dwivedi 2023-02-28 16:08 ` Ferruh Yigit 2023-02-28 16:27 ` Ferruh Yigit 0 siblings, 2 replies; 20+ messages in thread From: Ankur Dwivedi @ 2023-02-28 15:40 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: Jerin Jacob Kollanukkaran >---------------------------------------------------------------------- >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 dereferenced. >> >> Coverity issue: 383238 >> Bugzilla ID: 1162 >> Fixes: 6679cf21d608 ("ethdev: add trace points") >> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") >> > >In below changes, pointers can be NULL at runtime, so agree on to the change, >with minor comments please see below. > > >But I don't think this is a fix for Bug 1162, which is an ASan reported error, can >you please drop that tag unless it is verified. The asan error reported in 1162 was because of capturing rte_trace_point_emit_string(parent->bus_info); in rte_eth_trace_find_next_of. I could not find the exact reason for the asan error with parent->bus_info. But I think parent pointer can be NULL in rte_eth_find_next_of, so I removed the pointer reference. This resolved the asan error in 1162 as a side effect. - rte_trace_point_emit_string(parent->name); - rte_trace_point_emit_string(parent->bus_info); - rte_trace_point_emit_int(parent->numa_node); + rte_trace_point_emit_ptr(parent); I will check if I can add an if condition to check if a pointer is NULL inside the trace function. If that works I will update this patch. > ><...> > >> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( >> int ret), >> rte_trace_point_emit_u16(port_id); >> rte_trace_point_emit_ptr(flow); >> - rte_trace_point_emit_int(action->type); >> - rte_trace_point_emit_ptr(action->conf); >> + rte_trace_point_emit_ptr(action); >> rte_trace_point_emit_ptr(data); >> rte_trace_point_emit_int(ret); > >I think 'rte_flow_trace_create()' is missed, rest looks OK. > >Can you please double check 'rte_flow_trace_create()' too? Yes. Will add rte_flow_trace_create. > >> ) >> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( >> const struct rte_flow_indir_action_conf *conf, >> const struct rte_flow_action *action, >> const struct rte_flow_action_handle *handle), >> - uint8_t ingress = conf->ingress; >> - uint8_t egress = conf->egress; >> - uint8_t transfer = conf->transfer; >> - >> rte_trace_point_emit_u16(port_id); >> - rte_trace_point_emit_u8(ingress); >> - rte_trace_point_emit_u8(egress); >> - rte_trace_point_emit_u8(transfer); >> + rte_trace_point_emit_ptr(conf); >> rte_trace_point_emit_ptr(action); >> rte_trace_point_emit_ptr(handle); >> ) > >This change is different than others, this is removing bitfield related local >variable assignment. > >According to bug 1167 that is causing a crash. So we need a separate patch to >either remove or fix bitfield related issues, for now I am OK to remove (as >done above). > >But can you please make another patch for bitfield issue and move above >change to that patch? Yes, will move this change to fix bitfield patch. > >Thanks, >ferruh ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 15:40 ` [EXT] " Ankur Dwivedi @ 2023-02-28 16:08 ` Ferruh Yigit 2023-02-28 16:27 ` Ferruh Yigit 1 sibling, 0 replies; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 16:08 UTC (permalink / raw) To: Ankur Dwivedi, dev; +Cc: Jerin Jacob Kollanukkaran On 2/28/2023 3:40 PM, Ankur Dwivedi wrote: >> ---------------------------------------------------------------------- >> 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 dereferenced. >>> >>> Coverity issue: 383238 >>> Bugzilla ID: 1162 >>> Fixes: 6679cf21d608 ("ethdev: add trace points") >>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") >>> >> >> In below changes, pointers can be NULL at runtime, so agree on to the change, >> with minor comments please see below. >> >> >> But I don't think this is a fix for Bug 1162, which is an ASan reported error, can >> you please drop that tag unless it is verified. > The asan error reported in 1162 was because of capturing rte_trace_point_emit_string(parent->bus_info); in > rte_eth_trace_find_next_of. I could not find the exact reason for the asan error with parent->bus_info. > Yeah, I also looked at it but not able to identify why it is complaining. > But I think parent pointer can be NULL in rte_eth_find_next_of, so I removed the pointer reference. This resolved the asan error in 1162 as a side effect. > Is it confirmed that patch fixing asan issue, I have not seen it in the bugzilla comments. If it is confirmed, OK to keep Bugzilla ID tag. And aside from the asan issue, OK to have this patch, because of reason you described. > - rte_trace_point_emit_string(parent->name); > - rte_trace_point_emit_string(parent->bus_info); > - rte_trace_point_emit_int(parent->numa_node); > + rte_trace_point_emit_ptr(parent); > > I will check if I can add an if condition to check if a pointer is NULL inside the trace function. If that works I will update this patch. >> >> <...> >> >>> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( >>> int ret), >>> rte_trace_point_emit_u16(port_id); >>> rte_trace_point_emit_ptr(flow); >>> - rte_trace_point_emit_int(action->type); >>> - rte_trace_point_emit_ptr(action->conf); >>> + rte_trace_point_emit_ptr(action); >>> rte_trace_point_emit_ptr(data); >>> rte_trace_point_emit_int(ret); >> >> I think 'rte_flow_trace_create()' is missed, rest looks OK. >> >> Can you please double check 'rte_flow_trace_create()' too? > Yes. Will add rte_flow_trace_create. >> >>> ) >>> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( >>> const struct rte_flow_indir_action_conf *conf, >>> const struct rte_flow_action *action, >>> const struct rte_flow_action_handle *handle), >>> - uint8_t ingress = conf->ingress; >>> - uint8_t egress = conf->egress; >>> - uint8_t transfer = conf->transfer; >>> - >>> rte_trace_point_emit_u16(port_id); >>> - rte_trace_point_emit_u8(ingress); >>> - rte_trace_point_emit_u8(egress); >>> - rte_trace_point_emit_u8(transfer); >>> + rte_trace_point_emit_ptr(conf); >>> rte_trace_point_emit_ptr(action); >>> rte_trace_point_emit_ptr(handle); >>> ) >> >> This change is different than others, this is removing bitfield related local >> variable assignment. >> >> According to bug 1167 that is causing a crash. So we need a separate patch to >> either remove or fix bitfield related issues, for now I am OK to remove (as >> done above). >> >> But can you please make another patch for bitfield issue and move above >> change to that patch? > > Yes, will move this change to fix bitfield patch. >> >> Thanks, >> ferruh > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 15:40 ` [EXT] " Ankur Dwivedi 2023-02-28 16:08 ` Ferruh Yigit @ 2023-02-28 16:27 ` Ferruh Yigit 2023-03-02 9:58 ` Ferruh Yigit 1 sibling, 1 reply; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 16:27 UTC (permalink / raw) To: Ankur Dwivedi, dev; +Cc: Jerin Jacob Kollanukkaran On 2/28/2023 3:40 PM, Ankur Dwivedi wrote: >> ---------------------------------------------------------------------- >> 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 dereferenced. >>> >>> Coverity issue: 383238 >>> Bugzilla ID: 1162 >>> Fixes: 6679cf21d608 ("ethdev: add trace points") >>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") >>> >> >> In below changes, pointers can be NULL at runtime, so agree on to the change, >> with minor comments please see below. >> >> >> But I don't think this is a fix for Bug 1162, which is an ASan reported error, can >> you please drop that tag unless it is verified. > The asan error reported in 1162 was because of capturing rte_trace_point_emit_string(parent->bus_info); in > rte_eth_trace_find_next_of. I could not find the exact reason for the asan error with parent->bus_info. > > But I think parent pointer can be NULL in rte_eth_find_next_of, so I removed the pointer reference. This resolved the asan error in 1162 as a side effect. > > - rte_trace_point_emit_string(parent->name); > - rte_trace_point_emit_string(parent->bus_info); > - rte_trace_point_emit_int(parent->numa_node); > + rte_trace_point_emit_ptr(parent); > > I will check if I can add an if condition to check if a pointer is NULL inside the trace function. If that works I will update this patch. >> >> <...> >> >>> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( >>> int ret), >>> rte_trace_point_emit_u16(port_id); >>> rte_trace_point_emit_ptr(flow); >>> - rte_trace_point_emit_int(action->type); >>> - rte_trace_point_emit_ptr(action->conf); >>> + rte_trace_point_emit_ptr(action); >>> rte_trace_point_emit_ptr(data); >>> rte_trace_point_emit_int(ret); >> >> I think 'rte_flow_trace_create()' is missed, rest looks OK. >> >> Can you please double check 'rte_flow_trace_create()' too? > Yes. Will add rte_flow_trace_create. Can you please check 'rte_eth_trace_read_clock()' too, it has: RTE_TRACE_POINT_ARGS(..., const uint64_t *clk, ...) uint64_t clk_v = *clk; >> >>> ) >>> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( >>> const struct rte_flow_indir_action_conf *conf, >>> const struct rte_flow_action *action, >>> const struct rte_flow_action_handle *handle), >>> - uint8_t ingress = conf->ingress; >>> - uint8_t egress = conf->egress; >>> - uint8_t transfer = conf->transfer; >>> - >>> rte_trace_point_emit_u16(port_id); >>> - rte_trace_point_emit_u8(ingress); >>> - rte_trace_point_emit_u8(egress); >>> - rte_trace_point_emit_u8(transfer); >>> + rte_trace_point_emit_ptr(conf); >>> rte_trace_point_emit_ptr(action); >>> rte_trace_point_emit_ptr(handle); >>> ) >> >> This change is different than others, this is removing bitfield related local >> variable assignment. >> >> According to bug 1167 that is causing a crash. So we need a separate patch to >> either remove or fix bitfield related issues, for now I am OK to remove (as >> done above). >> >> But can you please make another patch for bitfield issue and move above >> change to that patch? > > Yes, will move this change to fix bitfield patch. >> >> Thanks, >> ferruh > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-02-28 16:27 ` Ferruh Yigit @ 2023-03-02 9:58 ` Ferruh Yigit 2023-03-02 16:49 ` Ankur Dwivedi 0 siblings, 1 reply; 20+ messages in thread From: Ferruh Yigit @ 2023-03-02 9:58 UTC (permalink / raw) To: Ankur Dwivedi, dev; +Cc: Jerin Jacob Kollanukkaran On 2/28/2023 4:27 PM, Ferruh Yigit wrote: > On 2/28/2023 3:40 PM, Ankur Dwivedi wrote: >>> ---------------------------------------------------------------------- >>> 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 dereferenced. >>>> >>>> Coverity issue: 383238 >>>> Bugzilla ID: 1162 >>>> Fixes: 6679cf21d608 ("ethdev: add trace points") >>>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") >>>> >>> >>> In below changes, pointers can be NULL at runtime, so agree on to the change, >>> with minor comments please see below. >>> >>> >>> But I don't think this is a fix for Bug 1162, which is an ASan reported error, can >>> you please drop that tag unless it is verified. >> The asan error reported in 1162 was because of capturing rte_trace_point_emit_string(parent->bus_info); in >> rte_eth_trace_find_next_of. I could not find the exact reason for the asan error with parent->bus_info. >> >> But I think parent pointer can be NULL in rte_eth_find_next_of, so I removed the pointer reference. This resolved the asan error in 1162 as a side effect. >> >> - rte_trace_point_emit_string(parent->name); >> - rte_trace_point_emit_string(parent->bus_info); >> - rte_trace_point_emit_int(parent->numa_node); >> + rte_trace_point_emit_ptr(parent); >> >> I will check if I can add an if condition to check if a pointer is NULL inside the trace function. If that works I will update this patch. >>> >>> <...> >>> >>>> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( >>>> int ret), >>>> rte_trace_point_emit_u16(port_id); >>>> rte_trace_point_emit_ptr(flow); >>>> - rte_trace_point_emit_int(action->type); >>>> - rte_trace_point_emit_ptr(action->conf); >>>> + rte_trace_point_emit_ptr(action); >>>> rte_trace_point_emit_ptr(data); >>>> rte_trace_point_emit_int(ret); >>> >>> I think 'rte_flow_trace_create()' is missed, rest looks OK. >>> >>> Can you please double check 'rte_flow_trace_create()' too? >> Yes. Will add rte_flow_trace_create. > > > Can you please check 'rte_eth_trace_read_clock()' too, > it has: > RTE_TRACE_POINT_ARGS(..., const uint64_t *clk, ...) > uint64_t clk_v = *clk; > Hi Ankur, It seems bug 1162 is verified with this patch, can you please send a v2 so we can close the defect. Thanks, ferruh >>> >>>> ) >>>> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( >>>> const struct rte_flow_indir_action_conf *conf, >>>> const struct rte_flow_action *action, >>>> const struct rte_flow_action_handle *handle), >>>> - uint8_t ingress = conf->ingress; >>>> - uint8_t egress = conf->egress; >>>> - uint8_t transfer = conf->transfer; >>>> - >>>> rte_trace_point_emit_u16(port_id); >>>> - rte_trace_point_emit_u8(ingress); >>>> - rte_trace_point_emit_u8(egress); >>>> - rte_trace_point_emit_u8(transfer); >>>> + rte_trace_point_emit_ptr(conf); >>>> rte_trace_point_emit_ptr(action); >>>> rte_trace_point_emit_ptr(handle); >>>> ) >>> >>> This change is different than others, this is removing bitfield related local >>> variable assignment. >>> >>> According to bug 1167 that is causing a crash. So we need a separate patch to >>> either remove or fix bitfield related issues, for now I am OK to remove (as >>> done above). >>> >>> But can you please make another patch for bitfield issue and move above >>> change to that patch? >> >> Yes, will move this change to fix bitfield patch. >>> >>> Thanks, >>> ferruh >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference 2023-03-02 9:58 ` Ferruh Yigit @ 2023-03-02 16:49 ` Ankur Dwivedi 0 siblings, 0 replies; 20+ messages in thread From: Ankur Dwivedi @ 2023-03-02 16:49 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: Jerin Jacob Kollanukkaran >On 2/28/2023 4:27 PM, Ferruh Yigit wrote: >> On 2/28/2023 3:40 PM, Ankur Dwivedi wrote: >>>> -------------------------------------------------------------------- >>>> -- 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 >dereferenced. >>>>> >>>>> Coverity issue: 383238 >>>>> Bugzilla ID: 1162 >>>>> Fixes: 6679cf21d608 ("ethdev: add trace points") >>>>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow") >>>>> >>>> >>>> In below changes, pointers can be NULL at runtime, so agree on to >>>> the change, with minor comments please see below. >>>> >>>> >>>> But I don't think this is a fix for Bug 1162, which is an ASan >>>> reported error, can you please drop that tag unless it is verified. >>> The asan error reported in 1162 was because of capturing >>> rte_trace_point_emit_string(parent->bus_info); in >rte_eth_trace_find_next_of. I could not find the exact reason for the asan >error with parent->bus_info. >>> >>> But I think parent pointer can be NULL in rte_eth_find_next_of, so I >removed the pointer reference. This resolved the asan error in 1162 as a side >effect. >>> >>> - rte_trace_point_emit_string(parent->name); >>> - rte_trace_point_emit_string(parent->bus_info); >>> - rte_trace_point_emit_int(parent->numa_node); >>> + rte_trace_point_emit_ptr(parent); >>> >>> I will check if I can add an if condition to check if a pointer is NULL inside the >trace function. If that works I will update this patch. >>>> >>>> <...> >>>> >>>>> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP( >>>>> int ret), >>>>> rte_trace_point_emit_u16(port_id); >>>>> rte_trace_point_emit_ptr(flow); >>>>> - rte_trace_point_emit_int(action->type); >>>>> - rte_trace_point_emit_ptr(action->conf); >>>>> + rte_trace_point_emit_ptr(action); >>>>> rte_trace_point_emit_ptr(data); >>>>> rte_trace_point_emit_int(ret); >>>> >>>> I think 'rte_flow_trace_create()' is missed, rest looks OK. >>>> >>>> Can you please double check 'rte_flow_trace_create()' too? >>> Yes. Will add rte_flow_trace_create. >> >> >> Can you please check 'rte_eth_trace_read_clock()' too, it has: >> RTE_TRACE_POINT_ARGS(..., const uint64_t *clk, ...) uint64_t clk_v = >> *clk; >> > >Hi Ankur, > >It seems bug 1162 is verified with this patch, can you please send a v2 >so we can close the defect. Sure, will send a v2. > >Thanks, >ferruh > >>>> >>>>> ) >>>>> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP( >>>>> const struct rte_flow_indir_action_conf *conf, >>>>> const struct rte_flow_action *action, >>>>> const struct rte_flow_action_handle *handle), >>>>> - uint8_t ingress = conf->ingress; >>>>> - uint8_t egress = conf->egress; >>>>> - uint8_t transfer = conf->transfer; >>>>> - >>>>> rte_trace_point_emit_u16(port_id); >>>>> - rte_trace_point_emit_u8(ingress); >>>>> - rte_trace_point_emit_u8(egress); >>>>> - rte_trace_point_emit_u8(transfer); >>>>> + rte_trace_point_emit_ptr(conf); >>>>> rte_trace_point_emit_ptr(action); >>>>> rte_trace_point_emit_ptr(handle); >>>>> ) >>>> >>>> This change is different than others, this is removing bitfield related local >>>> variable assignment. >>>> >>>> According to bug 1167 that is causing a crash. So we need a separate >patch to >>>> either remove or fix bitfield related issues, for now I am OK to remove (as >>>> done above). >>>> >>>> But can you please make another patch for bitfield issue and move above >>>> change to that patch? >>> >>> Yes, will move this change to fix bitfield patch. >>>> >>>> Thanks, >>>> ferruh >>> >> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v1 2/2] ethdev: pass structure pointer 2023-02-23 12:30 [PATCH v1 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi @ 2023-02-23 12:30 ` Ankur Dwivedi 2023-02-28 15:01 ` Ferruh Yigit 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi 2 siblings, 1 reply; 20+ messages in thread From: Ankur Dwivedi @ 2023-02-23 12:30 UTC (permalink / raw) To: dev; +Cc: jerinj, Ankur Dwivedi The rte_eth_xstat_name structure is of size 64 bytes. Instead of passing the structure as value it is passed as a pointer, to avoid copy of 64 bytes in function call stack. Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com> --- lib/ethdev/ethdev_trace.h | 4 ++-- lib/ethdev/rte_ethdev.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h index a13e33fe64..7518c902d1 100644 --- a/lib/ethdev/ethdev_trace.h +++ b/lib/ethdev/ethdev_trace.h @@ -551,11 +551,11 @@ RTE_TRACE_POINT( RTE_TRACE_POINT( rte_eth_trace_xstats_get_names, RTE_TRACE_POINT_ARGS(uint16_t port_id, int i, - struct rte_eth_xstat_name xstats_names, + const struct rte_eth_xstat_name *xstats_names, unsigned int size, int cnt_used_entries), rte_trace_point_emit_u16(port_id); rte_trace_point_emit_int(i); - rte_trace_point_emit_string(xstats_names.name); + rte_trace_point_emit_string(xstats_names->name); rte_trace_point_emit_u32(size); rte_trace_point_emit_int(cnt_used_entries); ) diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index 0266cc82ac..3b07e6feb8 100644 --- a/lib/ethdev/rte_ethdev.c +++ b/lib/ethdev/rte_ethdev.c @@ -3260,7 +3260,7 @@ rte_eth_xstats_get_names(uint16_t port_id, } for (i = 0; i < cnt_used_entries; i++) - rte_eth_trace_xstats_get_names(port_id, i, xstats_names[i], + rte_eth_trace_xstats_get_names(port_id, i, &xstats_names[i], size, cnt_used_entries); return cnt_used_entries; -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1 2/2] ethdev: pass structure pointer 2023-02-23 12:30 ` [PATCH v1 2/2] ethdev: pass structure pointer Ankur Dwivedi @ 2023-02-28 15:01 ` Ferruh Yigit 0 siblings, 0 replies; 20+ messages in thread From: Ferruh Yigit @ 2023-02-28 15:01 UTC (permalink / raw) To: Ankur Dwivedi, dev; +Cc: jerinj On 2/23/2023 12:30 PM, Ankur Dwivedi wrote: > The rte_eth_xstat_name structure is of size 64 bytes. Instead of passing > the structure as value it is passed as a pointer, to avoid copy of 64 bytes > in function call stack. > > Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com> > --- > lib/ethdev/ethdev_trace.h | 4 ++-- > lib/ethdev/rte_ethdev.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h > index a13e33fe64..7518c902d1 100644 > --- a/lib/ethdev/ethdev_trace.h > +++ b/lib/ethdev/ethdev_trace.h > @@ -551,11 +551,11 @@ RTE_TRACE_POINT( > RTE_TRACE_POINT( > rte_eth_trace_xstats_get_names, > RTE_TRACE_POINT_ARGS(uint16_t port_id, int i, > - struct rte_eth_xstat_name xstats_names, > + const struct rte_eth_xstat_name *xstats_names, > unsigned int size, int cnt_used_entries), > rte_trace_point_emit_u16(port_id); > rte_trace_point_emit_int(i); > - rte_trace_point_emit_string(xstats_names.name); > + rte_trace_point_emit_string(xstats_names->name); > rte_trace_point_emit_u32(size); > rte_trace_point_emit_int(cnt_used_entries); > ) > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 0266cc82ac..3b07e6feb8 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -3260,7 +3260,7 @@ rte_eth_xstats_get_names(uint16_t port_id, > } > > for (i = 0; i < cnt_used_entries; i++) > - rte_eth_trace_xstats_get_names(port_id, i, xstats_names[i], > + rte_eth_trace_xstats_get_names(port_id, i, &xstats_names[i], > size, cnt_used_entries); > > return cnt_used_entries; Acked-by: Ferruh Yigit <ferruh.yigit@amd.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 0/2] bug fix in ethdev trace 2023-02-23 12:30 [PATCH v1 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi 2023-02-23 12:30 ` [PATCH v1 2/2] ethdev: pass structure pointer Ankur Dwivedi @ 2023-03-03 11:31 ` Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 1/2] ethdev: fix null pointer dereference Ankur Dwivedi ` (2 more replies) 2 siblings, 3 replies; 20+ messages in thread From: Ankur Dwivedi @ 2023-03-03 11:31 UTC (permalink / raw) To: dev Cc: weiyuanx.li, ferruh.yigit, jerinj, david.marchand, thomas, yux.jiang, dukaix.yuan, Ankur Dwivedi The first patch in this series adds fix for bug and coverity. The second patch makes change to pass structure pointer instead of the structure value, to avoid 64 bytes copy in function call stack for rte_eth_trace_xstats_get_names. v2: - Removed null pointer reference in rte_eth_trace_read_clock and rte_flow_trace_create. - Added Ack by Ferruh in patch (2/2). Ankur Dwivedi (2): ethdev: fix null pointer dereference ethdev: pass structure pointer lib/ethdev/ethdev_trace.h | 32 ++++++++++---------------------- lib/ethdev/rte_ethdev.c | 2 +- 2 files changed, 11 insertions(+), 23 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] ethdev: fix null pointer dereference 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi @ 2023-03-03 11:31 ` Ankur Dwivedi 2023-03-03 13:38 ` Ferruh Yigit 2023-03-03 11:31 ` [PATCH v2 2/2] ethdev: pass structure pointer Ankur Dwivedi 2023-03-03 13:39 ` [PATCH v2 0/2] bug fix in ethdev trace Ferruh Yigit 2 siblings, 1 reply; 20+ messages in thread From: Ankur Dwivedi @ 2023-03-03 11:31 UTC (permalink / raw) To: dev Cc: weiyuanx.li, ferruh.yigit, jerinj, david.marchand, thomas, yux.jiang, dukaix.yuan, Ankur Dwivedi 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 dereferenced. As a result of this fix the address sanitizer error observed with rte_eth_trace_find_next_of() is also resolved. 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 <adwivedi@marvell.com> --- lib/ethdev/ethdev_trace.h | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h index 6c2a68216f..bfcb024ac1 100644 --- a/lib/ethdev/ethdev_trace.h +++ b/lib/ethdev/ethdev_trace.h @@ -123,8 +123,7 @@ RTE_TRACE_POINT( RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct rte_eth_dev_owner *owner, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u64(owner->id); - rte_trace_point_emit_string(owner->name); + rte_trace_point_emit_ptr(owner); rte_trace_point_emit_int(ret); ) @@ -351,9 +350,7 @@ RTE_TRACE_POINT( rte_eth_trace_find_next_of, RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct rte_device *parent), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_string(parent->name); - rte_trace_point_emit_string(parent->bus_info); - rte_trace_point_emit_int(parent->numa_node); + rte_trace_point_emit_ptr(parent); ) RTE_TRACE_POINT( @@ -831,8 +828,7 @@ RTE_TRACE_POINT( const struct rte_eth_fec_capa *speed_fec_capa, unsigned int num, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u32(speed_fec_capa->speed); - rte_trace_point_emit_u32(speed_fec_capa->capa); + rte_trace_point_emit_ptr(speed_fec_capa); rte_trace_point_emit_u32(num); rte_trace_point_emit_int(ret); ) @@ -1135,10 +1131,8 @@ RTE_TRACE_POINT( RTE_TRACE_POINT( rte_eth_trace_read_clock, RTE_TRACE_POINT_ARGS(uint16_t port_id, const uint64_t *clk, int ret), - uint64_t clk_v = *clk; - rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u64(clk_v); + rte_trace_point_emit_ptr(clk); rte_trace_point_emit_int(ret); ) @@ -1378,8 +1372,7 @@ RTE_TRACE_POINT( const struct rte_flow_item *pattern, const struct rte_flow_action *actions, int ret), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u32(attr->group); - rte_trace_point_emit_u32(attr->priority); + rte_trace_point_emit_ptr(attr); rte_trace_point_emit_ptr(pattern); rte_trace_point_emit_ptr(actions); rte_trace_point_emit_int(ret); @@ -1472,10 +1465,7 @@ RTE_TRACE_POINT( const struct rte_flow_item_flex_conf *conf, const struct rte_flow_item_flex_handle *handle), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_int(conf->tunnel); - rte_trace_point_emit_int(conf->nb_samples); - rte_trace_point_emit_int(conf->nb_inputs); - rte_trace_point_emit_int(conf->nb_outputs); + rte_trace_point_emit_ptr(conf); rte_trace_point_emit_ptr(handle); ) @@ -2216,8 +2206,7 @@ RTE_TRACE_POINT_FP( const struct rte_flow_action *actions, const struct rte_flow *flow), rte_trace_point_emit_u16(port_id); - rte_trace_point_emit_u32(attr->group); - rte_trace_point_emit_u32(attr->priority); + rte_trace_point_emit_ptr(attr); rte_trace_point_emit_ptr(pattern); rte_trace_point_emit_ptr(actions); rte_trace_point_emit_ptr(flow); @@ -2240,8 +2229,7 @@ RTE_TRACE_POINT_FP( int ret), rte_trace_point_emit_u16(port_id); rte_trace_point_emit_ptr(flow); - rte_trace_point_emit_int(action->type); - rte_trace_point_emit_ptr(action->conf); + rte_trace_point_emit_ptr(action); rte_trace_point_emit_ptr(data); rte_trace_point_emit_int(ret); ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] ethdev: fix null pointer dereference 2023-03-03 11:31 ` [PATCH v2 1/2] ethdev: fix null pointer dereference Ankur Dwivedi @ 2023-03-03 13:38 ` Ferruh Yigit 0 siblings, 0 replies; 20+ messages in thread From: Ferruh Yigit @ 2023-03-03 13:38 UTC (permalink / raw) To: Ankur Dwivedi, dev Cc: weiyuanx.li, jerinj, david.marchand, thomas, yux.jiang, dukaix.yuan On 3/3/2023 11:31 AM, 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 dereferenced. > As a result of this fix the address sanitizer error observed with > rte_eth_trace_find_next_of() is also resolved. > > 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 <adwivedi@marvell.com> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] ethdev: pass structure pointer 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 1/2] ethdev: fix null pointer dereference Ankur Dwivedi @ 2023-03-03 11:31 ` Ankur Dwivedi 2023-03-03 13:39 ` [PATCH v2 0/2] bug fix in ethdev trace Ferruh Yigit 2 siblings, 0 replies; 20+ messages in thread From: Ankur Dwivedi @ 2023-03-03 11:31 UTC (permalink / raw) To: dev Cc: weiyuanx.li, ferruh.yigit, jerinj, david.marchand, thomas, yux.jiang, dukaix.yuan, Ankur Dwivedi The rte_eth_xstat_name structure is of size 64 bytes. Instead of passing the structure as value it is passed as a pointer, to avoid copy of 64 bytes in function call stack. Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com> --- lib/ethdev/ethdev_trace.h | 4 ++-- lib/ethdev/rte_ethdev.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h index bfcb024ac1..c57ed08d36 100644 --- a/lib/ethdev/ethdev_trace.h +++ b/lib/ethdev/ethdev_trace.h @@ -513,11 +513,11 @@ RTE_TRACE_POINT( RTE_TRACE_POINT( rte_eth_trace_xstats_get_names, RTE_TRACE_POINT_ARGS(uint16_t port_id, int i, - struct rte_eth_xstat_name xstats_names, + const struct rte_eth_xstat_name *xstats_names, unsigned int size, int cnt_used_entries), rte_trace_point_emit_u16(port_id); rte_trace_point_emit_int(i); - rte_trace_point_emit_string(xstats_names.name); + rte_trace_point_emit_string(xstats_names->name); rte_trace_point_emit_u32(size); rte_trace_point_emit_int(cnt_used_entries); ) diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index 0266cc82ac..3b07e6feb8 100644 --- a/lib/ethdev/rte_ethdev.c +++ b/lib/ethdev/rte_ethdev.c @@ -3260,7 +3260,7 @@ rte_eth_xstats_get_names(uint16_t port_id, } for (i = 0; i < cnt_used_entries; i++) - rte_eth_trace_xstats_get_names(port_id, i, xstats_names[i], + rte_eth_trace_xstats_get_names(port_id, i, &xstats_names[i], size, cnt_used_entries); return cnt_used_entries; -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] bug fix in ethdev trace 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 1/2] ethdev: fix null pointer dereference Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 2/2] ethdev: pass structure pointer Ankur Dwivedi @ 2023-03-03 13:39 ` Ferruh Yigit 2 siblings, 0 replies; 20+ messages in thread From: Ferruh Yigit @ 2023-03-03 13:39 UTC (permalink / raw) To: Ankur Dwivedi, dev Cc: weiyuanx.li, jerinj, david.marchand, thomas, yux.jiang, dukaix.yuan On 3/3/2023 11:31 AM, Ankur Dwivedi wrote: > The first patch in this series adds fix for bug and coverity. > The second patch makes change to pass structure pointer instead of the > structure value, to avoid 64 bytes copy in function call stack for > rte_eth_trace_xstats_get_names. > > v2: > - Removed null pointer reference in rte_eth_trace_read_clock and > rte_flow_trace_create. > - Added Ack by Ferruh in patch (2/2). > > Ankur Dwivedi (2): > ethdev: fix null pointer dereference > ethdev: pass structure pointer Series applied to dpdk-next-net/main, thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-03-03 13:39 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-23 12:30 [PATCH v1 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-02-23 12:30 ` [PATCH v1 1/2] ethdev: fix null pointer dereference Ankur Dwivedi 2023-02-28 11:04 ` Ferruh Yigit 2023-02-28 11:29 ` David Marchand 2023-02-28 12:46 ` Ferruh Yigit 2023-02-28 13:17 ` Jerin Jacob 2023-02-28 13:39 ` Ferruh Yigit 2023-02-28 15:01 ` Ferruh Yigit 2023-02-28 15:40 ` [EXT] " Ankur Dwivedi 2023-02-28 16:08 ` Ferruh Yigit 2023-02-28 16:27 ` Ferruh Yigit 2023-03-02 9:58 ` Ferruh Yigit 2023-03-02 16:49 ` Ankur Dwivedi 2023-02-23 12:30 ` [PATCH v1 2/2] ethdev: pass structure pointer Ankur Dwivedi 2023-02-28 15:01 ` Ferruh Yigit 2023-03-03 11:31 ` [PATCH v2 0/2] bug fix in ethdev trace Ankur Dwivedi 2023-03-03 11:31 ` [PATCH v2 1/2] ethdev: fix null pointer dereference Ankur Dwivedi 2023-03-03 13:38 ` Ferruh Yigit 2023-03-03 11:31 ` [PATCH v2 2/2] ethdev: pass structure pointer Ankur Dwivedi 2023-03-03 13:39 ` [PATCH v2 0/2] bug fix in ethdev trace Ferruh Yigit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.