linux-kernel - Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1fdf71d7-7a54-0b69-3339-e792dd03b2c8@nvidia.com>
Date:   Tue, 9 Mar 2021 10:24:45 +0200
From:   Roi Dayan <roid@...dia.com>
To:     Jia-Ju Bai <baijiaju1990@...il.com>, <saeedm@...dia.com>,
        <leon@...nel.org>, <davem@...emloft.net>, <kuba@...nel.org>
CC:     <netdev@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: mellanox: mlx5: fix error return code of
 mlx5e_stats_flower()



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@...nghua.edu.cn>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@...il.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;
>>
> 
>