Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH] net: mellanox: mlx5: fix error return code of
 mlx5e_stats_flower()
From:   Roi Dayan <roid@nvidia.com>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>, <saeedm@nvidia.com>,
        <leon@kernel.org>, <davem@davemloft.net>, <kuba@kernel.org>
CC:     <netdev@vger.kernel.org>, <linux-rdma@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20210306134718.17566-1-baijiaju1990@gmail.com>
 <99807217-c2a3-928f-4c8c-2195f3500594@nvidia.com>
Message-ID: <1fdf71d7-7a54-0b69-3339-e792dd03b2c8@nvidia.com>
Date:   Tue, 9 Mar 2021 10:24:45 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <99807217-c2a3-928f-4c8c-2195f3500594@nvidia.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2021 08:24:49.8963
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: fa327291-2188-4448-108d-08d8e2d4ce9a
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT051.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3477
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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