netdev - Re: [net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

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: <Y4WdzWNVcLMvsYyF@fedora>
Date:   Mon, 28 Nov 2022 21:51:09 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
        Tariq Toukan <tariqt@...dia.com>, Chris Mi <cmi@...dia.com>,
        Roi Dayan <roid@...dia.com>, Mark Bloch <mbloch@...dia.com>,
        Vlad Buslov <vladbu@...dia.com>
Subject: Re: [net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

On 28 Nov 15:23, Jacob Keller wrote:
>
>
>On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
>>From: Chris Mi <cmi@...dia.com>
>>
>>If creating bond first and then enabling sriov in switchdev mode,
>>will hit the following syndrome:
>>
>>mlx5_core 0000:08:00.0: mlx5_cmd_out_err:778:(pid 25543): CREATE_LAG(0x840) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x7d49cb), err(-22)
>>
>>The reason is because the offending patch removes eswitch mode
>>none. In vf lag, the checking of eswitch mode none is replaced
>>by checking if sriov is enabled. But when driver enables sriov,
>>it triggers the bond workqueue task first and then setting sriov
>>number in pci_enable_sriov(). So the check fails.
>>
>>Fix it by checking if sriov is enabled using eswitch internal
>>counter that is set before triggering the bond workqueue task.
>>
>>Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
>>Signed-off-by: Chris Mi <cmi@...dia.com>
>>Reviewed-by: Roi Dayan <roid@...dia.com>
>>Reviewed-by: Mark Bloch <mbloch@...dia.com>
>>Reviewed-by: Vlad Buslov <vladbu@...dia.com>
>>Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
>>---
>>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 8 ++++++++
>>  drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 5 +++--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>

[...]

>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>index be1307a63e6d..4070dc1d17cb 100644
>>--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>@@ -701,8 +701,9 @@ static bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
>>  #ifdef CONFIG_MLX5_ESWITCH
>>  	dev = ldev->pf[MLX5_LAG_P1].dev;
>>-	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev))
>>-		return false;
>>+	for (i = 0; i  < ldev->ports; i++)
>>+		if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
>>+			return false;
>
>Am I missing something? whats with the for loop iterator here? i isn't 
>used or passed into these functions?
>
>Do you need to check multiple times or do these functions have some 
>side effect? But looking at their implementation neither of them 
>appear to have side effects?
>
>What am I missing?

Great catch! it's a copy/paste bug, here we need to grab each port's
eswitch on every iteration.

something like:
for (i = 0; i  < ldev->ports; i++) {
+     dev = ldev->pf[i].dev;
      if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
              return false;
}