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