Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower()

[Roi Dayan <roid@nvidia.com>]

On 2021-03-09 10:20 AM, Roi Dayan wrote:
> 
> 
> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote:
>> When mlx5e_tc_get_counter() returns NULL to counter or
>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return
>> code of mlx5e_stats_flower() is assigned.
>> To fix this bug, err is assigned with -EINVAL in these cases.
>>
>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c 
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index 0da69b98f38f..1f2c9da7bd35 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, 
>> struct mlx5e_priv *priv,
>>       if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) {
>>           counter = mlx5e_tc_get_counter(flow);
>> -        if (!counter)
>> +        if (!counter) {
>> +            err = -EINVAL;
>>               goto errout;
>> +        }
>>           mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
>>       }
>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, 
>> struct mlx5e_priv *priv,
>>        * un-offloaded while the other rule is offloaded.
>>        */
>>       peer_esw = mlx5_devcom_get_peer_data(devcom, 
>> MLX5_DEVCOM_ESW_OFFLOADS);
>> -    if (!peer_esw)
>> +    if (!peer_esw) {
>> +        err = -EINVAL;
> 
> note here it's not an error. it could be there is no peer esw
> so just continue with the stats update.
> 
>>           goto out;
>> +    }
>>       if (flow_flag_test(flow, DUP) &&
>>           flow_flag_test(flow->peer_flow, OFFLOADED)) {
>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, 
>> struct mlx5e_priv *priv,
>>           u64 lastuse2;
>>           counter = mlx5e_tc_get_counter(flow->peer_flow);
>> -        if (!counter)
>> +        if (!counter) {
>> +            err = -EINVAL;

this change is problematic. the current goto is to do stats update with
the first counter stats we got but if you now want to return an error
then you probably should not do any update at all.

>>               goto no_peer_counter;
>> +        }
>>           mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2);
>>           bytes += bytes2;
>>
> 
>