[PATCH net] net/smc: fix incorrect SMC-D link group matching logic

[PATCH net] net/smc: fix incorrect SMC-D link group matching logic

[Wen Gu]

The logic to determine if SMC-D link group matches is incorrect. The
correct logic should be that it only returns true when the GID is the
same, and the SMC-D device is the same and the extended GID is the same
(in the case of virtual ISM).

It can be fixed by adding brackets around the conditional (or ternary)
operator expression. But for better readability and maintainability, it
has been changed to an if-else statement.

Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
Closes: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
Fixes: b40584d14570 ("net/smc: compatible with 128-bits extended GID of virtual ISM device")
Link: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 95cc95458e2d..e4c858411207 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1877,9 +1877,15 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
 			   struct smcd_dev *smcismdev,
 			   struct smcd_gid *peer_gid)
 {
-	return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
-		smc_ism_is_virtual(smcismdev) ?
-		(lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
+	if (lgr->peer_gid.gid != peer_gid->gid ||
+	    lgr->smcd != smcismdev)
+		return false;
+
+	if (smc_ism_is_virtual(smcismdev) &&
+	    lgr->peer_gid.gid_ext != peer_gid->gid_ext)
+		return false;
+
+	return true;
 }
 
 /* create a new SMC connection (and a new link group if necessary) */
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net] net/smc: fix incorrect SMC-D link group matching logic

[Alexandra Winter]

On 25.01.24 13:39, Wen Gu wrote:
> The logic to determine if SMC-D link group matches is incorrect. The
> correct logic should be that it only returns true when the GID is the
> same, and the SMC-D device is the same and the extended GID is the same
> (in the case of virtual ISM).
> 
> It can be fixed by adding brackets around the conditional (or ternary)
> operator expression. But for better readability and maintainability, it
> has been changed to an if-else statement.
> 
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Closes: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
> Fixes: b40584d14570 ("net/smc: compatible with 128-bits extended GID of virtual ISM device")
> Link: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
>  net/smc/smc_core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 95cc95458e2d..e4c858411207 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1877,9 +1877,15 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
>  			   struct smcd_dev *smcismdev,
>  			   struct smcd_gid *peer_gid)
>  {
> -	return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
> -		smc_ism_is_virtual(smcismdev) ?
> -		(lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
> +	if (lgr->peer_gid.gid != peer_gid->gid ||
> +	    lgr->smcd != smcismdev)
> +		return false;
> +
> +	if (smc_ism_is_virtual(smcismdev) &&
> +	    lgr->peer_gid.gid_ext != peer_gid->gid_ext)
> +		return false;
> +
> +	return true;
>  }
>  
>  /* create a new SMC connection (and a new link group if necessary) */

Thank you Wen Gu

Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>

Re: [PATCH net] net/smc: fix incorrect SMC-D link group matching logic

[Matthew Rosato]

On 1/25/24 7:39 AM, Wen Gu wrote:
> The logic to determine if SMC-D link group matches is incorrect. The
> correct logic should be that it only returns true when the GID is the
> same, and the SMC-D device is the same and the extended GID is the same
> (in the case of virtual ISM).
> 
> It can be fixed by adding brackets around the conditional (or ternary)
> operator expression. But for better readability and maintainability, it
> has been changed to an if-else statement.
> 
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Closes: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
> Fixes: b40584d14570 ("net/smc: compatible with 128-bits extended GID of virtual ISM device")
> Link: https://lore.kernel.org/r/13579588-eb9d-4626-a063-c0b77ed80f11@linux.ibm.com
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>

Hi Wen Gu,

I just ran the same series of tests with this patch applied and it resolves the issue for me.  Thanks for the quick fix!

Thanks,
Matt


> ---
>  net/smc/smc_core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 95cc95458e2d..e4c858411207 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1877,9 +1877,15 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
>  			   struct smcd_dev *smcismdev,
>  			   struct smcd_gid *peer_gid)
>  {
> -	return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
> -		smc_ism_is_virtual(smcismdev) ?
> -		(lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
> +	if (lgr->peer_gid.gid != peer_gid->gid ||
> +	    lgr->smcd != smcismdev)
> +		return false;
> +
> +	if (smc_ism_is_virtual(smcismdev) &&
> +	    lgr->peer_gid.gid_ext != peer_gid->gid_ext)
> +		return false;
> +
> +	return true;
>  }
>  
>  /* create a new SMC connection (and a new link group if necessary) */

Re: [PATCH net] net/smc: fix incorrect SMC-D link group matching logic

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 25 Jan 2024 20:39:16 +0800 you wrote:
> The logic to determine if SMC-D link group matches is incorrect. The
> correct logic should be that it only returns true when the GID is the
> same, and the SMC-D device is the same and the extended GID is the same
> (in the case of virtual ISM).
> 
> It can be fixed by adding brackets around the conditional (or ternary)
> operator expression. But for better readability and maintainability, it
> has been changed to an if-else statement.
> 
> [...]

Here is the summary with links:
  - [net] net/smc: fix incorrect SMC-D link group matching logic
    https://git.kernel.org/netdev/net/c/c3dfcdb65ec1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html