[PATCH net 0/2] Two fixes for SMC-R

[PATCH net 0/2] Two fixes for SMC-R

[Guangguan Wang]

Two fixes for SMC-R.

Guangguan Wang (2):
  net/smc: bugfix for smcr v2 server connect success statistic
  net/smc: use smc_lgr_list.lock to protect smc_lgr_list.list iterate in
    smcr_port_add

 net/smc/smc_core.c  | 2 ++
 net/smc/smc_stats.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.24.3 (Apple Git-128)

[PATCH net 1/2] net/smc: bugfix for smcr v2 server connect success statistic

[Guangguan Wang]

In the macro SMC_STAT_SERV_SUCC_INC, the smcd_version is used
to determin whether to increase the v1 statistic or the v2
statistic. It is correct for SMCD. But for SMCR, smcr_version
should be used.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/smc_stats.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
index b60fe1eb37ab..aa8928975cc6 100644
--- a/net/smc/smc_stats.h
+++ b/net/smc/smc_stats.h
@@ -243,8 +243,9 @@ while (0)
 #define SMC_STAT_SERV_SUCC_INC(net, _ini) \
 do { \
 	typeof(_ini) i = (_ini); \
-	bool is_v2 = (i->smcd_version & SMC_V2); \
 	bool is_smcd = (i->is_smcd); \
+	u8 version = is_smcd ? i->smcd_version : i->smcr_version; \
+	bool is_v2 = (version & SMC_V2); \
 	typeof(net->smc.smc_stats) smc_stats = (net)->smc.smc_stats; \
 	if (is_v2 && is_smcd) \
 		this_cpu_inc(smc_stats->smc[SMC_TYPE_D].srv_v2_succ_cnt); \
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net 1/2] net/smc: bugfix for smcr v2 server connect success statistic

[Wenjia Zhang]

On 08.09.23 05:31, Guangguan Wang wrote:
> In the macro SMC_STAT_SERV_SUCC_INC, the smcd_version is used
> to determin whether to increase the v1 statistic or the v2
> statistic. It is correct for SMCD. But for SMCR, smcr_version
> should be used.
> 
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> ---
>   net/smc/smc_stats.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
> index b60fe1eb37ab..aa8928975cc6 100644
> --- a/net/smc/smc_stats.h
> +++ b/net/smc/smc_stats.h
> @@ -243,8 +243,9 @@ while (0)
>   #define SMC_STAT_SERV_SUCC_INC(net, _ini) \
>   do { \
>   	typeof(_ini) i = (_ini); \
> -	bool is_v2 = (i->smcd_version & SMC_V2); \
>   	bool is_smcd = (i->is_smcd); \
> +	u8 version = is_smcd ? i->smcd_version : i->smcr_version; \
> +	bool is_v2 = (version & SMC_V2); \
>   	typeof(net->smc.smc_stats) smc_stats = (net)->smc.smc_stats; \
>   	if (is_v2 && is_smcd) \
>   		this_cpu_inc(smc_stats->smc[SMC_TYPE_D].srv_v2_succ_cnt); \

ohje, that is because the statistic was implemented first, then SMCR_v2. 
Good catch! Thank you!

[PATCH net 2/2] net/smc: use smc_lgr_list.lock to protect smc_lgr_list.list iterate in smcr_port_add

[Guangguan Wang]

While doing smcr_port_add, there maybe linkgroup add into or delete
from smc_lgr_list.list at the same time, which may result kernel crash.
So, use smc_lgr_list.lock to protect smc_lgr_list.list iterate in
smcr_port_add.

The crash calltrace show below:
BUG: kernel NULL pointer dereference, address: 0000000000000000
PGD 0 P4D 0
Oops: 0000 [#1] SMP NOPTI
CPU: 0 PID: 559726 Comm: kworker/0:92 Kdump: loaded Tainted: G
Hardware name: Alibaba Cloud Alibaba Cloud ECS, BIOS 449e491 04/01/2014
Workqueue: events smc_ib_port_event_work [smc]
RIP: 0010:smcr_port_add+0xa6/0xf0 [smc]
RSP: 0000:ffffa5a2c8f67de0 EFLAGS: 00010297
RAX: 0000000000000001 RBX: ffff9935e0650000 RCX: 0000000000000000
RDX: 0000000000000010 RSI: ffff9935e0654290 RDI: ffff9935c8560000
RBP: 0000000000000000 R08: 0000000000000000 R09: ffff9934c0401918
R10: 0000000000000000 R11: ffffffffb4a5c278 R12: ffff99364029aae4
R13: ffff99364029aa00 R14: 00000000ffffffed R15: ffff99364029ab08
FS:  0000000000000000(0000) GS:ffff994380600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000f06a10003 CR4: 0000000002770ef0
PKRU: 55555554
Call Trace:
 smc_ib_port_event_work+0x18f/0x380 [smc]
 process_one_work+0x19b/0x340
 worker_thread+0x30/0x370
 ? process_one_work+0x340/0x340
 kthread+0x114/0x130
 ? __kthread_cancel_work+0x50/0x50
 ret_from_fork+0x1f/0x30

Fixes: 1f90a05d9ff9 ("net/smc: add smcr_port_add() and smcr_link_up() processing")
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/smc_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 3f465faf2b68..6aa3db47a956 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1654,6 +1654,7 @@ void smcr_port_add(struct smc_ib_device *smcibdev, u8 ibport)
 {
 	struct smc_link_group *lgr, *n;
 
+	spin_lock_bh(&smc_lgr_list.lock);
 	list_for_each_entry_safe(lgr, n, &smc_lgr_list.list, list) {
 		struct smc_link *link;
 
@@ -1669,6 +1670,7 @@ void smcr_port_add(struct smc_ib_device *smcibdev, u8 ibport)
 		if (link)
 			smc_llc_add_link_local(link);
 	}
+	spin_unlock_bh(&smc_lgr_list.lock);
 }
 
 /* link is down - switch connections to alternate link,
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net 0/2] Two fixes for SMC-R

[Wenjia Zhang]

On 08.09.23 05:31, Guangguan Wang wrote:
> Two fixes for SMC-R.
> 
> Guangguan Wang (2):
>    net/smc: bugfix for smcr v2 server connect success statistic
>    net/smc: use smc_lgr_list.lock to protect smc_lgr_list.list iterate in
>      smcr_port_add
> 
>   net/smc/smc_core.c  | 2 ++
>   net/smc/smc_stats.h | 3 ++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
Thank you for fixing them!

The sign is for the two patches:
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>