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

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

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
>