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

[Ankur Dwivedi <adwivedi@marvell.com>]

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