Received-SPF: pass (google.com: domain of linux-kernel+bounces-201429-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <2b09c707-208c-43ea-870a-1c637ca8ed53@intel.com>
Date: Tue, 4 Jun 2024 14:58:22 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net] net/mlx5: Fix tainted pointer delete is case of flow
 rules creation fail
To: Aleksandr Mishin <amishin@t-argos.ru>, Mark Bloch <mbloch@nvidia.com>
CC: Saeed Mahameed <saeedm@nvidia.com>, Leon Romanovsky <leon@kernel.org>,
	Tariq Toukan <tariqt@nvidia.com>, "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, "Paolo
 Abeni" <pabeni@redhat.com>, Maor Gottlieb <maorg@nvidia.com>, Shay Drory
	<shayd@nvidia.com>, Jianbo Liu <jianbol@nvidia.com>, Jinjie Ruan
	<ruanjinjie@huawei.com>, <netdev@vger.kernel.org>,
	<linux-rdma@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<lvc-project@linuxtesting.org>
References: <20240604100552.25201-1-amishin@t-argos.ru>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20240604100552.25201-1-amishin@t-argos.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eUtLaEJSVXFNaUdRUkppZE5MS2RPT25icERTYVh2ajA4ZzFLdHprR2ZWTmFZ?=
 =?utf-8?B?NkYyRi9BZlc0SVBtSXUyMWtmU2dQdk5LMGdjWXpSYlN5TVV3YUFkUEZ0WDJE?=
 =?utf-8?B?UDNCcXZwYWZvWVFVTk52Y01SbGxjRVFUY0FsbmpPQzJZNWkyMnpKdVorbXA3?=
 =?utf-8?B?R0Q4NVBCemFwOWRiUU0vM2tMM1hZS3NYeERLdFFUQ01PSkxXWWNzQmdUM3gx?=
 =?utf-8?B?dU1KaUZNZ04rdkJ6WVkvSnZsSk9iOVZKRGsvaVlnNGwzSHFITG9ZSmdJS3hF?=
 =?utf-8?B?N1lYR3JhekIvMWpNSEF3UGE0bTJUNTRjRVhEYVVjd2ZJU0dMWU1ObnpTUTho?=
 =?utf-8?B?WUNoclUwUTZDRlVXUDRqTGY0ekkwRXdLZkFqOGdOdFZsSFlpL2J3aWJWaklX?=
 =?utf-8?B?Ry9JeU9yd0ZPdkN2VUluaVhLU1Jqam1acXEwK20zOG9TelBuWEJ2Ykd3STZR?=
 =?utf-8?B?R25PcFlHSGhCTVpVNW9Gbjl5anJYbTN1S2FiS1JFbmdFVVlURGxrTTh3QVox?=
 =?utf-8?B?Vzc1a3Z1alh1UXgvMVJPVnFneXRrcXVnZVJ1Vm44OXZKT1RHVmhaNTJOV0lG?=
 =?utf-8?B?aU0rK3pSTHV2ZWlpc2QwQnUxMGg0bjJFTTVIWHRqVVQ0R0FOeXpvOVVXT2VX?=
 =?utf-8?B?SkFqVUdxWitMcEFna0IzMjNDRlRWcFJrUisvSUtVT0ZwYTJyalA0blBmeVQ2?=
 =?utf-8?B?RkRjNWpmZEdrSDBkaTE1aW1LQ2R1Rk1CajJPanJuUU1UeUp2ZFNqeEZGcEVu?=
 =?utf-8?B?dnV0K2hTbEhadHJUUVd0M0lWRFF2UGxEbGpFSUN3WUdxZHRDTlJEOWFuS2Jp?=
 =?utf-8?B?VG9OemN0dWtJb2ZXRzR5dXc0ODhpUDFWa0NpKzhwd0l2LzhwK1ZpMzhPUVEr?=
 =?utf-8?B?NXZzTlpiVllydmR5YmUvQjRuVGtBc2ZVNG4yMys2WElvNkg5Rk84dGNPWXN6?=
 =?utf-8?B?d1dPOXE0TWNzVWJpczYzaDUwblBVNFdmb3BxL2ltazBrWjU3dW9za1IwVFVi?=
 =?utf-8?B?L1pHUEgyRjBybHVJTGVBL2JOUjgzVkhXSGxBTkFkYmF5bXhxd2JVbFlMMmpQ?=
 =?utf-8?B?MFE2TEdvWGhPbnFlZzYwcjVUb0h6UDNFZ1dBSTIvWldHbzMxT1l6OEhHc3RD?=
 =?utf-8?B?Sjl3TkZscjlLRWdZdzlkOU9xZ2cxL2pRajJKZXRpTloyb0lEMUo5dFAySWU4?=
 =?utf-8?B?REh3M2RNS2JCTUE0T2lDblVhbnpvNzI1L2dJeEZJTGY0SmRuam9nTkJtL2cv?=
 =?utf-8?B?QzF2QnQrWjBpQVpWRVFJdFNIMnoybTJqYS91dExRZW9NS0JnaXgvd2ZrSGM0?=
 =?utf-8?B?K1NPa2lCOGZCTWRWMUFMYU1ocndDVXZBYUp6enNPaEZJVk9iaUN3M2ZNdFN5?=
 =?utf-8?B?bkwyRDkrRWRKZzM4Vmc0K05zZkxhSTB1Qmd4UUpFQkpGQkhzMG5ubEU3bHhy?=
 =?utf-8?B?OXI4TTBHTXJYbXVIMUhCbXlvMzNnS2QrSWNvQmxCZVE0MmtKeW54NzJWTlBW?=
 =?utf-8?B?RjFYcE5zUEg4OTU3WDhrTDhwRlV4QzlHOXkzWjg3YytHdlhiNHZHZlRsMUhk?=
 =?utf-8?B?Zk8rcXJRazIzUEYveElQNGhLNEJ5T3NFWXFxY1lLNDBDWEJKbVYyVWlqRmh2?=
 =?utf-8?B?emxxSmNuSUhha2l1eWgybng4cjRYcmhlNUNrM2pKZVBwTFVpOTE3dng3eVRm?=
 =?utf-8?B?Mlc4QjR5dkJMTlBSaGVaL0hkWjZjNFJYQkJpd2pvWWxMcStiR0h3TFExSWNn?=
 =?utf-8?B?VGV4T1gzWGo3Zzl0RGVzUTZKVHc1ZHNDV3pGREFGbWRJcnhETm1uaENtdUJi?=
 =?utf-8?B?N3ZadXlmQUNhTDdjQVR6MDZVdGpKak01NUx2d3pheURJVGRraWJGdXZqUldR?=
 =?utf-8?B?eVJET29ZSmhtNFlDaGZpOCt0MWo5QkFTajd0TUtYaDFScTV5Z2FvajNqZVZL?=
 =?utf-8?B?NDljcytYeE5mc0U2V0R4WXZvZ1V6TFg1T3RrN0R3SHNUNjNJSTJzS3h4WlNm?=
 =?utf-8?B?OVZhY3NJVzJDbTF0VW9OaGhQS3dWekRMK0N0S0o2ZXh6bzVFYWtkTVlrVmFp?=
 =?utf-8?B?ZnFLa3ZQNzZtQWtNTkxXTDVnZ1l3eEMra251eGphdEZvYS8wYURwSzBqQ0xV?=
 =?utf-8?B?MTA5ZmlZYnc2cEFnQU5jclpxR3hxRHlzUlJnbVF2byt0bzFUS2ZzalNKbEFM?=
 =?utf-8?B?WXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 41bfea1c-c5a7-4379-2af4-08dc84e17575
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 21:58:25.3731
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RBj4vpKIew8BVbbP4ZYF6dCL6zKyvPxPNdsry7wcocwt5WjtSnLsNFeUwpBAPRQmr9MHk9KErnjONWKYkOeCg3QvCQ2IPCPmu5l7ribs//I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6819
X-OriginatorOrg: intel.com


On 6/4/2024 3:05 AM, Aleksandr Mishin wrote:
> In case of flow rule creation fail in mlx5_lag_create_port_sel_table(),
> instead of previously created rules, the tainted pointer is deleted
> deveral times.
> Fix this bug by using correct flow rules pointers.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 352899f384d4 ("net/mlx5: Lag, use buckets in hash mode")
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/lag/port_sel.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/port_sel.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/port_sel.c
> index c16b462ddedf..ab2717012b79 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/port_sel.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/port_sel.c
> @@ -88,9 +88,13 @@ static int mlx5_lag_create_port_sel_table(struct mlx5_lag *ldev,
>  								      &dest, 1);
>  			if (IS_ERR(lag_definer->rules[idx])) {
>  				err = PTR_ERR(lag_definer->rules[idx]);
> -				while (i--)
> -					while (j--)
> +				do {
> +					while (j--) {
> +						idx = i * ldev->buckets + j;
>  						mlx5_del_flow_rules(lag_definer->rules[idx]);
> +					}
> +					j = ldev->buckets;
> +				} while (i--);

So, before the code was:

while (i--)
    while (j--)
        mlx5_del_flow_rules(lag_definer->rules[idx]);

That just calls mlx5_del_flow_rules a bunch of times but keeps using the
wrong index, which is obviously wrong.

The new fix is to calculate the index properly (hence the switch to a do
{ } while (0) loop, so that we properly delete all of the older rules
rather than calling mlx5_del_flow_rules multiple times on the wrong index.

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  				goto destroy_fg;
>  			}
>  		}