Re: [PATCH v1 1/2] ethdev: fix null pointer dereference

[Ferruh Yigit <ferruh.yigit@amd.com>]

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