* [PATCH for-next 0/3] rtrs bugfix and cleanups
@ 2023-04-10 6:43 Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
` (2 more replies)
0 siblings, 3 replies; 38+ messages in thread
From: Li Zhijian @ 2023-04-10 6:43 UTC (permalink / raw)
To: haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: guoqing.jiang, linux-kernel, Li Zhijian
It's trying to fix 2 issues caused by the following script which
connect/disconnect rnbd frequently.
# cat rnbd-self.sh
#!/bin/bash
/root/rpma/tools/config_softroce.sh eth0
modprobe rnbd_server
modprobe rnbd_client
while true;
do
echo "sessname=xyz path=ip:<server-ip> device_path=/dev/nvme0n1" > /sys/devices/virtual/rnbd-client/ctl/map_device
for i in /sys/block/rnbd*/rnbd/unmap_device
do
echo "normal" > $i
done
done
Li Zhijian (3):
RDMA/rtrs: remove duplicate cq_num assignment
RDMA/rtrs: Fix rxe_dealloc_pd warning
RDMA/rtrs: Fix use-after-free in rtrs_clt_rdma_cm_handler
drivers/infiniband/ulp/rtrs/rtrs-clt.c | 19 ++++++++++++++-----
drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
2 files changed, 15 insertions(+), 5 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment
2023-04-10 6:43 [PATCH for-next 0/3] rtrs bugfix and cleanups Li Zhijian
@ 2023-04-10 6:43 ` Li Zhijian
2023-04-10 13:09 ` Guoqing Jiang
2023-04-19 10:37 ` Jinpu Wang
2023-04-10 6:43 ` [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler Li Zhijian
2 siblings, 2 replies; 38+ messages in thread
From: Li Zhijian @ 2023-04-10 6:43 UTC (permalink / raw)
To: haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: guoqing.jiang, linux-kernel, Li Zhijian
line 1701 and 1713 are duplicate:
> 1701 cq_num = max_send_wr + max_recv_wr;
1702 /* alloc iu to recv new rkey reply when server reports flags set */
1703 if (clt_path->flags & RTRS_MSG_NEW_RKEY_F || con->c.cid == 0) {
1704 con->rsp_ius = rtrs_iu_alloc(cq_num, sizeof(*rsp),
1705 GFP_KERNEL,
1706 clt_path->s.dev->ib_dev,
1707 DMA_FROM_DEVICE,
1708 rtrs_clt_rdma_done);
1709 if (!con->rsp_ius)
1710 return -ENOMEM;
1711 con->queue_num = cq_num;
1712 }
> 1713 cq_num = max_send_wr + max_recv_wr;
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
drivers/infiniband/ulp/rtrs/rtrs-clt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 80abf45a197a..c2065fc33a56 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1710,7 +1710,6 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
return -ENOMEM;
con->queue_num = cq_num;
}
- cq_num = max_send_wr + max_recv_wr;
cq_vector = con->cpu % clt_path->s.dev->ib_dev->num_comp_vectors;
if (con->c.cid >= clt_path->s.irq_con_num)
err = rtrs_cq_qp_create(&clt_path->s, &con->c, max_send_sge,
--
2.29.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-10 6:43 [PATCH for-next 0/3] rtrs bugfix and cleanups Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
@ 2023-04-10 6:43 ` Li Zhijian
2023-04-10 12:08 ` Leon Romanovsky
2023-04-10 6:43 ` [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler Li Zhijian
2 siblings, 1 reply; 38+ messages in thread
From: Li Zhijian @ 2023-04-10 6:43 UTC (permalink / raw)
To: haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: guoqing.jiang, linux-kernel, Li Zhijian
The warning occurs when destroying PD whose reference count is not zero.
Precodition: clt_path->s.con_num is 2.
So 2 cm connection will be created as below:
CPU0 CPU1
init_conns { |
create_cm() // a. con[0] created |
| a'. rtrs_clt_rdma_cm_handler() {
| rtrs_rdma_addr_resolved()
| create_con_cq_qp(con); << con[0]
| }
| in this moment, refcnt of PD was increased to 2+
|
create_cm() // b. cid = 1, failed |
destroy_con_cq_qp() |
rtrs_ib_dev_put() |
dev_free() |
ib_dealloc_pd(dev->ib_pd) << PD |
is destroyed, but refcnt is |
still greater than 0 |
}
Simply, Here we can avoid this warning by introducing conn own flag to
track if its cleanup should drop the PD.
-----------------------------------------------
rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
ib_dealloc_pd_user+0x4b/0x80 [ib_core]
rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
init_path+0x1e7/0x9a0 [rtrs_client]
? __pfx_autoremove_wake_function+0x10/0x10
? lock_is_held_type+0xd7/0x130
? rcu_read_lock_sched_held+0x43/0x80
? pcpu_alloc+0x3dd/0x7d0
? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index c2065fc33a56..4c8f42e46e2f 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
return -ENOMEM;
}
clt_path->s.dev_ref = 1;
+ con->has_dev = true;
query_fast_reg_mode(clt_path);
wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
/*
@@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
/* Shared between connections */
clt_path->s.dev_ref++;
+ con->has_dev = true;
max_send_wr = min_t(int, wr_limit,
/* QD * (REQ + RSP + FR REGS or INVS) + drain */
clt_path->queue_depth * 3 + 1);
@@ -1742,6 +1744,8 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
con->rsp_ius = NULL;
con->queue_num = 0;
}
+ if (!con->has_dev)
+ return;
if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
rtrs_ib_dev_put(clt_path->s.dev);
clt_path->s.dev = NULL;
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
index f848c0392d98..970b75633594 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
@@ -75,6 +75,7 @@ struct rtrs_clt_con {
unsigned int cpu;
struct mutex con_mutex;
int cm_err;
+ bool has_dev;
};
/**
--
2.29.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler
2023-04-10 6:43 [PATCH for-next 0/3] rtrs bugfix and cleanups Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning Li Zhijian
@ 2023-04-10 6:43 ` Li Zhijian
2023-04-10 12:10 ` Leon Romanovsky
2023-04-10 13:13 ` Guoqing Jiang
2 siblings, 2 replies; 38+ messages in thread
From: Li Zhijian @ 2023-04-10 6:43 UTC (permalink / raw)
To: haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: guoqing.jiang, linux-kernel, Li Zhijian
Currently, con will be destroyed when wait_event_interruptible_timeout()
returns ERESTARTSYS. But the in-flight event handler
rtrs_clt_rdma_cm_handler() could be rescheduled/wakeup which
may cause a use-after-free.
WARNING: CPU: 0 PID: 14766 at drivers/infiniband/ulp/rtrs/rtrs-clt.c:1687 rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
Modules linked in: rnbd_client rtrs_client rtrs_core rdma_cm iw_cm ib_cm rdma_rxe ib_uverbs ib_core libiscsi scsi_transport_iscsi crc32_generic udp_tunnel dax_pmem nd_pmem nd_btt virtiofs crc32c_intel nvme fuse nvme_core nfit
libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_core]
CPU: 0 PID: 14766 Comm: kworker/u2:3 Kdump: loaded Tainted: G W 6.2.0-rc6-roce-flush+ #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
Workqueue: ib_addr process_one_req [ib_core]
RIP: 0010:rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
Code: 00 0f 85 5f fd ff ff 4c 8b 23 41 bd f4 ff ff ff e9 95 fb ff ff 0f 0b 4c 89 f7 41 bd ea ff ff ff e8 75 c8 92 ec e9 4b ff ff ff <0f> 0b 4c 89 f7 41 bd ea ff ff ff e8 60 c8 92 ec e9 36 ff ff ff e8
RSP: 0018:ffffa4ef41cdbc60 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff9372c394e600 RCX: 0000000000000001
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffffad634277
RBP: ffffa4ef41cdbd00 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000003ff3 R11: 0000000000000000 R12: ffff9372c3164800
R13: ffff9372c3164800 R14: ffff9372c394e640 R15: ffff9372c5219020
FS: 0000000000000000(0000) GS:ffff9372fbc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f35bb7d5de0 CR3: 0000000020c2a006 CR4: 00000000001706f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
? mark_held_locks+0x49/0x80
? lock_is_held_type+0xd7/0x130
? cma_cm_event_handler+0x49/0x200 [rdma_cm]
cma_cm_event_handler+0x49/0x200 [rdma_cm]
addr_handler+0xf1/0x1e0 [rdma_cm]
? lock_acquire+0xca/0x2f0
? lock_acquire+0xda/0x2f0
process_one_req+0x43/0x170 [ib_core]
process_one_work+0x274/0x590
worker_thread+0x4f/0x3d0
? __pfx_worker_thread+0x10/0x10
kthread+0xe7/0x110
? __pfx_kthread+0x10/0x10
ret_from_fork+0x2c/0x50
</TASK>
irq event stamp: 1432669
hardirqs last enabled at (1432683): [<ffffffffac508eb2>] __up_console_sem+0x52/0x60
hardirqs last disabled at (1432698): [<ffffffffac508e97>] __up_console_sem+0x37/0x60
softirqs last enabled at (1432518): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
softirqs last disabled at (1432509): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
---[ end trace 0000000000000000 ]---
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
drivers/infiniband/ulp/rtrs/rtrs-clt.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 4c8f42e46e2f..760a7eb51297 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2074,6 +2074,7 @@ static int create_cm(struct rtrs_clt_con *con)
rtrs_err(s, "Failed to resolve address, err: %d\n", err);
goto destroy_cm;
}
+again:
/*
* Combine connection status and session events. This is needed
* for waiting two possible cases: cm_err has something meaningful
@@ -2083,10 +2084,15 @@ static int create_cm(struct rtrs_clt_con *con)
clt_path->state_wq,
con->cm_err || clt_path->state != RTRS_CLT_CONNECTING,
msecs_to_jiffies(RTRS_CONNECT_TIMEOUT_MS));
- if (err == 0 || err == -ERESTARTSYS) {
- if (err == 0)
- err = -ETIMEDOUT;
- /* Timedout or interrupted */
+ if (err == -ERESTARTSYS) {
+ /* interrupted,
+ * try again to avoid the in-flight rtrs_clt_rdma_cm_handler()
+ * getting a use-after-free
+ */
+ goto again;
+ } else if (err == 0) {
+ err = -ETIMEDOUT;
+ /* Timedout */
goto errr;
}
if (con->cm_err < 0) {
--
2.29.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-10 6:43 ` [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning Li Zhijian
@ 2023-04-10 12:08 ` Leon Romanovsky
2023-04-10 13:10 ` Guoqing Jiang
0 siblings, 1 reply; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-10 12:08 UTC (permalink / raw)
To: Li Zhijian
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, guoqing.jiang, linux-kernel
On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> The warning occurs when destroying PD whose reference count is not zero.
>
> Precodition: clt_path->s.con_num is 2.
> So 2 cm connection will be created as below:
> CPU0 CPU1
> init_conns { |
> create_cm() // a. con[0] created |
> | a'. rtrs_clt_rdma_cm_handler() {
> | rtrs_rdma_addr_resolved()
> | create_con_cq_qp(con); << con[0]
> | }
> | in this moment, refcnt of PD was increased to 2+
> |
> create_cm() // b. cid = 1, failed |
> destroy_con_cq_qp() |
> rtrs_ib_dev_put() |
> dev_free() |
> ib_dealloc_pd(dev->ib_pd) << PD |
> is destroyed, but refcnt is |
> still greater than 0 |
> }
>
> Simply, Here we can avoid this warning by introducing conn own flag to
> track if its cleanup should drop the PD.
>
> -----------------------------------------------
> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
> init_path+0x1e7/0x9a0 [rtrs_client]
> ? __pfx_autoremove_wake_function+0x10/0x10
> ? lock_is_held_type+0xd7/0x130
> ? rcu_read_lock_sched_held+0x43/0x80
> ? pcpu_alloc+0x3dd/0x7d0
> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index c2065fc33a56..4c8f42e46e2f 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> return -ENOMEM;
> }
> clt_path->s.dev_ref = 1;
> + con->has_dev = true;
> query_fast_reg_mode(clt_path);
> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
> /*
> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
> /* Shared between connections */
> clt_path->s.dev_ref++;
Without looking in the code, I would expect dev_ref from the line above
to perform PD protection.
> + con->has_dev = true;
> max_send_wr = min_t(int, wr_limit,
> /* QD * (REQ + RSP + FR REGS or INVS) + drain */
> clt_path->queue_depth * 3 + 1);
> @@ -1742,6 +1744,8 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
> con->rsp_ius = NULL;
> con->queue_num = 0;
> }
> + if (!con->has_dev)
> + return;
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> rtrs_ib_dev_put(clt_path->s.dev);
> clt_path->s.dev = NULL;
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
> index f848c0392d98..970b75633594 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
> @@ -75,6 +75,7 @@ struct rtrs_clt_con {
> unsigned int cpu;
> struct mutex con_mutex;
> int cm_err;
> + bool has_dev;
> };
>
> /**
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler
2023-04-10 6:43 ` [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler Li Zhijian
@ 2023-04-10 12:10 ` Leon Romanovsky
2023-04-10 13:13 ` Guoqing Jiang
1 sibling, 0 replies; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-10 12:10 UTC (permalink / raw)
To: Li Zhijian
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, guoqing.jiang, linux-kernel
On Mon, Apr 10, 2023 at 06:43:04AM +0000, Li Zhijian wrote:
> Currently, con will be destroyed when wait_event_interruptible_timeout()
> returns ERESTARTSYS. But the in-flight event handler
> rtrs_clt_rdma_cm_handler() could be rescheduled/wakeup which
> may cause a use-after-free.
>
> WARNING: CPU: 0 PID: 14766 at drivers/infiniband/ulp/rtrs/rtrs-clt.c:1687 rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
> Modules linked in: rnbd_client rtrs_client rtrs_core rdma_cm iw_cm ib_cm rdma_rxe ib_uverbs ib_core libiscsi scsi_transport_iscsi crc32_generic udp_tunnel dax_pmem nd_pmem nd_btt virtiofs crc32c_intel nvme fuse nvme_core nfit
> libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_core]
> CPU: 0 PID: 14766 Comm: kworker/u2:3 Kdump: loaded Tainted: G W 6.2.0-rc6-roce-flush+ #56
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> Workqueue: ib_addr process_one_req [ib_core]
> RIP: 0010:rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
> Code: 00 0f 85 5f fd ff ff 4c 8b 23 41 bd f4 ff ff ff e9 95 fb ff ff 0f 0b 4c 89 f7 41 bd ea ff ff ff e8 75 c8 92 ec e9 4b ff ff ff <0f> 0b 4c 89 f7 41 bd ea ff ff ff e8 60 c8 92 ec e9 36 ff ff ff e8
> RSP: 0018:ffffa4ef41cdbc60 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff9372c394e600 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffffad634277
> RBP: ffffa4ef41cdbd00 R08: 0000000000000000 R09: 0000000000000001
> R10: 0000000000003ff3 R11: 0000000000000000 R12: ffff9372c3164800
> R13: ffff9372c3164800 R14: ffff9372c394e640 R15: ffff9372c5219020
> FS: 0000000000000000(0000) GS:ffff9372fbc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f35bb7d5de0 CR3: 0000000020c2a006 CR4: 00000000001706f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ? mark_held_locks+0x49/0x80
> ? lock_is_held_type+0xd7/0x130
> ? cma_cm_event_handler+0x49/0x200 [rdma_cm]
> cma_cm_event_handler+0x49/0x200 [rdma_cm]
> addr_handler+0xf1/0x1e0 [rdma_cm]
> ? lock_acquire+0xca/0x2f0
> ? lock_acquire+0xda/0x2f0
> process_one_req+0x43/0x170 [ib_core]
> process_one_work+0x274/0x590
> worker_thread+0x4f/0x3d0
> ? __pfx_worker_thread+0x10/0x10
> kthread+0xe7/0x110
> ? __pfx_kthread+0x10/0x10
> ret_from_fork+0x2c/0x50
> </TASK>
> irq event stamp: 1432669
> hardirqs last enabled at (1432683): [<ffffffffac508eb2>] __up_console_sem+0x52/0x60
> hardirqs last disabled at (1432698): [<ffffffffac508e97>] __up_console_sem+0x37/0x60
> softirqs last enabled at (1432518): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
> softirqs last disabled at (1432509): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
> ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 4c8f42e46e2f..760a7eb51297 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -2074,6 +2074,7 @@ static int create_cm(struct rtrs_clt_con *con)
> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
> goto destroy_cm;
> }
> +again:
> /*
> * Combine connection status and session events. This is needed
> * for waiting two possible cases: cm_err has something meaningful
> @@ -2083,10 +2084,15 @@ static int create_cm(struct rtrs_clt_con *con)
> clt_path->state_wq,
> con->cm_err || clt_path->state != RTRS_CLT_CONNECTING,
> msecs_to_jiffies(RTRS_CONNECT_TIMEOUT_MS));
> - if (err == 0 || err == -ERESTARTSYS) {
> - if (err == 0)
> - err = -ETIMEDOUT;
> - /* Timedout or interrupted */
> + if (err == -ERESTARTSYS) {
> + /* interrupted,
> + * try again to avoid the in-flight rtrs_clt_rdma_cm_handler()
> + * getting a use-after-free
> + */
> + goto again;
> + } else if (err == 0) {
> + err = -ETIMEDOUT;
> + /* Timedout */
Timedout -> Timeout
> goto errr;
errrr, sigh.
> }
> if (con->cm_err < 0) {
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
@ 2023-04-10 13:09 ` Guoqing Jiang
2023-04-19 10:37 ` Jinpu Wang
1 sibling, 0 replies; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-10 13:09 UTC (permalink / raw)
To: Li Zhijian, haris.iqbal, jinpu.wang, jgg, leon, linux-rdma; +Cc: linux-kernel
On 4/10/23 14:43, Li Zhijian wrote:
> line 1701 and 1713 are duplicate:
>> 1701 cq_num = max_send_wr + max_recv_wr;
> 1702 /* alloc iu to recv new rkey reply when server reports flags set */
> 1703 if (clt_path->flags & RTRS_MSG_NEW_RKEY_F || con->c.cid == 0) {
> 1704 con->rsp_ius = rtrs_iu_alloc(cq_num, sizeof(*rsp),
> 1705 GFP_KERNEL,
> 1706 clt_path->s.dev->ib_dev,
> 1707 DMA_FROM_DEVICE,
> 1708 rtrs_clt_rdma_done);
> 1709 if (!con->rsp_ius)
> 1710 return -ENOMEM;
> 1711 con->queue_num = cq_num;
> 1712 }
>> 1713 cq_num = max_send_wr + max_recv_wr;
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 80abf45a197a..c2065fc33a56 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1710,7 +1710,6 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> return -ENOMEM;
> con->queue_num = cq_num;
> }
> - cq_num = max_send_wr + max_recv_wr;
> cq_vector = con->cpu % clt_path->s.dev->ib_dev->num_comp_vectors;
> if (con->c.cid >= clt_path->s.irq_con_num)
> err = rtrs_cq_qp_create(&clt_path->s, &con->c, max_send_sge,
Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-10 12:08 ` Leon Romanovsky
@ 2023-04-10 13:10 ` Guoqing Jiang
2023-04-11 2:43 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-10 13:10 UTC (permalink / raw)
To: Leon Romanovsky, Li Zhijian
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 4/10/23 20:08, Leon Romanovsky wrote:
> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>> The warning occurs when destroying PD whose reference count is not zero.
>>
>> Precodition: clt_path->s.con_num is 2.
>> So 2 cm connection will be created as below:
>> CPU0 CPU1
>> init_conns { |
>> create_cm() // a. con[0] created |
>> | a'. rtrs_clt_rdma_cm_handler() {
>> | rtrs_rdma_addr_resolved()
>> | create_con_cq_qp(con); << con[0]
>> | }
>> | in this moment, refcnt of PD was increased to 2+
>> |
>> create_cm() // b. cid = 1, failed |
>> destroy_con_cq_qp() |
>> rtrs_ib_dev_put() |
>> dev_free() |
>> ib_dealloc_pd(dev->ib_pd) << PD |
>> is destroyed, but refcnt is |
>> still greater than 0 |
>> }
>>
>> Simply, Here we can avoid this warning by introducing conn own flag to
>> track if its cleanup should drop the PD.
>>
>> -----------------------------------------------
>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <TASK>
>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
>> init_path+0x1e7/0x9a0 [rtrs_client]
>> ? __pfx_autoremove_wake_function+0x10/0x10
>> ? lock_is_held_type+0xd7/0x130
>> ? rcu_read_lock_sched_held+0x43/0x80
>> ? pcpu_alloc+0x3dd/0x7d0
>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> index c2065fc33a56..4c8f42e46e2f 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>> return -ENOMEM;
>> }
>> clt_path->s.dev_ref = 1;
>> + con->has_dev = true;
>> query_fast_reg_mode(clt_path);
>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>> /*
>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>> /* Shared between connections */
>> clt_path->s.dev_ref++;
> Without looking in the code, I would expect dev_ref from the line above
> to perform PD protection.
Agreed.
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler
2023-04-10 6:43 ` [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler Li Zhijian
2023-04-10 12:10 ` Leon Romanovsky
@ 2023-04-10 13:13 ` Guoqing Jiang
2023-04-11 1:33 ` Zhijian Li (Fujitsu)
1 sibling, 1 reply; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-10 13:13 UTC (permalink / raw)
To: Li Zhijian, haris.iqbal, jinpu.wang, jgg, leon, linux-rdma; +Cc: linux-kernel
On 4/10/23 14:43, Li Zhijian wrote:
> Currently, con will be destroyed when wait_event_interruptible_timeout()
> returns ERESTARTSYS. But the in-flight event handler
> rtrs_clt_rdma_cm_handler() could be rescheduled/wakeup which
> may cause a use-after-free.
>
> WARNING: CPU: 0 PID: 14766 at drivers/infiniband/ulp/rtrs/rtrs-clt.c:1687 rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
> Modules linked in: rnbd_client rtrs_client rtrs_core rdma_cm iw_cm ib_cm rdma_rxe ib_uverbs ib_core libiscsi scsi_transport_iscsi crc32_generic udp_tunnel dax_pmem nd_pmem nd_btt virtiofs crc32c_intel nvme fuse nvme_core nfit
> libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_core]
> CPU: 0 PID: 14766 Comm: kworker/u2:3 Kdump: loaded Tainted: G W 6.2.0-rc6-roce-flush+ #56
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> Workqueue: ib_addr process_one_req [ib_core]
> RIP: 0010:rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
> Code: 00 0f 85 5f fd ff ff 4c 8b 23 41 bd f4 ff ff ff e9 95 fb ff ff 0f 0b 4c 89 f7 41 bd ea ff ff ff e8 75 c8 92 ec e9 4b ff ff ff <0f> 0b 4c 89 f7 41 bd ea ff ff ff e8 60 c8 92 ec e9 36 ff ff ff e8
> RSP: 0018:ffffa4ef41cdbc60 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff9372c394e600 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffffad634277
> RBP: ffffa4ef41cdbd00 R08: 0000000000000000 R09: 0000000000000001
> R10: 0000000000003ff3 R11: 0000000000000000 R12: ffff9372c3164800
> R13: ffff9372c3164800 R14: ffff9372c394e640 R15: ffff9372c5219020
> FS: 0000000000000000(0000) GS:ffff9372fbc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f35bb7d5de0 CR3: 0000000020c2a006 CR4: 00000000001706f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ? mark_held_locks+0x49/0x80
> ? lock_is_held_type+0xd7/0x130
> ? cma_cm_event_handler+0x49/0x200 [rdma_cm]
> cma_cm_event_handler+0x49/0x200 [rdma_cm]
> addr_handler+0xf1/0x1e0 [rdma_cm]
> ? lock_acquire+0xca/0x2f0
> ? lock_acquire+0xda/0x2f0
> process_one_req+0x43/0x170 [ib_core]
> process_one_work+0x274/0x590
> worker_thread+0x4f/0x3d0
> ? __pfx_worker_thread+0x10/0x10
> kthread+0xe7/0x110
> ? __pfx_kthread+0x10/0x10
> ret_from_fork+0x2c/0x50
> </TASK>
> irq event stamp: 1432669
> hardirqs last enabled at (1432683): [<ffffffffac508eb2>] __up_console_sem+0x52/0x60
> hardirqs last disabled at (1432698): [<ffffffffac508e97>] __up_console_sem+0x37/0x60
> softirqs last enabled at (1432518): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
> softirqs last disabled at (1432509): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
> ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 4c8f42e46e2f..760a7eb51297 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -2074,6 +2074,7 @@ static int create_cm(struct rtrs_clt_con *con)
> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
> goto destroy_cm;
> }
> +again:
> /*
> * Combine connection status and session events. This is needed
> * for waiting two possible cases: cm_err has something meaningful
> @@ -2083,10 +2084,15 @@ static int create_cm(struct rtrs_clt_con *con)
> clt_path->state_wq,
> con->cm_err || clt_path->state != RTRS_CLT_CONNECTING,
> msecs_to_jiffies(RTRS_CONNECT_TIMEOUT_MS));
> - if (err == 0 || err == -ERESTARTSYS) {
> - if (err == 0)
> - err = -ETIMEDOUT;
> - /* Timedout or interrupted */
> + if (err == -ERESTARTSYS) {
> + /* interrupted,
> + * try again to avoid the in-flight rtrs_clt_rdma_cm_handler()
> + * getting a use-after-free
> + */
> + goto again;
> + } else if (err == 0) {
> + err = -ETIMEDOUT;
> + /* Timedout */
> goto errr;
> }
Can event handler still be triggered in case of timeout?
And I guess either stop_cm -> rdma_disconnect or destroy_cm ->
rdma_destroy_id
should prevent this kind of racy issue.
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler
2023-04-10 13:13 ` Guoqing Jiang
@ 2023-04-11 1:33 ` Zhijian Li (Fujitsu)
2023-04-12 1:15 ` Guoqing Jiang
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-11 1:33 UTC (permalink / raw)
To: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: linux-kernel
On 10/04/2023 21:13, Guoqing Jiang wrote:
>
>
> On 4/10/23 14:43, Li Zhijian wrote:
>> Currently, con will be destroyed when wait_event_interruptible_timeout()
>> returns ERESTARTSYS. But the in-flight event handler
>> rtrs_clt_rdma_cm_handler() could be rescheduled/wakeup which
>> may cause a use-after-free.
>>
>> WARNING: CPU: 0 PID: 14766 at drivers/infiniband/ulp/rtrs/rtrs-clt.c:1687 rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
>> Modules linked in: rnbd_client rtrs_client rtrs_core rdma_cm iw_cm ib_cm rdma_rxe ib_uverbs ib_core libiscsi scsi_transport_iscsi crc32_generic udp_tunnel dax_pmem nd_pmem nd_btt virtiofs crc32c_intel nvme fuse nvme_core nfit
>> libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_core]
>> CPU: 0 PID: 14766 Comm: kworker/u2:3 Kdump: loaded Tainted: G W 6.2.0-rc6-roce-flush+ #56
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>> Workqueue: ib_addr process_one_req [ib_core]
>> RIP: 0010:rtrs_clt_rdma_cm_handler+0x620/0x640 [rtrs_client]
>> Code: 00 0f 85 5f fd ff ff 4c 8b 23 41 bd f4 ff ff ff e9 95 fb ff ff 0f 0b 4c 89 f7 41 bd ea ff ff ff e8 75 c8 92 ec e9 4b ff ff ff <0f> 0b 4c 89 f7 41 bd ea ff ff ff e8 60 c8 92 ec e9 36 ff ff ff e8
>> RSP: 0018:ffffa4ef41cdbc60 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: ffff9372c394e600 RCX: 0000000000000001
>> RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffffad634277
>> RBP: ffffa4ef41cdbd00 R08: 0000000000000000 R09: 0000000000000001
>> R10: 0000000000003ff3 R11: 0000000000000000 R12: ffff9372c3164800
>> R13: ffff9372c3164800 R14: ffff9372c394e640 R15: ffff9372c5219020
>> FS: 0000000000000000(0000) GS:ffff9372fbc00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007f35bb7d5de0 CR3: 0000000020c2a006 CR4: 00000000001706f0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <TASK>
>> ? mark_held_locks+0x49/0x80
>> ? lock_is_held_type+0xd7/0x130
>> ? cma_cm_event_handler+0x49/0x200 [rdma_cm]
>> cma_cm_event_handler+0x49/0x200 [rdma_cm]
>> addr_handler+0xf1/0x1e0 [rdma_cm]
>> ? lock_acquire+0xca/0x2f0
>> ? lock_acquire+0xda/0x2f0
>> process_one_req+0x43/0x170 [ib_core]
>> process_one_work+0x274/0x590
>> worker_thread+0x4f/0x3d0
>> ? __pfx_worker_thread+0x10/0x10
>> kthread+0xe7/0x110
>> ? __pfx_kthread+0x10/0x10
>> ret_from_fork+0x2c/0x50
>> </TASK>
>> irq event stamp: 1432669
>> hardirqs last enabled at (1432683): [<ffffffffac508eb2>] __up_console_sem+0x52/0x60
>> hardirqs last disabled at (1432698): [<ffffffffac508e97>] __up_console_sem+0x37/0x60
>> softirqs last enabled at (1432518): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
>> softirqs last disabled at (1432509): [<ffffffffac48c985>] __irq_exit_rcu+0xc5/0x120
>> ---[ end trace 0000000000000000 ]---
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> index 4c8f42e46e2f..760a7eb51297 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> @@ -2074,6 +2074,7 @@ static int create_cm(struct rtrs_clt_con *con)
>> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
>> goto destroy_cm;
>> }
>> +again:
>> /*
>> * Combine connection status and session events. This is needed
>> * for waiting two possible cases: cm_err has something meaningful
>> @@ -2083,10 +2084,15 @@ static int create_cm(struct rtrs_clt_con *con)
>> clt_path->state_wq,
>> con->cm_err || clt_path->state != RTRS_CLT_CONNECTING,
>> msecs_to_jiffies(RTRS_CONNECT_TIMEOUT_MS));
>> - if (err == 0 || err == -ERESTARTSYS) {
>> - if (err == 0)
>> - err = -ETIMEDOUT;
>> - /* Timedout or interrupted */
>> + if (err == -ERESTARTSYS) {
>> + /* interrupted,
>> + * try again to avoid the in-flight rtrs_clt_rdma_cm_handler()
>> + * getting a use-after-free
>> + */
>> + goto again;
>> + } else if (err == 0) {
>> + err = -ETIMEDOUT;
>> + /* Timedout */
>> goto errr;
>> }
>
> Can event handler still be triggered in case of timeout?
I have never hit such race.
>
> And I guess either stop_cm -> rdma_disconnect or destroy_cm -> rdma_destroy_id
> should prevent this kind of racy issue.
In practice, they are possible that rtrs_clt_rdma_cm_handler() is in-flight during
'either stop_cm -> rdma_disconnect or destroy_cm -> rdma_destroy_id'. rtrs_clt_rdma_cm_handler() and
cm's cleanup path need to hold mutex_lock(&con->con_mutex), once cm's cleanup path get this lock first
rtrs_clt_rdma_cm_handler has to sleep, when rtrs_clt_rdma_cm_handler is wakeup again, some resources has been
freed by cm's cleanup path.
Thanks
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-10 13:10 ` Guoqing Jiang
@ 2023-04-11 2:43 ` Zhijian Li (Fujitsu)
2023-04-11 12:26 ` Leon Romanovsky
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-11 2:43 UTC (permalink / raw)
To: Guoqing Jiang, Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 10/04/2023 21:10, Guoqing Jiang wrote:
>
>
> On 4/10/23 20:08, Leon Romanovsky wrote:
>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>> The warning occurs when destroying PD whose reference count is not zero.
>>>
>>> Precodition: clt_path->s.con_num is 2.
>>> So 2 cm connection will be created as below:
>>> CPU0 CPU1
>>> init_conns { |
>>> create_cm() // a. con[0] created |
>>> | a'. rtrs_clt_rdma_cm_handler() {
>>> | rtrs_rdma_addr_resolved()
>>> | create_con_cq_qp(con); << con[0]
>>> | }
>>> | in this moment, refcnt of PD was increased to 2+
>>> |
>>> create_cm() // b. cid = 1, failed |
>>> destroy_con_cq_qp() |
>>> rtrs_ib_dev_put() |
>>> dev_free() |
>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>> is destroyed, but refcnt is |
>>> still greater than 0 |
>>> }
>>>
>>> Simply, Here we can avoid this warning by introducing conn own flag to
>>> track if its cleanup should drop the PD.
>>>
>>> -----------------------------------------------
>>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
>>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
>>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
>>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
>>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
>>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
>>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
>>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
>>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
>>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>> Call Trace:
>>> <TASK>
>>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
>>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
>>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
>>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
>>> init_path+0x1e7/0x9a0 [rtrs_client]
>>> ? __pfx_autoremove_wake_function+0x10/0x10
>>> ? lock_is_held_type+0xd7/0x130
>>> ? rcu_read_lock_sched_held+0x43/0x80
>>> ? pcpu_alloc+0x3dd/0x7d0
>>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
>>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
>>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
>>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>>>
>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>> ---
>>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
>>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> index c2065fc33a56..4c8f42e46e2f 100644
>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>> return -ENOMEM;
>>> }
>>> clt_path->s.dev_ref = 1;
>>> + con->has_dev = true;
>>> query_fast_reg_mode(clt_path);
>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>> /*
>>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>> /* Shared between connections */
>>> clt_path->s.dev_ref++;
>> Without looking in the code, I would expect dev_ref from the line above
>> to perform PD protection.
>
> Agreed.
Sorry, i didn't get your point. Do you mean something like this:
+ con->has_dev = true;
clt_path->s.dev_ref++;
Thanks
Zhijian
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-11 2:43 ` Zhijian Li (Fujitsu)
@ 2023-04-11 12:26 ` Leon Romanovsky
2023-04-12 1:15 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-11 12:26 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 10/04/2023 21:10, Guoqing Jiang wrote:
> >
> >
> > On 4/10/23 20:08, Leon Romanovsky wrote:
> >> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> >>> The warning occurs when destroying PD whose reference count is not zero.
> >>>
> >>> Precodition: clt_path->s.con_num is 2.
> >>> So 2 cm connection will be created as below:
> >>> CPU0 CPU1
> >>> init_conns { |
> >>> create_cm() // a. con[0] created |
> >>> | a'. rtrs_clt_rdma_cm_handler() {
> >>> | rtrs_rdma_addr_resolved()
> >>> | create_con_cq_qp(con); << con[0]
> >>> | }
> >>> | in this moment, refcnt of PD was increased to 2+
> >>> |
> >>> create_cm() // b. cid = 1, failed |
> >>> destroy_con_cq_qp() |
> >>> rtrs_ib_dev_put() |
> >>> dev_free() |
> >>> ib_dealloc_pd(dev->ib_pd) << PD |
> >>> is destroyed, but refcnt is |
> >>> still greater than 0 |
> >>> }
> >>>
> >>> Simply, Here we can avoid this warning by introducing conn own flag to
> >>> track if its cleanup should drop the PD.
> >>>
> >>> -----------------------------------------------
> >>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
> >>> ------------[ cut here ]------------
> >>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
> >>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
> >>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
> >>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> >>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
> >>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
> >>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
> >>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
> >>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
> >>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
> >>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
> >>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
> >>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
> >>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
> >>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >>> Call Trace:
> >>> <TASK>
> >>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
> >>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
> >>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
> >>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
> >>> init_path+0x1e7/0x9a0 [rtrs_client]
> >>> ? __pfx_autoremove_wake_function+0x10/0x10
> >>> ? lock_is_held_type+0xd7/0x130
> >>> ? rcu_read_lock_sched_held+0x43/0x80
> >>> ? pcpu_alloc+0x3dd/0x7d0
> >>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
> >>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
> >>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
> >>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
> >>>
> >>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >>> ---
> >>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
> >>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
> >>> 2 files changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> >>> index c2065fc33a56..4c8f42e46e2f 100644
> >>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> >>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> >>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> >>> return -ENOMEM;
> >>> }
> >>> clt_path->s.dev_ref = 1;
> >>> + con->has_dev = true;
> >>> query_fast_reg_mode(clt_path);
> >>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
> >>> /*
> >>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> >>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
> >>> /* Shared between connections */
> >>> clt_path->s.dev_ref++;
> >> Without looking in the code, I would expect dev_ref from the line above
> >> to perform PD protection.
> >
> > Agreed.
>
> Sorry, i didn't get your point. Do you mean something like this:
>
> + con->has_dev = true;
> clt_path->s.dev_ref++;
No, my point was that clt_path->s.dev_ref > 0 means that has_dev is
equal to true, and dev_ref is supposed to protect from early PD
destruction.
Thanks
>
>
>
> Thanks
> Zhijian
>
> >
> > Thanks,
> > Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler
2023-04-11 1:33 ` Zhijian Li (Fujitsu)
@ 2023-04-12 1:15 ` Guoqing Jiang
0 siblings, 0 replies; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-12 1:15 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), haris.iqbal, jinpu.wang, jgg, leon, linux-rdma
Cc: linux-kernel
On 4/11/23 09:33, Zhijian Li (Fujitsu) wrote:
>
>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> index 4c8f42e46e2f..760a7eb51297 100644
>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>> @@ -2074,6 +2074,7 @@ static int create_cm(struct rtrs_clt_con *con)
>>> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
>>> goto destroy_cm;
>>> }
>>> +again:
>>> /*
>>> * Combine connection status and session events. This is needed
>>> * for waiting two possible cases: cm_err has something meaningful
>>> @@ -2083,10 +2084,15 @@ static int create_cm(struct rtrs_clt_con *con)
>>> clt_path->state_wq,
>>> con->cm_err || clt_path->state != RTRS_CLT_CONNECTING,
>>> msecs_to_jiffies(RTRS_CONNECT_TIMEOUT_MS));
>>> - if (err == 0 || err == -ERESTARTSYS) {
>>> - if (err == 0)
>>> - err = -ETIMEDOUT;
>>> - /* Timedout or interrupted */
>>> + if (err == -ERESTARTSYS) {
>>> + /* interrupted,
>>> + * try again to avoid the in-flight rtrs_clt_rdma_cm_handler()
>>> + * getting a use-after-free
>>> + */
>>> + goto again;
>>> + } else if (err == 0) {
>>> + err = -ETIMEDOUT;
>>> + /* Timedout */
>>> goto errr;
>>> }
>> Can event handler still be triggered in case of timeout?
> I have never hit such race.
But it is still possible with the theory.
>> And I guess either stop_cm -> rdma_disconnect or destroy_cm -> rdma_destroy_id
>> should prevent this kind of racy issue.
> In practice, they are possible that rtrs_clt_rdma_cm_handler() is in-flight during
> 'either stop_cm -> rdma_disconnect or destroy_cm -> rdma_destroy_id'. rtrs_clt_rdma_cm_handler() and
> cm's cleanup path need to hold mutex_lock(&con->con_mutex), once cm's cleanup path get this lock first
> rtrs_clt_rdma_cm_handler has to sleep, when rtrs_clt_rdma_cm_handler is wakeup again, some resources has been
> freed by cm's cleanup path.
First, stop_cm doesn't need to hold &con->con_mutex. But
rtrs_clt_rdma_cm_handler
is called from rdma core layer which need id_priv->handler_mutexinstead
of con_mutex
I thinnk. Also RTRS has similar behavior as nvme host rdma, ib_srp and
iser_verb.
@Jinpu/Haris, can we move destroy_cm right after stop_cm?*
*Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-11 12:26 ` Leon Romanovsky
@ 2023-04-12 1:15 ` Zhijian Li (Fujitsu)
2023-04-13 7:35 ` Guoqing Jiang
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-12 1:15 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 11/04/2023 20:26, Leon Romanovsky wrote:
> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>
>>>
>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>
>>>>> Precodition: clt_path->s.con_num is 2.
>>>>> So 2 cm connection will be created as below:
>>>>> CPU0 CPU1
>>>>> init_conns { |
>>>>> create_cm() // a. con[0] created |
>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>> | rtrs_rdma_addr_resolved()
>>>>> | create_con_cq_qp(con); << con[0]
>>>>> | }
>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>> |
>>>>> create_cm() // b. cid = 1, failed |
>>>>> destroy_con_cq_qp() |
>>>>> rtrs_ib_dev_put() |
>>>>> dev_free() |
>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>> is destroyed, but refcnt is |
>>>>> still greater than 0 |
>>>>> }
>>>>>
>>>>> Simply, Here we can avoid this warning by introducing conn own flag to
>>>>> track if its cleanup should drop the PD.
>>>>>
>>>>> -----------------------------------------------
>>>>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
>>>>> ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
>>>>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
>>>>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>>>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
>>>>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
>>>>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
>>>>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
>>>>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
>>>>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
>>>>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
>>>>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
>>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>>> Call Trace:
>>>>> <TASK>
>>>>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
>>>>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
>>>>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
>>>>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
>>>>> init_path+0x1e7/0x9a0 [rtrs_client]
>>>>> ? __pfx_autoremove_wake_function+0x10/0x10
>>>>> ? lock_is_held_type+0xd7/0x130
>>>>> ? rcu_read_lock_sched_held+0x43/0x80
>>>>> ? pcpu_alloc+0x3dd/0x7d0
>>>>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
>>>>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
>>>>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
>>>>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>>>>>
>>>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>>>> ---
>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
>>>>> 2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>> index c2065fc33a56..4c8f42e46e2f 100644
>>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>> return -ENOMEM;
>>>>> }
>>>>> clt_path->s.dev_ref = 1;
>>>>> + con->has_dev = true;
>>>>> query_fast_reg_mode(clt_path);
>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>> /*
>>>>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>> /* Shared between connections */
>>>>> clt_path->s.dev_ref++;
>>>> Without looking in the code, I would expect dev_ref from the line above
>>>> to perform PD protection.
>>>
>>> Agreed.
>>
>> Sorry, i didn't get your point. Do you mean something like this:
>>
>> + con->has_dev = true;
>> clt_path->s.dev_ref++;
>
> No, my point was that clt_path->s.dev_ref > 0 means that has_dev is
> equal to true, and dev_ref is supposed to protect from early PD
> destruction.
>
> + if (!con->has_dev)
> + return;
We have already done such protection VVVV
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) { <<< each cleanup will decrease clt_path->s.dev_ref
> rtrs_ib_dev_put(clt_path->s.dev); <<< when it becomes to 0, PD will be destructed.
> clt_path->s.dev = NULL;
But they are not equal, clt_path->s.dev_ref could be shared by multiple connections.
So in the case con[0] successed and con[1] failed(clt_path->s.dev_ref is 1),
the con[1]'s cleanup path(destroy_con_cq_qp) will destroy PD while conn[0] still associates this PD.
Thanks
Zhijian
> Thanks
>
>>
>>
>>
>> Thanks
>> Zhijian
>>
>>>
>>> Thanks,
>>> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-12 1:15 ` Zhijian Li (Fujitsu)
@ 2023-04-13 7:35 ` Guoqing Jiang
2023-04-13 8:12 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-13 7:35 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
Hi,
I take a closer look today.
On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>
> On 11/04/2023 20:26, Leon Romanovsky wrote:
>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>
>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>
>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>
>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>> So 2 cm connection will be created as below:
>>>>>> CPU0 CPU1
>>>>>> init_conns { |
>>>>>> create_cm() // a. con[0] created |
>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>> | rtrs_rdma_addr_resolved()
>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>> | }
>>>>>> | in this moment, refcnt of PD was increased to 2+
What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>>>> |
>>>>>> create_cm() // b. cid = 1, failed |
>>>>>> destroy_con_cq_qp() |
>>>>>> rtrs_ib_dev_put() |
>>>>>> dev_free() |
>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>> is destroyed, but refcnt is |
>>>>>> still greater than 0 |
Assuming you mean "pd->usecnt". We only allocate pd in con[0] by
rtrs_ib_dev_find_or_add,
if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr ->
atomic_inc(&pd->usecnt)
can't be triggered. Is there other places could increase the refcnt?
>>>>>> }
>>>>>>
>>>>>> Simply, Here we can avoid this warning by introducing conn own flag to
>>>>>> track if its cleanup should drop the PD.
>>>>>>
>>>>>> -----------------------------------------------
>>>>>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
>>>>>> ------------[ cut here ]------------
>>>>>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
>>>>>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
>>>>>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
>>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>>>>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
>>>>>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
>>>>>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
>>>>>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
>>>>>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
>>>>>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
>>>>>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
>>>>>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
>>>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
>>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>>>> Call Trace:
>>>>>> <TASK>
>>>>>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
>>>>>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
>>>>>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
>>>>>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
>>>>>> init_path+0x1e7/0x9a0 [rtrs_client]
>>>>>> ? __pfx_autoremove_wake_function+0x10/0x10
>>>>>> ? lock_is_held_type+0xd7/0x130
>>>>>> ? rcu_read_lock_sched_held+0x43/0x80
>>>>>> ? pcpu_alloc+0x3dd/0x7d0
>>>>>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
>>>>>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
>>>>>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
>>>>>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>>>>>>
>>>>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>>>>> ---
>>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
>>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
>>>>>> 2 files changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>> index c2065fc33a56..4c8f42e46e2f 100644
>>>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>>> return -ENOMEM;
>>>>>> }
>>>>>> clt_path->s.dev_ref = 1;
>>>>>> + con->has_dev = true;
>>>>>> query_fast_reg_mode(clt_path);
>>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>>> /*
>>>>>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>>> /* Shared between connections */
>>>>>> clt_path->s.dev_ref++;
>>>>> Without looking in the code, I would expect dev_ref from the line above
>>>>> to perform PD protection.
>>>> Agreed.
>>> Sorry, i didn't get your point. Do you mean something like this:
>>>
>>> + con->has_dev = true;
>>> clt_path->s.dev_ref++;
>> No, my point was that clt_path->s.dev_ref > 0 means that has_dev is
>> equal to true, and dev_ref is supposed to protect from early PD
>> destruction.
>>
>
>> + if (!con->has_dev)
>> + return;
> We have already done such protection VVVV
>
>> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) { <<< each cleanup will decrease clt_path->s.dev_ref
>> rtrs_ib_dev_put(clt_path->s.dev); <<< when it becomes to 0, PD will be destructed.
>> clt_path->s.dev = NULL;
>
> But they are not equal, clt_path->s.dev_ref could be shared by multiple connections.
> So in the case con[0] successed and con[1] failed(clt_path->s.dev_ref is 1),
> the con[1]'s cleanup path(destroy_con_cq_qp) will destroy PD while conn[0] still associates this PD.
Then what is the appropriate time to call destroy_con_cq_qp for this
scenario?
Otherwise there could be memory leak.
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-13 7:35 ` Guoqing Jiang
@ 2023-04-13 8:12 ` Zhijian Li (Fujitsu)
2023-04-13 13:24 ` Leon Romanovsky
2023-04-14 3:40 ` Guoqing Jiang
0 siblings, 2 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-13 8:12 UTC (permalink / raw)
To: Guoqing Jiang, Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 13/04/2023 15:35, Guoqing Jiang wrote:
> Hi,
>
> I take a closer look today.
>
> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>
>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>
>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>
>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>
>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>> So 2 cm connection will be created as below:
>>>>>>> CPU0 CPU1
>>>>>>> init_conns { |
>>>>>>> create_cm() // a. con[0] created |
>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>> | }
>>>>>>> | in this moment, refcnt of PD was increased to 2+
>
> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
I mean usecnt in struct ib_pd
>
>>>>>>> |
>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>> destroy_con_cq_qp() |
>>>>>>> rtrs_ib_dev_put() |
>>>>>>> dev_free() |
>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>> is destroyed, but refcnt is |
>>>>>>> still greater than 0 |
>
> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
> can't be triggered. Is there other places could increase the refcnt?
Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
refcnt is still 1.
> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
> Otherwise there could be memory leak.
we must ensure QP in con[0] is closed before destroying the PD.
Currently destroy_con_cq_qp() subroutine will close the opened QP first.
Thanks
>
>>>>>>> }
>>>>>>>
>>>>>>> Simply, Here we can avoid this warning by introducing conn own flag to
>>>>>>> track if its cleanup should drop the PD.
>>>>>>>
>>>>>>> -----------------------------------------------
>>>>>>> rnbd_client L597: Mapping device /dev/nvme0n1 on session client, (access_mode: rw, nr_poll_queues: 0)
>>>>>>> ------------[ cut here ]------------
>>>>>>> WARNING: CPU: 0 PID: 26407 at drivers/infiniband/sw/rxe/rxe_pool.c:256 __rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>>>> Modules linked in: rpcrdma rdma_ucm ib_iser rnbd_client libiscsi rtrs_client scsi_transport_iscsi rtrs_core rdma_cm iw_cm ib_cm crc32_generic rdma_rxe udp_tunnel ib_uverbs ib_core kmem device_dax nd_pmem dax_pmem nd_
>>>>>>> vme crc32c_intel fuse nvme_core nfit libnvdimm dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_mirror dm_region_hash dm_log dm_mod
>>>>>>> CPU: 0 PID: 26407 Comm: rnbd-client.sh Kdump: loaded Not tainted 6.2.0-rc6-roce-flush+ #53
>>>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>>>>>> RIP: 0010:__rxe_cleanup+0x13a/0x170 [rdma_rxe]
>>>>>>> Code: 45 84 e4 0f 84 5a ff ff ff 48 89 ef e8 5f 18 71 f9 84 c0 75 90 be c8 00 00 00 48 89 ef e8 be 89 1f fa 85 c0 0f 85 7b ff ff ff <0f> 0b 41 bc ea ff ff ff e9 71 ff ff ff e8 84 7f 1f fa e9 d0 fe ff
>>>>>>> RSP: 0018:ffffb09880b6f5f0 EFLAGS: 00010246
>>>>>>> RAX: 0000000000000000 RBX: ffff99401f15d6a8 RCX: 0000000000000000
>>>>>>> RDX: 0000000000000001 RSI: ffffffffbac8234b RDI: 00000000ffffffff
>>>>>>> RBP: ffff99401f15d6d0 R08: 0000000000000001 R09: 0000000000000001
>>>>>>> R10: 0000000000002d82 R11: 0000000000000000 R12: 0000000000000001
>>>>>>> R13: ffff994101eff208 R14: ffffb09880b6f6a0 R15: 00000000fffffe00
>>>>>>> FS: 00007fe113904740(0000) GS:ffff99413bc00000(0000) knlGS:0000000000000000
>>>>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>> CR2: 00007ff6cde656c8 CR3: 000000001f108004 CR4: 00000000001706f0
>>>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>>>>> Call Trace:
>>>>>>> <TASK>
>>>>>>> rxe_dealloc_pd+0x16/0x20 [rdma_rxe]
>>>>>>> ib_dealloc_pd_user+0x4b/0x80 [ib_core]
>>>>>>> rtrs_ib_dev_put+0x79/0xd0 [rtrs_core]
>>>>>>> destroy_con_cq_qp+0x8a/0xa0 [rtrs_client]
>>>>>>> init_path+0x1e7/0x9a0 [rtrs_client]
>>>>>>> ? __pfx_autoremove_wake_function+0x10/0x10
>>>>>>> ? lock_is_held_type+0xd7/0x130
>>>>>>> ? rcu_read_lock_sched_held+0x43/0x80
>>>>>>> ? pcpu_alloc+0x3dd/0x7d0
>>>>>>> ? rtrs_clt_init_stats+0x18/0x40 [rtrs_client]
>>>>>>> rtrs_clt_open+0x24f/0x5a0 [rtrs_client]
>>>>>>> ? __pfx_rnbd_clt_link_ev+0x10/0x10 [rnbd_client]
>>>>>>> rnbd_clt_map_device+0x6a5/0xe10 [rnbd_client]
>>>>>>>
>>>>>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>>>>>> ---
>>>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++++
>>>>>>> drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 +
>>>>>>> 2 files changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>>> index c2065fc33a56..4c8f42e46e2f 100644
>>>>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>>>>> @@ -1664,6 +1664,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>>>> return -ENOMEM;
>>>>>>> }
>>>>>>> clt_path->s.dev_ref = 1;
>>>>>>> + con->has_dev = true;
>>>>>>> query_fast_reg_mode(clt_path);
>>>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>>>> /*
>>>>>>> @@ -1690,6 +1691,7 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
>>>>>>> wr_limit = clt_path->s.dev->ib_dev->attrs.max_qp_wr;
>>>>>>> /* Shared between connections */
>>>>>>> clt_path->s.dev_ref++;
>>>>>> Without looking in the code, I would expect dev_ref from the line above
>>>>>> to perform PD protection.
>>>>> Agreed.
>>>> Sorry, i didn't get your point. Do you mean something like this:
>>>>
>>>> + con->has_dev = true;
>>>> clt_path->s.dev_ref++;
>>> No, my point was that clt_path->s.dev_ref > 0 means that has_dev is
>>> equal to true, and dev_ref is supposed to protect from early PD
>>> destruction.
>>>
>>
>>> + if (!con->has_dev)
>>> + return;
>> We have already done such protection VVVV
>>
>>> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) { <<< each cleanup will decrease clt_path->s.dev_ref
>>> rtrs_ib_dev_put(clt_path->s.dev); <<< when it becomes to 0, PD will be destructed.
>>> clt_path->s.dev = NULL;
>>
>> But they are not equal, clt_path->s.dev_ref could be shared by multiple connections.
>> So in the case con[0] successed and con[1] failed(clt_path->s.dev_ref is 1),
>> the con[1]'s cleanup path(destroy_con_cq_qp) will destroy PD while conn[0] still associates this PD.
>
> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
> Otherwise there could be memory leak.
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-13 8:12 ` Zhijian Li (Fujitsu)
@ 2023-04-13 13:24 ` Leon Romanovsky
2023-04-14 15:58 ` Zhu Yanjun
2023-04-14 3:40 ` Guoqing Jiang
1 sibling, 1 reply; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-13 13:24 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 13/04/2023 15:35, Guoqing Jiang wrote:
> > Hi,
> >
> > I take a closer look today.
> >
> > On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
> >>
> >> On 11/04/2023 20:26, Leon Romanovsky wrote:
> >>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>>
> >>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
> >>>>>
> >>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
> >>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> >>>>>>> The warning occurs when destroying PD whose reference count is not zero.
> >>>>>>>
> >>>>>>> Precodition: clt_path->s.con_num is 2.
> >>>>>>> So 2 cm connection will be created as below:
> >>>>>>> CPU0 CPU1
> >>>>>>> init_conns { |
> >>>>>>> create_cm() // a. con[0] created |
> >>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
> >>>>>>> | rtrs_rdma_addr_resolved()
> >>>>>>> | create_con_cq_qp(con); << con[0]
> >>>>>>> | }
> >>>>>>> | in this moment, refcnt of PD was increased to 2+
> >
> > What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>
> I mean usecnt in struct ib_pd
>
>
>
> >
> >>>>>>> |
> >>>>>>> create_cm() // b. cid = 1, failed |
> >>>>>>> destroy_con_cq_qp() |
> >>>>>>> rtrs_ib_dev_put() |
> >>>>>>> dev_free() |
> >>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
> >>>>>>> is destroyed, but refcnt is |
> >>>>>>> still greater than 0 |
> >
> > Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
> > if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
> > can't be triggered. Is there other places could increase the refcnt?
>
>
> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>
> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
> refcnt is still 1.
Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-13 8:12 ` Zhijian Li (Fujitsu)
2023-04-13 13:24 ` Leon Romanovsky
@ 2023-04-14 3:40 ` Guoqing Jiang
2023-04-14 4:25 ` Bob Pearson
2023-04-14 5:37 ` Zhijian Li (Fujitsu)
1 sibling, 2 replies; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-14 3:40 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
> On 13/04/2023 15:35, Guoqing Jiang wrote:
>> Hi,
>>
>> I take a closer look today.
>>
>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>
>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>> CPU0 CPU1
>>>>>>>> init_conns { |
>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>> | }
>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
> I mean usecnt in struct ib_pd
>
>
>
>>>>>>>> |
>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>> destroy_con_cq_qp() |
>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>> dev_free() |
>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>> is destroyed, but refcnt is |
>>>>>>>> still greater than 0 |
>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
The above can't be invoked, right?
>> can't be triggered. Is there other places could increase the refcnt?
> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>
> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
> refcnt is still 1.
I can see the path increase usecnt to 1.
rtrs_cq_qp_create -> create_qp
-> rdma_create_qp
-> ib_create_qp
-> create_qp
-> ib_qp_usecnt_inc which increases pd->usecnt
Where is another place to increase usecnt to 2?
>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
>> Otherwise there could be memory leak.
> we must ensure QP in con[0] is closed before destroying the PD.
> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
Let me try another way, with below change, rtrs_ib_dev_put can't be called
from destroy_con_cq_qp, right?
+ if (!con->has_dev)
+ return;
if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
rtrs_ib_dev_put(clt_path->s.dev);
clt_path->s.dev = NULL;
Then when will you dealloc pd and free rtrs_ib_dev?
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 3:40 ` Guoqing Jiang
@ 2023-04-14 4:25 ` Bob Pearson
2023-04-14 5:37 ` Zhijian Li (Fujitsu)
1 sibling, 0 replies; 38+ messages in thread
From: Bob Pearson @ 2023-04-14 4:25 UTC (permalink / raw)
To: Guoqing Jiang, Zhijian Li (Fujitsu), Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 4/13/23 22:40, Guoqing Jiang wrote:
>
>
> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>> Hi,
>>>
>>> I take a closer look today.
>>>
>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>
>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>> CPU0 CPU1
>>>>>>>>> init_conns { |
>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>> | }
>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>> I mean usecnt in struct ib_pd
>>
>>
>>
>>>>>>>>> |
>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>> dev_free() |
>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>> still greater than 0 |
>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>
> The above can't be invoked, right?
>
>>> can't be triggered. Is there other places could increase the refcnt?
>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>
>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>> refcnt is still 1.
>
> I can see the path increase usecnt to 1.
>
> rtrs_cq_qp_create -> create_qp
> -> rdma_create_qp
> -> ib_create_qp
> -> create_qp
> -> ib_qp_usecnt_inc which increases pd->usecnt
>
> Where is another place to increase usecnt to 2?
>
>>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
>>> Otherwise there could be memory leak.
>> we must ensure QP in con[0] is closed before destroying the PD.
>> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
>
> Let me try another way, with below change, rtrs_ib_dev_put can't be called
> from destroy_con_cq_qp, right?
>
> + if (!con->has_dev)
> + return;
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> rtrs_ib_dev_put(clt_path->s.dev);
> clt_path->s.dev = NULL;
>
> Then when will you dealloc pd and free rtrs_ib_dev?
>
> Thanks,
> Guoqing
I think that wondering into Leon's reference counting is a really bad idea. Currently the assumed rule
is that rdma-core keeps its ref counts and rxe keeps its. rxe defers the return from rxe_dealloc_pd()
until the rxe ref count drops to zero for that pd, sleeping if necessary. (There is a timeout value set
where rxe will return anyway but it will throw a WARN. If the timeout isn't long enough under heavy load
we could extend it.) If it doesn't happen, or it happens too soon, then there is a ref count bug in rxe
that needs to be fixed.
Fixing rxe ref count bugs is hard enough without entangling rdma-core ref counts into the mix.
Bob
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 3:40 ` Guoqing Jiang
2023-04-14 4:25 ` Bob Pearson
@ 2023-04-14 5:37 ` Zhijian Li (Fujitsu)
2023-04-14 6:03 ` Jinpu Wang
2023-04-14 6:04 ` Guoqing Jiang
1 sibling, 2 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-14 5:37 UTC (permalink / raw)
To: Guoqing Jiang, Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 14/04/2023 11:40, Guoqing Jiang wrote:
>
>
> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>> Hi,
>>>
>>> I take a closer look today.
>>>
>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>
>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>> CPU0 CPU1
>>>>>>>>> init_conns { |
>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>> | }
>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>> I mean usecnt in struct ib_pd
>>
>>
>>
>>>>>>>>> |
>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>> dev_free() |
>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>> still greater than 0 |
>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>
> The above can't be invoked, right?
>
>>> can't be triggered. Is there other places could increase the refcnt?
>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>
>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>> refcnt is still 1.
>
> I can see the path increase usecnt to 1.
>
> rtrs_cq_qp_create -> create_qp
> -> rdma_create_qp
> -> ib_create_qp
> -> create_qp
> -> ib_qp_usecnt_inc which increases pd->usecnt
>
> Where is another place to increase usecnt to 2?
It should be
ib_create_qp ...
-> rxe_create_qp
-> rxe_qp_from_init
-> rxe_get(pd) <<< pd's refcnt will be increased.
>
>>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
>>> Otherwise there could be memory leak.
>> we must ensure QP in con[0] is closed before destroying the PD.
>> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
>
> Let me try another way, with below change, rtrs_ib_dev_put can't be called
> from destroy_con_cq_qp, right?
Not really, con[0]->has_dev is true, so con[0]'s cleanup will call rtrs_ib_dev_put()
Without this patch, when con[1] failed, con[1]'s cleanup will be called first. then call con[0]'s cleanup.
After this change, con[1]'s cleanup will not call rtrs_ib_dev_put, but it will be called the later con[0]'s cleanup.
Thanks
Zhijian
>
> + if (!con->has_dev)
> + return;
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> rtrs_ib_dev_put(clt_path->s.dev);
> clt_path->s.dev = NULL;
>
> Then when will you dealloc pd and free rtrs_ib_dev?
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 5:37 ` Zhijian Li (Fujitsu)
@ 2023-04-14 6:03 ` Jinpu Wang
2023-04-14 6:47 ` Zhijian Li (Fujitsu)
2023-04-14 6:04 ` Guoqing Jiang
1 sibling, 1 reply; 38+ messages in thread
From: Jinpu Wang @ 2023-04-14 6:03 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Guoqing Jiang, Leon Romanovsky, haris.iqbal, jgg, linux-rdma,
linux-kernel
Hi Zhijian, Guoqing, Leon, Bob
First of all, thanks for the patch and discussion.
On Fri, Apr 14, 2023 at 7:37 AM Zhijian Li (Fujitsu)
<lizhijian@fujitsu.com> wrote:
>
>
>
> On 14/04/2023 11:40, Guoqing Jiang wrote:
> >
> >
> > On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
> >> On 13/04/2023 15:35, Guoqing Jiang wrote:
> >>> Hi,
> >>>
> >>> I take a closer look today.
> >>>
> >>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
> >>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
> >>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
> >>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
> >>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> >>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
> >>>>>>>>>
> >>>>>>>>> Precodition: clt_path->s.con_num is 2.
> >>>>>>>>> So 2 cm connection will be created as below:
> >>>>>>>>> CPU0 CPU1
> >>>>>>>>> init_conns { |
> >>>>>>>>> create_cm() // a. con[0] created |
> >>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
> >>>>>>>>> | rtrs_rdma_addr_resolved()
> >>>>>>>>> | create_con_cq_qp(con); << con[0]
> >>>>>>>>> | }
> >>>>>>>>> | in this moment, refcnt of PD was increased to 2+
> >>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
> >> I mean usecnt in struct ib_pd
> >>
> >>
> >>
> >>>>>>>>> |
> >>>>>>>>> create_cm() // b. cid = 1, failed |
> >>>>>>>>> destroy_con_cq_qp() |
> >>>>>>>>> rtrs_ib_dev_put() |
> >>>>>>>>> dev_free() |
> >>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
> >>>>>>>>> is destroyed, but refcnt is |
> >>>>>>>>> still greater than 0 |
> >>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
> >>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
> >
> > The above can't be invoked, right?
> >
> >>> can't be triggered. Is there other places could increase the refcnt?
> >> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
> >>
> >> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
> >> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
> >> refcnt is still 1.
> >
> > I can see the path increase usecnt to 1.
> >
> > rtrs_cq_qp_create -> create_qp
> > -> rdma_create_qp
> > -> ib_create_qp
> > -> create_qp
> > -> ib_qp_usecnt_inc which increases pd->usecnt
> >
> > Where is another place to increase usecnt to 2?
>
> It should be
> ib_create_qp ...
> -> rxe_create_qp
> -> rxe_qp_from_init
> -> rxe_get(pd) <<< pd's refcnt will be increased.
IIUC, this problem is rxe specific, because rxe manipulate refcnt
itself? I checked mlx5/mlx4 they do not change the refcnt of pd when
create_kernel_qp.
So question is then if the bug is on rxe side or rtrs side?
Zhijian how do you reproduce the warning? do you inject error explictly?
Regards!
>
>
> >
> >>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
> >>> Otherwise there could be memory leak.
> >> we must ensure QP in con[0] is closed before destroying the PD.
> >> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
> >
> > Let me try another way, with below change, rtrs_ib_dev_put can't be called
> > from destroy_con_cq_qp, right?
>
> Not really, con[0]->has_dev is true, so con[0]'s cleanup will call rtrs_ib_dev_put()
>
> Without this patch, when con[1] failed, con[1]'s cleanup will be called first. then call con[0]'s cleanup.
> After this change, con[1]'s cleanup will not call rtrs_ib_dev_put, but it will be called the later con[0]'s cleanup.
>
>
> Thanks
> Zhijian
>
> >
> > + if (!con->has_dev)
> > + return;
> > if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> > rtrs_ib_dev_put(clt_path->s.dev);
> > clt_path->s.dev = NULL;
> >
> > Then when will you dealloc pd and free rtrs_ib_dev?
> >
> > Thanks,
> > Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 5:37 ` Zhijian Li (Fujitsu)
2023-04-14 6:03 ` Jinpu Wang
@ 2023-04-14 6:04 ` Guoqing Jiang
2023-04-14 10:09 ` Zhijian Li (Fujitsu)
1 sibling, 1 reply; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-14 6:04 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 4/14/23 13:37, Zhijian Li (Fujitsu) wrote:
>
> On 14/04/2023 11:40, Guoqing Jiang wrote:
>>
>> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>> Hi,
>>>>
>>>> I take a closer look today.
>>>>
>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>
>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>> CPU0 CPU1
>>>>>>>>>> init_conns { |
>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>> | }
>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>> I mean usecnt in struct ib_pd
>>>
>>>
>>>
>>>>>>>>>> |
>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>> dev_free() |
>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>> still greater than 0 |
>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>> The above can't be invoked, right?
>>
>>>> can't be triggered. Is there other places could increase the refcnt?
>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>
>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>> refcnt is still 1.
>> I can see the path increase usecnt to 1.
>>
>> rtrs_cq_qp_create -> create_qp
>> -> rdma_create_qp
>> -> ib_create_qp
>> -> create_qp
>> -> ib_qp_usecnt_inc which increases pd->usecnt
>>
>> Where is another place to increase usecnt to 2?
> It should be
> ib_create_qp ...
> -> rxe_create_qp
> -> rxe_qp_from_init
> -> rxe_get(pd) <<< pd's refcnt will be increased.
Isn't rxe_get just increase elem->ref_cnt?
https://elixir.bootlin.com/linux/v6.3-rc6/source/drivers/infiniband/sw/rxe/rxe_pool.c#L240
>>>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
>>>> Otherwise there could be memory leak.
>>> we must ensure QP in con[0] is closed before destroying the PD.
>>> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
>> Let me try another way, with below change, rtrs_ib_dev_put can't be called
>> from destroy_con_cq_qp, right?
> Not really, con[0]->has_dev is true, so con[0]'s cleanup will call rtrs_ib_dev_put()
>
> Without this patch, when con[1] failed, con[1]'s cleanup will be called first. then call con[0]'s cleanup.
> After this change, con[1]'s cleanup will not call rtrs_ib_dev_put, but it will be called the later con[0]'s cleanup.
But rtrs_ib_dev_put relies on dev_ref, if con[1] returns earlier without
decrease dev_ref
(it is shared among connections), how rtrs_ib_dev_put can be called?
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 6:03 ` Jinpu Wang
@ 2023-04-14 6:47 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-14 6:47 UTC (permalink / raw)
To: Jinpu Wang
Cc: Guoqing Jiang, Leon Romanovsky, haris.iqbal, jgg, linux-rdma,
linux-kernel
On 14/04/2023 14:03, Jinpu Wang wrote:
>>> I can see the path increase usecnt to 1.
>>>
>>> rtrs_cq_qp_create -> create_qp
>>> -> rdma_create_qp
>>> -> ib_create_qp
>>> -> create_qp
>>> -> ib_qp_usecnt_inc which increases pd->usecnt
>>>
>>> Where is another place to increase usecnt to 2?
>> It should be
>> ib_create_qp ...
>> -> rxe_create_qp
>> -> rxe_qp_from_init
>> -> rxe_get(pd) <<< pd's refcnt will be increased.
> IIUC, this problem is rxe specific, because rxe manipulate refcnt
> itself? I checked mlx5/mlx4 they do not change the refcnt of pd when
> create_kernel_qp.
>
> So question is then if the bug is on rxe side or rtrs side?
>
> Zhijian how do you reproduce the warning? do you inject error explictly?
# cat rnbd-self.sh
#!/bin/bash
/root/rpma/tools/config_softroce.sh eth0
modprobe rnbd_server
modprobe rnbd_client
while true;
do
echo "sessname=xyz path=ip:<server-ip> device_path=/dev/nvme0n1" > /sys/devices/virtual/rnbd-client/ctl/map_device
for i in /sys/block/rnbd*/rnbd/unmap_device
do
echo "normal" > $i
done
done
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 6:04 ` Guoqing Jiang
@ 2023-04-14 10:09 ` Zhijian Li (Fujitsu)
2023-04-17 3:08 ` Guoqing Jiang
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-14 10:09 UTC (permalink / raw)
To: Guoqing Jiang, Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 14/04/2023 14:04, Guoqing Jiang wrote:
>
>
> On 4/14/23 13:37, Zhijian Li (Fujitsu) wrote:
>>
>> On 14/04/2023 11:40, Guoqing Jiang wrote:
>>>
>>> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>>> Hi,
>>>>>
>>>>> I take a closer look today.
>>>>>
>>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>>
>>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>>> CPU0 CPU1
>>>>>>>>>>> init_conns { |
>>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>>> | }
>>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>> I mean usecnt in struct ib_pd
>>>>
>>>>
>>>>
>>>>>>>>>>> |
>>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>>> dev_free() |
>>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>>> still greater than 0 |
>>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>> The above can't be invoked, right?
>>>
>>>>> can't be triggered. Is there other places could increase the refcnt?
>>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>>
>>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>>> refcnt is still 1.
>>> I can see the path increase usecnt to 1.
>>>
>>> rtrs_cq_qp_create -> create_qp
>>> -> rdma_create_qp
>>> -> ib_create_qp
>>> -> create_qp
>>> -> ib_qp_usecnt_inc which increases pd->usecnt
>>>
>>> Where is another place to increase usecnt to 2?
>> It should be
>> ib_create_qp ...
>> -> rxe_create_qp
>> -> rxe_qp_from_init
>> -> rxe_get(pd) <<< pd's refcnt will be increased.
>
> Isn't rxe_get just increase elem->ref_cnt?
Yes, that's true.
>
> https://elixir.bootlin.com/linux/v6.3-rc6/source/drivers/infiniband/sw/rxe/rxe_pool.c#L240
>
>>>>> Then what is the appropriate time to call destroy_con_cq_qp for this scenario?
>>>>> Otherwise there could be memory leak.
>>>> we must ensure QP in con[0] is closed before destroying the PD.
>>>> Currently destroy_con_cq_qp() subroutine will close the opened QP first.
>>> Let me try another way, with below change, rtrs_ib_dev_put can't be called
>>> from destroy_con_cq_qp, right?
>> Not really, con[0]->has_dev is true, so con[0]'s cleanup will call rtrs_ib_dev_put()
>>
>> Without this patch, when con[1] failed, con[1]'s cleanup will be called first. then call con[0]'s cleanup.
>> After this change, con[1]'s cleanup will not call rtrs_ib_dev_put, but it will be called the later con[0]'s cleanup.
>
> But rtrs_ib_dev_put relies on dev_ref, if con[1] returns earlier without decrease dev_ref
> (it is shared among connections), how rtrs_ib_dev_put can be called?
>
we must ensure each connections that take dev_ref decrease dev_ref during its cleanup path.
So the new flag con->has_dev added to track if the con has taken the dev_ref.
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-13 13:24 ` Leon Romanovsky
@ 2023-04-14 15:58 ` Zhu Yanjun
2023-04-17 2:18 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Zhu Yanjun @ 2023-04-14 15:58 UTC (permalink / raw)
To: Leon Romanovsky, Zhijian Li (Fujitsu)
Cc: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
在 2023/4/13 21:24, Leon Romanovsky 写道:
> On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>> Hi,
>>>
>>> I take a closer look today.
>>>
>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>
>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>
>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>
>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>
>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>> CPU0 CPU1
>>>>>>>>> init_conns { |
>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>> | }
>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>
>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>
>> I mean usecnt in struct ib_pd
>>
>>
>>
>>>
>>>>>>>>> |
>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>> dev_free() |
>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>> still greater than 0 |
>>>
>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>> can't be triggered. Is there other places could increase the refcnt?
>>
>>
>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>
>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>> refcnt is still 1.
>
> Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
Agree. We should find out why refcnt 1 and fix this problem.
Zhu Yanjun
>
> Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 15:58 ` Zhu Yanjun
@ 2023-04-17 2:18 ` Zhijian Li (Fujitsu)
2023-04-17 18:04 ` Leon Romanovsky
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-17 2:18 UTC (permalink / raw)
To: Zhu Yanjun, Leon Romanovsky
Cc: Guoqing Jiang, haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 14/04/2023 23:58, Zhu Yanjun wrote:
> 在 2023/4/13 21:24, Leon Romanovsky 写道:
>> On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
>>>
>>>
>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>> Hi,
>>>>
>>>> I take a closer look today.
>>>>
>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>>
>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>>
>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>>
>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>
>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>> CPU0 CPU1
>>>>>>>>>> init_conns { |
>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>> | }
>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>
>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>
>>> I mean usecnt in struct ib_pd
>>>
>>>
>>>
>>>>
>>>>>>>>>> |
>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>> dev_free() |
>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>> still greater than 0 |
>>>>
>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>>> can't be triggered. Is there other places could increase the refcnt?
>>>
>>>
>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>
>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>> refcnt is still 1.
>>
>> Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
> + if (!con->has_dev)
> + return;
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> rtrs_ib_dev_put(clt_path->s.dev);
> clt_path->s.dev = NULL;
Currently, without this patch:
1. PD and clt_path->s.dev are shared among connections.
2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
clt_path->s.dev become zero, it will destroy PD.
4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
Thanks
Zhijian
> Agree. We should find out why refcnt 1 and fix this problem.
>
> Zhu Yanjun
>>
>> Thanks
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-14 10:09 ` Zhijian Li (Fujitsu)
@ 2023-04-17 3:08 ` Guoqing Jiang
2023-04-18 6:47 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Guoqing Jiang @ 2023-04-17 3:08 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 4/14/23 18:09, Zhijian Li (Fujitsu) wrote:
>
> On 14/04/2023 14:04, Guoqing Jiang wrote:
>>
>> On 4/14/23 13:37, Zhijian Li (Fujitsu) wrote:
>>> On 14/04/2023 11:40, Guoqing Jiang wrote:
>>>> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>>>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I take a closer look today.
>>>>>>
>>>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>>>
>>>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>>>> CPU0 CPU1
>>>>>>>>>>>> init_conns { |
>>>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>>>> | }
>>>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>>> I mean usecnt in struct ib_pd
>>>>>
>>>>>>>>>>>> |
>>>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>>>> dev_free() |
>>>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>>>> still greater than 0 |
>>>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>>> The above can't be invoked, right?
>>>>
>>>>>> can't be triggered. Is there other places could increase the refcnt?
>>>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>>>
>>>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>>>> refcnt is still 1.
>>>> I can see the path increase usecnt to 1.
>>>>
>>>> rtrs_cq_qp_create -> create_qp
>>>> -> rdma_create_qp
>>>> -> ib_create_qp
>>>> -> create_qp
>>>> -> ib_qp_usecnt_inc which increases pd->usecnt
>>>>
>>>> Where is another place to increase usecnt to 2?
>>> It should be
>>> ib_create_qp ...
>>> -> rxe_create_qp
>>> -> rxe_qp_from_init
>>> -> rxe_get(pd) <<< pd's refcnt will be increased.
>> Isn't rxe_get just increase elem->ref_cnt?
> Yes, that's true.
I am confused, does increase ref_cnt equal to increase usecnt?
If not, then where is another place to increase usecnt to 2?
BTW, I traced with 6.3-rc5, seems pd's usecnt is only increase once
after create one connection.
[ 6941.525088] in init_conns 2353 con_num=3
[ 6941.525732] in create_con_cq_qp 1648
[ 6941.525944] in rtrs_cq_qp_create 311 con->cid=0
path->dev->ib_pd->usecnt=1
[ 6941.532460] in create_con_cq_qp 1648
[ 6941.532746] in rtrs_cq_qp_create 311 con->cid=1
path->dev->ib_pd->usecnt=2
[ 6941.533183] in create_con_cq_qp 1648
[ 6941.533464] in rtrs_cq_qp_create 311 con->cid=2
path->dev->ib_pd->usecnt=3
[ 6941.533685] in init_conns 2365, clt_path->s.dev->ib_pd->usecnt=3
[ 6941.535680] in init_conns 2371, clt_path->s.dev->ib_pd->usecnt=515
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-17 2:18 ` Zhijian Li (Fujitsu)
@ 2023-04-17 18:04 ` Leon Romanovsky
2023-04-18 7:04 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-17 18:04 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Zhu Yanjun, Guoqing Jiang, haris.iqbal, jinpu.wang, jgg,
linux-rdma, linux-kernel
On Mon, Apr 17, 2023 at 02:18:24AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 14/04/2023 23:58, Zhu Yanjun wrote:
> > 在 2023/4/13 21:24, Leon Romanovsky 写道:
> >> On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>
> >>>
> >>> On 13/04/2023 15:35, Guoqing Jiang wrote:
> >>>> Hi,
> >>>>
> >>>> I take a closer look today.
> >>>>
> >>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
> >>>>>
> >>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
> >>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>>>>>
> >>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
> >>>>>>>>
> >>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
> >>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> >>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
> >>>>>>>>>>
> >>>>>>>>>> Precodition: clt_path->s.con_num is 2.
> >>>>>>>>>> So 2 cm connection will be created as below:
> >>>>>>>>>> CPU0 CPU1
> >>>>>>>>>> init_conns { |
> >>>>>>>>>> create_cm() // a. con[0] created |
> >>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
> >>>>>>>>>> | rtrs_rdma_addr_resolved()
> >>>>>>>>>> | create_con_cq_qp(con); << con[0]
> >>>>>>>>>> | }
> >>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
> >>>>
> >>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
> >>>
> >>> I mean usecnt in struct ib_pd
> >>>
> >>>
> >>>
> >>>>
> >>>>>>>>>> |
> >>>>>>>>>> create_cm() // b. cid = 1, failed |
> >>>>>>>>>> destroy_con_cq_qp() |
> >>>>>>>>>> rtrs_ib_dev_put() |
> >>>>>>>>>> dev_free() |
> >>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
> >>>>>>>>>> is destroyed, but refcnt is |
> >>>>>>>>>> still greater than 0 |
> >>>>
> >>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
> >>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
> >>>> can't be triggered. Is there other places could increase the refcnt?
> >>>
> >>>
> >>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
> >>>
> >>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
> >>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
> >>> refcnt is still 1.
> >>
> >> Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
>
>
>
> > + if (!con->has_dev)
> > + return;
> > if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> > rtrs_ib_dev_put(clt_path->s.dev);
> > clt_path->s.dev = NULL;
>
> Currently, without this patch:
> 1. PD and clt_path->s.dev are shared among connections.
> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
> clt_path->s.dev become zero, it will destroy PD.
> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
So please fix it by making sure that failure to create con[1] will
release resources which were allocated. If con[1] didn't increase
s.dev_ref, it shouldn't decrease it either.
Thanks
>
>
> Thanks
> Zhijian
>
> > Agree. We should find out why refcnt 1 and fix this problem.
>
>
>
>
> >
> > Zhu Yanjun
> >>
> >> Thanks
> >
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-17 3:08 ` Guoqing Jiang
@ 2023-04-18 6:47 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-18 6:47 UTC (permalink / raw)
To: Guoqing Jiang, Leon Romanovsky
Cc: haris.iqbal, jinpu.wang, jgg, linux-rdma, linux-kernel
On 17/04/2023 11:08, Guoqing Jiang wrote:
>
>
> On 4/14/23 18:09, Zhijian Li (Fujitsu) wrote:
>>
>> On 14/04/2023 14:04, Guoqing Jiang wrote:
>>>
>>> On 4/14/23 13:37, Zhijian Li (Fujitsu) wrote:
>>>> On 14/04/2023 11:40, Guoqing Jiang wrote:
>>>>> On 4/13/23 16:12, Zhijian Li (Fujitsu) wrote:
>>>>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I take a closer look today.
>>>>>>>
>>>>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>>>>> CPU0 CPU1
>>>>>>>>>>>>> init_conns { |
>>>>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>>>>> | }
>>>>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>>>> I mean usecnt in struct ib_pd
>>>>>>
>>>>>>>>>>>>> |
>>>>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>>>>> dev_free() |
>>>>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>>>>> still greater than 0 |
>>>>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>>>> The above can't be invoked, right?
>>>>>
>>>>>>> can't be triggered. Is there other places could increase the refcnt?
>>>>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>>>>
>>>>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>>>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>>>>> refcnt is still 1.
>>>>> I can see the path increase usecnt to 1.
>>>>>
>>>>> rtrs_cq_qp_create -> create_qp
>>>>> -> rdma_create_qp
>>>>> -> ib_create_qp
>>>>> -> create_qp
>>>>> -> ib_qp_usecnt_inc which increases pd->usecnt
>>>>>
>>>>> Where is another place to increase usecnt to 2?
>>>> It should be
>>>> ib_create_qp ...
>>>> -> rxe_create_qp
>>>> -> rxe_qp_from_init
>>>> -> rxe_get(pd) <<< pd's refcnt will be increased.
>>> Isn't rxe_get just increase elem->ref_cnt?
>> Yes, that's true.
>
> I am confused, does increase ref_cnt equal to increase usecnt?
I need to apologize for my mistake. I have been referring to the elem.ref_cnt of the rxe driver as the refcnt of PD.
> If not, then where is another place to increase usecnt to 2?
>
> BTW, I traced with 6.3-rc5, seems pd's usecnt is only increase once
> after create one connection.
And the warning mentioned above it also pointed to the PD's elem.ref_cnt.
>
> [ 6941.525088] in init_conns 2353 con_num=3
> [ 6941.525732] in create_con_cq_qp 1648
> [ 6941.525944] in rtrs_cq_qp_create 311 con->cid=0 path->dev->ib_pd->usecnt=1
> [ 6941.532460] in create_con_cq_qp 1648
> [ 6941.532746] in rtrs_cq_qp_create 311 con->cid=1 path->dev->ib_pd->usecnt=2
> [ 6941.533183] in create_con_cq_qp 1648
> [ 6941.533464] in rtrs_cq_qp_create 311 con->cid=2 path->dev->ib_pd->usecnt=3
> [ 6941.533685] in init_conns 2365, clt_path->s.dev->ib_pd->usecnt=3
> [ 6941.535680] in init_conns 2371, clt_path->s.dev->ib_pd->usecnt=515
Thanks
Zhijian
below is a piece of code that i used to debug this issue.
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1730,15 +1730,39 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
return err;
}
+struct rxe_pool;
+struct rxe_pool_elem {
+ struct rxe_pool *pool;
+ void *obj;
+ struct kref ref_cnt;
+ struct list_head list;
+ struct completion complete;
+ u32 index;
+};
+
+struct rxe_pd {
+ struct ib_pd ibpd;
+ struct rxe_pool_elem elem;
+};
+
+static inline struct rxe_pd *to_rpd(struct ib_pd *pd)
+{
+ return pd ? container_of(pd, struct rxe_pd, ibpd) : NULL;
+}
+
+#define rxe_read(obj) kref_read(&(obj)->elem.ref_cnt)
static void destroy_con_cq_qp(struct rtrs_clt_con *con)
{
struct rtrs_clt_path *clt_path = to_clt_path(con->c.path);
+ struct rtrs_ib_dev *dev = clt_path->s.dev;
+ struct rxe_pd *pd = to_rpd(dev->ib_pd);
/*
* Be careful here: destroy_con_cq_qp() can be called even
* create_con_cq_qp() failed, see comments there.
*/
lockdep_assert_held(&con->con_mutex);
+ rtrs_info(clt_path->clt, "%s: clt_path->s.dev_ref: %d, pd %px, ref: %d\n", __func__, clt_path->s.dev_ref, &pd->elem, rxe_read(pd));
rtrs_cq_qp_destroy(&con->c);
if (con->rsp_ius) {
rtrs_iu_free(con->rsp_ius, clt_path->s.dev->ib_dev,
@@ -1746,7 +1770,8 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
con->rsp_ius = NULL;
con->queue_num = 0;
}
+ rtrs_info(clt_path->clt, "%s: clt_path->s.dev_ref: %d, pd %px, ref: %d\n", __func__, clt_path->s.dev_ref, &pd->elem, rxe_read(pd));
if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
rtrs_ib_dev_put(clt_path->s.dev);
clt_path->s.dev = NULL;
>
> Thanks,
> Guoqing
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-17 18:04 ` Leon Romanovsky
@ 2023-04-18 7:04 ` Zhijian Li (Fujitsu)
2023-04-18 7:57 ` Leon Romanovsky
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-18 7:04 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Zhu Yanjun, Guoqing Jiang, haris.iqbal, jinpu.wang, jgg,
linux-rdma, linux-kernel
On 18/04/2023 02:04, Leon Romanovsky wrote:
> On Mon, Apr 17, 2023 at 02:18:24AM +0000, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 14/04/2023 23:58, Zhu Yanjun wrote:
>>> 在 2023/4/13 21:24, Leon Romanovsky 写道:
>>>> On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>
>>>>>
>>>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I take a closer look today.
>>>>>>
>>>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
>>>>>>>
>>>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
>>>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
>>>>>>>>>
>>>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
>>>>>>>>>>
>>>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
>>>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
>>>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
>>>>>>>>>>>>
>>>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
>>>>>>>>>>>> So 2 cm connection will be created as below:
>>>>>>>>>>>> CPU0 CPU1
>>>>>>>>>>>> init_conns { |
>>>>>>>>>>>> create_cm() // a. con[0] created |
>>>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
>>>>>>>>>>>> | rtrs_rdma_addr_resolved()
>>>>>>>>>>>> | create_con_cq_qp(con); << con[0]
>>>>>>>>>>>> | }
>>>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
>>>>>>
>>>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
>>>>>
>>>>> I mean usecnt in struct ib_pd
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>>>>>> |
>>>>>>>>>>>> create_cm() // b. cid = 1, failed |
>>>>>>>>>>>> destroy_con_cq_qp() |
>>>>>>>>>>>> rtrs_ib_dev_put() |
>>>>>>>>>>>> dev_free() |
>>>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
>>>>>>>>>>>> is destroyed, but refcnt is |
>>>>>>>>>>>> still greater than 0 |
>>>>>>
>>>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
>>>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
>>>>>> can't be triggered. Is there other places could increase the refcnt?
>>>>>
>>>>>
>>>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
>>>>>
>>>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
>>>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
>>>>> refcnt is still 1.
>>>>
>>>> Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
>>
>>
>>
>>> + if (!con->has_dev)
>>> + return;
>>> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
>>> rtrs_ib_dev_put(clt_path->s.dev);
>>> clt_path->s.dev = NULL;
>>
>> Currently, without this patch:
>> 1. PD and clt_path->s.dev are shared among connections.
>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>> clt_path->s.dev become zero, it will destroy PD.
>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>
> So please fix it by making sure that failure to create con[1] will
> release resources which were allocated. If con[1] didn't increase
> s.dev_ref, it shouldn't decrease it either.
You are right, the current patch did exactly that.
It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
so that its cleanup phase will only decrease its s.dev properly.
Thanks
Zhijian
>
> Thanks
>
>>
>>
>> Thanks
>> Zhijian
>>
>>> Agree. We should find out why refcnt 1 and fix this problem.
>>
>>
>>
>>
>>>
>>> Zhu Yanjun
>>>>
>>>> Thanks
>>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-18 7:04 ` Zhijian Li (Fujitsu)
@ 2023-04-18 7:57 ` Leon Romanovsky
2023-04-19 9:53 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Leon Romanovsky @ 2023-04-18 7:57 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Zhu Yanjun, Guoqing Jiang, haris.iqbal, jinpu.wang, jgg,
linux-rdma, linux-kernel
On Tue, Apr 18, 2023 at 07:04:00AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 18/04/2023 02:04, Leon Romanovsky wrote:
> > On Mon, Apr 17, 2023 at 02:18:24AM +0000, Zhijian Li (Fujitsu) wrote:
> >>
> >>
> >> On 14/04/2023 23:58, Zhu Yanjun wrote:
> >>> 在 2023/4/13 21:24, Leon Romanovsky 写道:
> >>>> On Thu, Apr 13, 2023 at 08:12:15AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>>>
> >>>>>
> >>>>> On 13/04/2023 15:35, Guoqing Jiang wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I take a closer look today.
> >>>>>>
> >>>>>> On 4/12/23 09:15, Zhijian Li (Fujitsu) wrote:
> >>>>>>>
> >>>>>>> On 11/04/2023 20:26, Leon Romanovsky wrote:
> >>>>>>>> On Tue, Apr 11, 2023 at 02:43:46AM +0000, Zhijian Li (Fujitsu) wrote:
> >>>>>>>>>
> >>>>>>>>> On 10/04/2023 21:10, Guoqing Jiang wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 4/10/23 20:08, Leon Romanovsky wrote:
> >>>>>>>>>>> On Mon, Apr 10, 2023 at 06:43:03AM +0000, Li Zhijian wrote:
> >>>>>>>>>>>> The warning occurs when destroying PD whose reference count is not zero.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Precodition: clt_path->s.con_num is 2.
> >>>>>>>>>>>> So 2 cm connection will be created as below:
> >>>>>>>>>>>> CPU0 CPU1
> >>>>>>>>>>>> init_conns { |
> >>>>>>>>>>>> create_cm() // a. con[0] created |
> >>>>>>>>>>>> | a'. rtrs_clt_rdma_cm_handler() {
> >>>>>>>>>>>> | rtrs_rdma_addr_resolved()
> >>>>>>>>>>>> | create_con_cq_qp(con); << con[0]
> >>>>>>>>>>>> | }
> >>>>>>>>>>>> | in this moment, refcnt of PD was increased to 2+
> >>>>>>
> >>>>>> What do you mean "refcnt of PD"? usecnt in struct ib_pd or dev_ref.
> >>>>>
> >>>>> I mean usecnt in struct ib_pd
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>>>>>>>> |
> >>>>>>>>>>>> create_cm() // b. cid = 1, failed |
> >>>>>>>>>>>> destroy_con_cq_qp() |
> >>>>>>>>>>>> rtrs_ib_dev_put() |
> >>>>>>>>>>>> dev_free() |
> >>>>>>>>>>>> ib_dealloc_pd(dev->ib_pd) << PD |
> >>>>>>>>>>>> is destroyed, but refcnt is |
> >>>>>>>>>>>> still greater than 0 |
> >>>>>>
> >>>>>> Assuming you mean "pd->usecnt". We only allocate pd in con[0] by rtrs_ib_dev_find_or_add,
> >>>>>> if con[1] failed to create cm, then alloc_path_reqs -> ib_alloc_mr -> atomic_inc(&pd->usecnt)
> >>>>>> can't be triggered. Is there other places could increase the refcnt?
> >>>>>
> >>>>>
> >>>>> Yes, when create a qp, it will also associate to this PD, that also mean refcnt of PD will be increased.
> >>>>>
> >>>>> When con[0](create_con_cq_qp) succeeded, refcnt of PD will be 2. and then when con[1] failed, since
> >>>>> QP didn't create, refcnt of PD is still 2. con[1]'s cleanup will destroy the PD(ib_dealloc_pd) since dev_ref = 1, after that its
> >>>>> refcnt is still 1.
> >>>>
> >>>> Why is refcnt 1 in con[1] destruction phase? It seems to me like a bug.
> >>
> >>
> >>
> >>> + if (!con->has_dev)
> >>> + return;
> >>> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> >>> rtrs_ib_dev_put(clt_path->s.dev);
> >>> clt_path->s.dev = NULL;
> >>
> >> Currently, without this patch:
> >> 1. PD and clt_path->s.dev are shared among connections.
> >> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> >> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
> >> clt_path->s.dev become zero, it will destroy PD.
> >> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
> >
> > So please fix it by making sure that failure to create con[1] will
> > release resources which were allocated. If con[1] didn't increase
> > s.dev_ref, it shouldn't decrease it either.
>
> You are right, the current patch did exactly that.
> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
> so that its cleanup phase will only decrease its s.dev properly.
The has_dev is a workaround and not a solution. In proper error unwind
sequence, you won't need extra flag.
Thanks
>
> Thanks
> Zhijian
>
>
> >
> > Thanks
> >
> >>
> >>
> >> Thanks
> >> Zhijian
> >>
> >>> Agree. We should find out why refcnt 1 and fix this problem.
> >>
> >>
> >>
> >>
> >>>
> >>> Zhu Yanjun
> >>>>
> >>>> Thanks
> >>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-18 7:57 ` Leon Romanovsky
@ 2023-04-19 9:53 ` Zhijian Li (Fujitsu)
2023-04-19 13:20 ` Jinpu Wang
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-19 9:53 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Zhu Yanjun, Guoqing Jiang, haris.iqbal, jinpu.wang, jgg,
linux-rdma, linux-kernel
Leon, Guoqing
On 18/04/2023 15:57, Leon Romanovsky wrote:
>>>> Currently, without this patch:
>>>> 1. PD and clt_path->s.dev are shared among connections.
>>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>>>> clt_path->s.dev become zero, it will destroy PD.
>>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>>> So please fix it by making sure that failure to create con[1] will
>>> release resources which were allocated. If con[1] didn't increase
>>> s.dev_ref, it shouldn't decrease it either.
>> You are right, the current patch did exactly that.
>> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
>> so that its cleanup phase will only decrease its s.dev properly.
> The has_dev is a workaround and not a solution. In proper error unwind
> sequence, you won't need extra flag.
>
> Thanks
>
how about below changes
commit 61dba725384e226d472b8142d70d40d4103df87a
Author: Li Zhijian <lizhijian@fujitsu.com>
Date: Wed Apr 19 17:42:26 2023 +0800
RDMA/rtrs: Fix rxe_dealloc_pd warning
con[0] always sets s.dev to 1, correspondingly, we should let it to
release the last dev.
Previously,
1. PD and clt_path->s.dev are shared among connections.
2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
clt_path->s.dev become zero, it will destroy PD.
4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
but it try to decreased clt_path->s.dev <<< it's wrong to do that.
The warning occurs when destroying PD whose reference count is not zero.
Precodition: clt_path->s.con_num is 2.
So 2 cm connection will be created as below:
CPU0 CPU1
init_conns { |
create_cm() // a. con[0] created |
| a'. rtrs_clt_rdma_cm_handler() {
| rtrs_rdma_addr_resolved()
| create_con_cq_qp(con); << con[0]
| }
| in this moment, refcnt of PD was increased to 2+
|
create_cm() // b. cid = 1, failed |
destroy_con_cq_qp() |
rtrs_ib_dev_put() |
dev_free() |
ib_dealloc_pd(dev->ib_pd) << PD |
is destroyed, but refcnt is |
still greater than 0 |
}
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 80abf45a197a..1eb652dedca3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1743,6 +1743,15 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
con->rsp_ius = NULL;
con->queue_num = 0;
}
+
+ /*
+ * Every con will try to decreased s.dev_ref, but we should
+ * reserve the last s.dev_ref for con[0]. In case con[1+]'s
+ * cleanup phase call rtrs_ib_dev_put(clt_path->s.dev) early.
+ */
+ if (con->c.cid != 0 && clt_path->s.dev_ref == 1)
+ return;
+
if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
rtrs_ib_dev_put(clt_path->s.dev);
clt_path->s.dev = NULL;
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
2023-04-10 13:09 ` Guoqing Jiang
@ 2023-04-19 10:37 ` Jinpu Wang
1 sibling, 0 replies; 38+ messages in thread
From: Jinpu Wang @ 2023-04-19 10:37 UTC (permalink / raw)
To: Li Zhijian
Cc: haris.iqbal, jgg, leon, linux-rdma, guoqing.jiang, linux-kernel
On Mon, Apr 10, 2023 at 8:43 AM Li Zhijian <lizhijian@fujitsu.com> wrote:
>
> line 1701 and 1713 are duplicate:
> > 1701 cq_num = max_send_wr + max_recv_wr;
> 1702 /* alloc iu to recv new rkey reply when server reports flags set */
> 1703 if (clt_path->flags & RTRS_MSG_NEW_RKEY_F || con->c.cid == 0) {
> 1704 con->rsp_ius = rtrs_iu_alloc(cq_num, sizeof(*rsp),
> 1705 GFP_KERNEL,
> 1706 clt_path->s.dev->ib_dev,
> 1707 DMA_FROM_DEVICE,
> 1708 rtrs_clt_rdma_done);
> 1709 if (!con->rsp_ius)
> 1710 return -ENOMEM;
> 1711 con->queue_num = cq_num;
> 1712 }
> > 1713 cq_num = max_send_wr + max_recv_wr;
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Thx!
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 80abf45a197a..c2065fc33a56 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1710,7 +1710,6 @@ static int create_con_cq_qp(struct rtrs_clt_con *con)
> return -ENOMEM;
> con->queue_num = cq_num;
> }
> - cq_num = max_send_wr + max_recv_wr;
> cq_vector = con->cpu % clt_path->s.dev->ib_dev->num_comp_vectors;
> if (con->c.cid >= clt_path->s.irq_con_num)
> err = rtrs_cq_qp_create(&clt_path->s, &con->c, max_send_sge,
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-19 9:53 ` Zhijian Li (Fujitsu)
@ 2023-04-19 13:20 ` Jinpu Wang
2023-04-20 2:00 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Jinpu Wang @ 2023-04-19 13:20 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Leon Romanovsky, Zhu Yanjun, Guoqing Jiang, haris.iqbal, jgg,
linux-rdma, linux-kernel
On Wed, Apr 19, 2023 at 11:53 AM Zhijian Li (Fujitsu)
<lizhijian@fujitsu.com> wrote:
>
> Leon, Guoqing
>
>
> On 18/04/2023 15:57, Leon Romanovsky wrote:
> >>>> Currently, without this patch:
> >>>> 1. PD and clt_path->s.dev are shared among connections.
> >>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> >>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
> >>>> clt_path->s.dev become zero, it will destroy PD.
> >>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
> >>> So please fix it by making sure that failure to create con[1] will
> >>> release resources which were allocated. If con[1] didn't increase
> >>> s.dev_ref, it shouldn't decrease it either.
> >> You are right, the current patch did exactly that.
> >> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
> >> so that its cleanup phase will only decrease its s.dev properly.
> > The has_dev is a workaround and not a solution. In proper error unwind
> > sequence, you won't need extra flag.
> >
> > Thanks
> >
>
> how about below changes
>
> commit 61dba725384e226d472b8142d70d40d4103df87a
> Author: Li Zhijian <lizhijian@fujitsu.com>
> Date: Wed Apr 19 17:42:26 2023 +0800
>
> RDMA/rtrs: Fix rxe_dealloc_pd warning
>
> con[0] always sets s.dev to 1, correspondingly, we should let it to
> release the last dev.
>
> Previously,
> 1. PD and clt_path->s.dev are shared among connections.
> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
> clt_path->s.dev become zero, it will destroy PD.
> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
> but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>
> The warning occurs when destroying PD whose reference count is not zero.
> Precodition: clt_path->s.con_num is 2.
> So 2 cm connection will be created as below:
> CPU0 CPU1
> init_conns { |
> create_cm() // a. con[0] created |
> | a'. rtrs_clt_rdma_cm_handler() {
> | rtrs_rdma_addr_resolved()
> | create_con_cq_qp(con); << con[0]
> | }
> | in this moment, refcnt of PD was increased to 2+
> |
> create_cm() // b. cid = 1, failed |
> destroy_con_cq_qp() |
> rtrs_ib_dev_put() |
> dev_free() |
> ib_dealloc_pd(dev->ib_pd) << PD |
> is destroyed, but refcnt is |
> still greater than 0 |
> }
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 80abf45a197a..1eb652dedca3 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1743,6 +1743,15 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
> con->rsp_ius = NULL;
> con->queue_num = 0;
> }
> +
> + /*
> + * Every con will try to decreased s.dev_ref, but we should
> + * reserve the last s.dev_ref for con[0]. In case con[1+]'s
> + * cleanup phase call rtrs_ib_dev_put(clt_path->s.dev) early.
> + */
> + if (con->c.cid != 0 && clt_path->s.dev_ref == 1)
> + return;
> +
> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
> rtrs_ib_dev_put(clt_path->s.dev);
> clt_path->s.dev = NULL;
I run a regression test in our test env, it triggers a warning on
1681 if (WARN_ON(clt_path->s.dev))
[ 1333.042633] ------------[ cut here ]------------
[ 1333.042650] WARNING: CPU: 8 PID: 559 at
/root/kernel-test/ibnbd2/rtrs/rtrs-clt.c:1681
rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
[ 1333.042651] Modules linked in: loop rnbd_client(O) rtrs_client(O)
rtrs_core(O) kvm_amd kvm input_leds led_class irqbypass crc32_pclmul
aesni_intel sp5100_tco evdev libaes watchdog sg k10temp crypto_simd
fam15h_power ipmi_si serio_raw cryptd ipmi_devintf glue_helper
ipmi_msghandler acpi_cpufreq button ib_ipoib ib_umad null_blk brd
rdma_cm iw_cm ib_cm ip_tables x_tables autofs4 raid10 raid456
async_raid6_recov async_memcpy async_pq async_xor async_tx xor
raid6_pq libcrc32c raid1 raid0 linear mlx4_ib md_mod ib_uverbs ib_core
sd_mod t10_pi crc_t10dif crct10dif_generic ahci libahci
crct10dif_pclmul crct10dif_common crc32c_intel igb libata usb_storage
psmouse i2c_piix4 i2c_algo_bit mlx4_core dca scsi_mod i2c_core ptp
pps_core
[ 1333.042737] CPU: 8 PID: 559 Comm: kworker/u128:1 Tainted: G
O 5.10.136-pserver-develop-5.10 #257
[ 1333.042738] Hardware name: Supermicro H8QG6/H8QG6, BIOS 3.00 09/04/2012
[ 1333.042752] Workqueue: rdma_cm cma_work_handler [rdma_cm]
[ 1333.042758] RIP: 0010:rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
[ 1333.042761] Code: ff bb ea ff ff ff e8 db a5 24 fc 49 8d b4 24 10
01 00 00 89 da 48 c7 c7 40 93 5b c0 e8 4b 47 21 fc 4d 8b 65 00 e9 15
fe ff ff <0f> 0b 4c 89 ff bb ea ff ff ff e8 ad a5 24 fc eb d0 0f 0b 4c
89 ff
[ 1333.042763] RSP: 0018:ffffaff68e57bdb0 EFLAGS: 00010286
[ 1333.042765] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff9eddc0051420
[ 1333.042767] RDX: ffff9ee4ef716e40 RSI: ffff9f14ea288f30 RDI: ffff9eddc88db240
[ 1333.042768] RBP: ffffaff68e57be50 R08: 0000000000000000 R09: 006d635f616d6472
[ 1333.042769] R10: ffffaff68e57be68 R11: 0000000000000000 R12: ffff9edde1388000
[ 1333.042771] R13: ffff9eddc88db200 R14: ffff9edde1388000 R15: ffff9eddc88db240
[ 1333.042773] FS: 0000000000000000(0000) GS:ffff9eecc7c00000(0000)
knlGS:0000000000000000
[ 1333.042774] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1333.042776] CR2: 00007f0ac4ed4004 CR3: 0000002b5040a000 CR4: 00000000000406e0
[ 1333.042777] Call Trace:
[ 1333.042790] ? newidle_balance+0x25e/0x3c0
[ 1333.042795] ? psi_group_change+0x43/0x230
[ 1333.042801] ? cma_cm_event_handler+0x23/0xb0 [rdma_cm]
[ 1333.042807] cma_cm_event_handler+0x23/0xb0 [rdma_cm]
[ 1333.042814] cma_work_handler+0x5a/0xb0 [rdma_cm]
[ 1333.042819] process_one_work+0x1f3/0x390
[ 1333.042822] worker_thread+0x2d/0x3c0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-19 13:20 ` Jinpu Wang
@ 2023-04-20 2:00 ` Zhijian Li (Fujitsu)
2023-04-21 1:38 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-20 2:00 UTC (permalink / raw)
To: Jinpu Wang
Cc: Leon Romanovsky, Zhu Yanjun, Guoqing Jiang, haris.iqbal, jgg,
linux-rdma, linux-kernel
On 19/04/2023 21:20, Jinpu Wang wrote:
> On Wed, Apr 19, 2023 at 11:53 AM Zhijian Li (Fujitsu)
> <lizhijian@fujitsu.com> wrote:
>>
>> Leon, Guoqing
>>
>>
>> On 18/04/2023 15:57, Leon Romanovsky wrote:
>>>>>> Currently, without this patch:
>>>>>> 1. PD and clt_path->s.dev are shared among connections.
>>>>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>>>>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>>>>>> clt_path->s.dev become zero, it will destroy PD.
>>>>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>>>>> So please fix it by making sure that failure to create con[1] will
>>>>> release resources which were allocated. If con[1] didn't increase
>>>>> s.dev_ref, it shouldn't decrease it either.
>>>> You are right, the current patch did exactly that.
>>>> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
>>>> so that its cleanup phase will only decrease its s.dev properly.
>>> The has_dev is a workaround and not a solution. In proper error unwind
>>> sequence, you won't need extra flag.
>>>
>>> Thanks
>>>
>>
>> how about below changes
>>
>> commit 61dba725384e226d472b8142d70d40d4103df87a
>> Author: Li Zhijian <lizhijian@fujitsu.com>
>> Date: Wed Apr 19 17:42:26 2023 +0800
>>
>> RDMA/rtrs: Fix rxe_dealloc_pd warning
>>
>> con[0] always sets s.dev to 1, correspondingly, we should let it to
>> release the last dev.
>>
>> Previously,
>> 1. PD and clt_path->s.dev are shared among connections.
>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>> clt_path->s.dev become zero, it will destroy PD.
>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
>> but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>>
>> The warning occurs when destroying PD whose reference count is not zero.
>> Precodition: clt_path->s.con_num is 2.
>> So 2 cm connection will be created as below:
>> CPU0 CPU1
>> init_conns { |
>> create_cm() // a. con[0] created |
>> | a'. rtrs_clt_rdma_cm_handler() {
>> | rtrs_rdma_addr_resolved()
>> | create_con_cq_qp(con); << con[0]
>> | }
>> | in this moment, refcnt of PD was increased to 2+
>> |
>> create_cm() // b. cid = 1, failed |
>> destroy_con_cq_qp() |
>> rtrs_ib_dev_put() |
>> dev_free() |
>> ib_dealloc_pd(dev->ib_pd) << PD |
>> is destroyed, but refcnt is |
>> still greater than 0 |
>> }
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> index 80abf45a197a..1eb652dedca3 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>> @@ -1743,6 +1743,15 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
>> con->rsp_ius = NULL;
>> con->queue_num = 0;
>> }
>> +
>> + /*
>> + * Every con will try to decreased s.dev_ref, but we should
>> + * reserve the last s.dev_ref for con[0]. In case con[1+]'s
>> + * cleanup phase call rtrs_ib_dev_put(clt_path->s.dev) early.
>> + */
>> + if (con->c.cid != 0 && clt_path->s.dev_ref == 1)
>> + return;
>> +
>> if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
>> rtrs_ib_dev_put(clt_path->s.dev);
>> clt_path->s.dev = NULL;
>
Jinpu, thanks for your testing.
Indeed, above changes are not correct. it breaks the normal cleanup like below:
for (i=0; i < N, i++)
destroy_con_cq_qp(con[i])
Thanks
Zhijian
> I run a regression test in our test env, it triggers a warning on
>
> 1681 if (WARN_ON(clt_path->s.dev))
>
> [ 1333.042633] ------------[ cut here ]------------
> [ 1333.042650] WARNING: CPU: 8 PID: 559 at
> /root/kernel-test/ibnbd2/rtrs/rtrs-clt.c:1681
> rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
> [ 1333.042651] Modules linked in: loop rnbd_client(O) rtrs_client(O)
> rtrs_core(O) kvm_amd kvm input_leds led_class irqbypass crc32_pclmul
> aesni_intel sp5100_tco evdev libaes watchdog sg k10temp crypto_simd
> fam15h_power ipmi_si serio_raw cryptd ipmi_devintf glue_helper
> ipmi_msghandler acpi_cpufreq button ib_ipoib ib_umad null_blk brd
> rdma_cm iw_cm ib_cm ip_tables x_tables autofs4 raid10 raid456
> async_raid6_recov async_memcpy async_pq async_xor async_tx xor
> raid6_pq libcrc32c raid1 raid0 linear mlx4_ib md_mod ib_uverbs ib_core
> sd_mod t10_pi crc_t10dif crct10dif_generic ahci libahci
> crct10dif_pclmul crct10dif_common crc32c_intel igb libata usb_storage
> psmouse i2c_piix4 i2c_algo_bit mlx4_core dca scsi_mod i2c_core ptp
> pps_core
> [ 1333.042737] CPU: 8 PID: 559 Comm: kworker/u128:1 Tainted: G
> O 5.10.136-pserver-develop-5.10 #257
> [ 1333.042738] Hardware name: Supermicro H8QG6/H8QG6, BIOS 3.00 09/04/2012
> [ 1333.042752] Workqueue: rdma_cm cma_work_handler [rdma_cm]
> [ 1333.042758] RIP: 0010:rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
> [ 1333.042761] Code: ff bb ea ff ff ff e8 db a5 24 fc 49 8d b4 24 10
> 01 00 00 89 da 48 c7 c7 40 93 5b c0 e8 4b 47 21 fc 4d 8b 65 00 e9 15
> fe ff ff <0f> 0b 4c 89 ff bb ea ff ff ff e8 ad a5 24 fc eb d0 0f 0b 4c
> 89 ff
> [ 1333.042763] RSP: 0018:ffffaff68e57bdb0 EFLAGS: 00010286
> [ 1333.042765] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff9eddc0051420
> [ 1333.042767] RDX: ffff9ee4ef716e40 RSI: ffff9f14ea288f30 RDI: ffff9eddc88db240
> [ 1333.042768] RBP: ffffaff68e57be50 R08: 0000000000000000 R09: 006d635f616d6472
> [ 1333.042769] R10: ffffaff68e57be68 R11: 0000000000000000 R12: ffff9edde1388000
> [ 1333.042771] R13: ffff9eddc88db200 R14: ffff9edde1388000 R15: ffff9eddc88db240
> [ 1333.042773] FS: 0000000000000000(0000) GS:ffff9eecc7c00000(0000)
> knlGS:0000000000000000
> [ 1333.042774] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1333.042776] CR2: 00007f0ac4ed4004 CR3: 0000002b5040a000 CR4: 00000000000406e0
> [ 1333.042777] Call Trace:
> [ 1333.042790] ? newidle_balance+0x25e/0x3c0
> [ 1333.042795] ? psi_group_change+0x43/0x230
> [ 1333.042801] ? cma_cm_event_handler+0x23/0xb0 [rdma_cm]
> [ 1333.042807] cma_cm_event_handler+0x23/0xb0 [rdma_cm]
> [ 1333.042814] cma_work_handler+0x5a/0xb0 [rdma_cm]
> [ 1333.042819] process_one_work+0x1f3/0x390
> [ 1333.042822] worker_thread+0x2d/0x3c0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-20 2:00 ` Zhijian Li (Fujitsu)
@ 2023-04-21 1:38 ` Zhijian Li (Fujitsu)
2023-04-21 6:49 ` Zhijian Li (Fujitsu)
2023-04-21 7:05 ` Jinpu Wang
0 siblings, 2 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-21 1:38 UTC (permalink / raw)
To: Jinpu Wang
Cc: Leon Romanovsky, Zhu Yanjun, Guoqing Jiang, haris.iqbal, jgg,
linux-rdma, linux-kernel
Jinpu
I updated the changes as below, and tested for thousand rounds.
From d441c0e2496c1795b5af2b6b8ae4672203d6af3c Mon Sep 17 00:00:00 2001
From: Li Zhijian <lizhijian@fujitsu.com>
Date: Thu, 20 Apr 2023 17:28:28 +0800
Subject: [PATCH] RDMA/rtrs: Fix rxe_dealloc_pd warning
In current design:
1. PD and clt_path->s.dev are shared among connections.
2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and
when clt_path->s.dev become zero, it will destroy PD.
4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
but it try to decreased clt_path->s.dev
So, in case create_cm(con[0]) succeeds but create_cm(con[1])
fails, destroy_con_cq_qp(con[1]) will be called first which will destory
the PD while this PD is still taken by con[0].
Here, we refactor the error path of create_cm() and init_conns(), so that
we do the cleanup in the order they are created.
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
drivers/infiniband/ulp/rtrs/rtrs-clt.c | 47 +++++++++++---------------
1 file changed, 19 insertions(+), 28 deletions(-)
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 80abf45a197a..5faf0ecb726b 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2040,6 +2040,7 @@ static int rtrs_clt_rdma_cm_handler(struct rdma_cm_id *cm_id,
return 0;
}
+/* The caller should the do the cleanup in case of error */
static int create_cm(struct rtrs_clt_con *con)
{
struct rtrs_path *s = con->c.path;
@@ -2062,14 +2063,14 @@ static int create_cm(struct rtrs_clt_con *con)
err = rdma_set_reuseaddr(cm_id, 1);
if (err != 0) {
rtrs_err(s, "Set address reuse failed, err: %d\n", err);
- goto destroy_cm;
+ return err;
}
err = rdma_resolve_addr(cm_id, (struct sockaddr *)&clt_path->s.src_addr,
(struct sockaddr *)&clt_path->s.dst_addr,
RTRS_CONNECT_TIMEOUT_MS);
if (err) {
rtrs_err(s, "Failed to resolve address, err: %d\n", err);
- goto destroy_cm;
+ return err;
}
/*
* Combine connection status and session events. This is needed
@@ -2084,29 +2085,17 @@ static int create_cm(struct rtrs_clt_con *con)
if (err == 0)
err = -ETIMEDOUT;
/* Timedout or interrupted */
- goto errr;
+ return err;
}
if (con->cm_err < 0) {
- err = con->cm_err;
- goto errr;
+ return con->cm_err;
}
if (READ_ONCE(clt_path->state) != RTRS_CLT_CONNECTING) {
/* Device removal */
- err = -ECONNABORTED;
- goto errr;
+ return -ECONNABORTED;
}
return 0;
-
-errr:
- stop_cm(con);
- mutex_lock(&con->con_mutex);
- destroy_con_cq_qp(con);
- mutex_unlock(&con->con_mutex);
-destroy_cm:
- destroy_cm(con);
-
- return err;
}
static void rtrs_clt_path_up(struct rtrs_clt_path *clt_path)
@@ -2334,7 +2323,7 @@ static void rtrs_clt_close_work(struct work_struct *work)
static int init_conns(struct rtrs_clt_path *clt_path)
{
unsigned int cid;
- int err;
+ int err, i;
/*
* On every new session connections increase reconnect counter
@@ -2350,10 +2339,8 @@ static int init_conns(struct rtrs_clt_path *clt_path)
goto destroy;
err = create_cm(to_clt_con(clt_path->s.con[cid]));
- if (err) {
- destroy_con(to_clt_con(clt_path->s.con[cid]));
+ if (err)
goto destroy;
- }
}
err = alloc_path_reqs(clt_path);
if (err)
@@ -2364,15 +2351,19 @@ static int init_conns(struct rtrs_clt_path *clt_path)
return 0;
destroy:
- while (cid--) {
+ /* Make sure we do the cleanup in the order they are created */
+ for (i = 0; i <= cid; i++) {
struct rtrs_clt_con *con = to_clt_con(clt_path->s.con[cid]);
- stop_cm(con);
-
- mutex_lock(&con->con_mutex);
- destroy_con_cq_qp(con);
- mutex_unlock(&con->con_mutex);
- destroy_cm(con);
+ if (!con)
+ break;
+ if (con->c.cm_id) {
+ stop_cm(con);
+ mutex_lock(&con->con_mutex);
+ destroy_con_cq_qp(con);
+ mutex_unlock(&con->con_mutex);
+ destroy_cm(con);
+ }
destroy_con(con);
}
/*
--
2.29.2
On 20/04/2023 10:00, Li Zhijian wrote:
> On 19/04/2023 21:20, Jinpu Wang wrote:
>> On Wed, Apr 19, 2023 at 11:53 AM Zhijian Li (Fujitsu)
>> <lizhijian@fujitsu.com> wrote:
>>>
>>> Leon, Guoqing
>>>
>>>
>>> On 18/04/2023 15:57, Leon Romanovsky wrote:
>>>>>>> Currently, without this patch:
>>>>>>> 1. PD and clt_path->s.dev are shared among connections.
>>>>>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>>>>>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>>>>>>> clt_path->s.dev become zero, it will destroy PD.
>>>>>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>>>>>> So please fix it by making sure that failure to create con[1] will
>>>>>> release resources which were allocated. If con[1] didn't increase
>>>>>> s.dev_ref, it shouldn't decrease it either.
>>>>> You are right, the current patch did exactly that.
>>>>> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
>>>>> so that its cleanup phase will only decrease its s.dev properly.
>>>> The has_dev is a workaround and not a solution. In proper error unwind
>>>> sequence, you won't need extra flag.
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-21 1:38 ` Zhijian Li (Fujitsu)
@ 2023-04-21 6:49 ` Zhijian Li (Fujitsu)
2023-04-21 7:05 ` Jinpu Wang
1 sibling, 0 replies; 38+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-04-21 6:49 UTC (permalink / raw)
To: Jinpu Wang
Cc: Leon Romanovsky, Zhu Yanjun, Guoqing Jiang, haris.iqbal, jgg,
linux-rdma, linux-kernel
On 21/04/2023 09:38, Li Zhijian wrote:
> Jinpu
>
> I updated the changes as below, and tested for thousand rounds.
>
> From d441c0e2496c1795b5af2b6b8ae4672203d6af3c Mon Sep 17 00:00:00 2001
> From: Li Zhijian <lizhijian@fujitsu.com>
> Date: Thu, 20 Apr 2023 17:28:28 +0800
> Subject: [PATCH] RDMA/rtrs: Fix rxe_dealloc_pd warning
>
> In current design:
> 1. PD and clt_path->s.dev are shared among connections.
> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and
> when clt_path->s.dev become zero, it will destroy PD.
> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
> but it try to decreased clt_path->s.dev
>
> So, in case create_cm(con[0]) succeeds but create_cm(con[1])
> fails, destroy_con_cq_qp(con[1]) will be called first which will destory
> the PD while this PD is still taken by con[0].
>
> Here, we refactor the error path of create_cm() and init_conns(), so that
> we do the cleanup in the order they are created.
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 47 +++++++++++---------------
> 1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 80abf45a197a..5faf0ecb726b 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -2040,6 +2040,7 @@ static int rtrs_clt_rdma_cm_handler(struct rdma_cm_id *cm_id,
> return 0;
> }
>
> +/* The caller should the do the cleanup in case of error */
> static int create_cm(struct rtrs_clt_con *con)
> {
> struct rtrs_path *s = con->c.path;
> @@ -2062,14 +2063,14 @@ static int create_cm(struct rtrs_clt_con *con)
> err = rdma_set_reuseaddr(cm_id, 1);
> if (err != 0) {
> rtrs_err(s, "Set address reuse failed, err: %d\n", err);
> - goto destroy_cm;
> + return err;
> }
> err = rdma_resolve_addr(cm_id, (struct sockaddr *)&clt_path->s.src_addr,
> (struct sockaddr *)&clt_path->s.dst_addr,
> RTRS_CONNECT_TIMEOUT_MS);
> if (err) {
> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
> - goto destroy_cm;
> + return err;
> }
> /*
> * Combine connection status and session events. This is needed
> @@ -2084,29 +2085,17 @@ static int create_cm(struct rtrs_clt_con *con)
> if (err == 0)
> err = -ETIMEDOUT;
> /* Timedout or interrupted */
> - goto errr;
> + return err;
> }
> if (con->cm_err < 0) {
> - err = con->cm_err;
> - goto errr;
> + return con->cm_err;
> }
> if (READ_ONCE(clt_path->state) != RTRS_CLT_CONNECTING) {
> /* Device removal */
> - err = -ECONNABORTED;
> - goto errr;
> + return -ECONNABORTED;
> }
>
> return 0;
> -
> -errr:
> - stop_cm(con);
> - mutex_lock(&con->con_mutex);
> - destroy_con_cq_qp(con);
> - mutex_unlock(&con->con_mutex);
> -destroy_cm:
> - destroy_cm(con);
> -
> - return err;
> }
>
> static void rtrs_clt_path_up(struct rtrs_clt_path *clt_path)
> @@ -2334,7 +2323,7 @@ static void rtrs_clt_close_work(struct work_struct *work)
> static int init_conns(struct rtrs_clt_path *clt_path)
> {
> unsigned int cid;
> - int err;
> + int err, i;
>
> /*
> * On every new session connections increase reconnect counter
> @@ -2350,10 +2339,8 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> goto destroy;
>
> err = create_cm(to_clt_con(clt_path->s.con[cid]));
> - if (err) {
> - destroy_con(to_clt_con(clt_path->s.con[cid]));
> + if (err)
> goto destroy;
> - }
> }
> err = alloc_path_reqs(clt_path);
> if (err)
> @@ -2364,15 +2351,19 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> return 0;
>
> destroy:
> - while (cid--) {
> + /* Make sure we do the cleanup in the order they are created */
> + for (i = 0; i <= cid; i++) {
> struct rtrs_clt_con *con = to_clt_con(clt_path->s.con[cid]);
s/cid/i
>
> - stop_cm(con);
> -
> - mutex_lock(&con->con_mutex);
> - destroy_con_cq_qp(con);
> - mutex_unlock(&con->con_mutex);
> - destroy_cm(con);
> + if (!con)
> + break;
> + if (con->c.cm_id) {
> + stop_cm(con);
> + mutex_lock(&con->con_mutex);
> + destroy_con_cq_qp(con);
> + mutex_unlock(&con->con_mutex);
> + destroy_cm(con);
> + }
> destroy_con(con);
> }
> /*
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning
2023-04-21 1:38 ` Zhijian Li (Fujitsu)
2023-04-21 6:49 ` Zhijian Li (Fujitsu)
@ 2023-04-21 7:05 ` Jinpu Wang
1 sibling, 0 replies; 38+ messages in thread
From: Jinpu Wang @ 2023-04-21 7:05 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Leon Romanovsky, Zhu Yanjun, Guoqing Jiang, haris.iqbal, jgg,
linux-rdma, linux-kernel
On Fri, Apr 21, 2023 at 3:38 AM Zhijian Li (Fujitsu)
<lizhijian@fujitsu.com> wrote:
>
> Jinpu
>
> I updated the changes as below, and tested for thousand rounds.
>
> From d441c0e2496c1795b5af2b6b8ae4672203d6af3c Mon Sep 17 00:00:00 2001
> From: Li Zhijian <lizhijian@fujitsu.com>
> Date: Thu, 20 Apr 2023 17:28:28 +0800
> Subject: [PATCH] RDMA/rtrs: Fix rxe_dealloc_pd warning
>
> In current design:
> 1. PD and clt_path->s.dev are shared among connections.
> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and
> when clt_path->s.dev become zero, it will destroy PD.
> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
> but it try to decreased clt_path->s.dev
>
> So, in case create_cm(con[0]) succeeds but create_cm(con[1])
> fails, destroy_con_cq_qp(con[1]) will be called first which will destory
> the PD while this PD is still taken by con[0].
>
> Here, we refactor the error path of create_cm() and init_conns(), so that
> we do the cleanup in the order they are created.
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> drivers/infiniband/ulp/rtrs/rtrs-clt.c | 47 +++++++++++---------------
> 1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 80abf45a197a..5faf0ecb726b 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -2040,6 +2040,7 @@ static int rtrs_clt_rdma_cm_handler(struct rdma_cm_id *cm_id,
> return 0;
> }
>
> +/* The caller should the do the cleanup in case of error */
> static int create_cm(struct rtrs_clt_con *con)
> {
> struct rtrs_path *s = con->c.path;
> @@ -2062,14 +2063,14 @@ static int create_cm(struct rtrs_clt_con *con)
> err = rdma_set_reuseaddr(cm_id, 1);
> if (err != 0) {
> rtrs_err(s, "Set address reuse failed, err: %d\n", err);
> - goto destroy_cm;
> + return err;
> }
> err = rdma_resolve_addr(cm_id, (struct sockaddr *)&clt_path->s.src_addr,
> (struct sockaddr *)&clt_path->s.dst_addr,
> RTRS_CONNECT_TIMEOUT_MS);
> if (err) {
> rtrs_err(s, "Failed to resolve address, err: %d\n", err);
> - goto destroy_cm;
> + return err;
> }
> /*
> * Combine connection status and session events. This is needed
> @@ -2084,29 +2085,17 @@ static int create_cm(struct rtrs_clt_con *con)
> if (err == 0)
> err = -ETIMEDOUT;
> /* Timedout or interrupted */
> - goto errr;
> + return err;
> }
> if (con->cm_err < 0) {
> - err = con->cm_err;
> - goto errr;
> + return con->cm_err;
> }
The bracket can be removed too
> if (READ_ONCE(clt_path->state) != RTRS_CLT_CONNECTING) {
> /* Device removal */
> - err = -ECONNABORTED;
> - goto errr;
> + return -ECONNABORTED;
> }
same here.
>
> return 0;
> -
> -errr:
> - stop_cm(con);
> - mutex_lock(&con->con_mutex);
> - destroy_con_cq_qp(con);
> - mutex_unlock(&con->con_mutex);
> -destroy_cm:
> - destroy_cm(con);
> -
> - return err;
> }
>
> static void rtrs_clt_path_up(struct rtrs_clt_path *clt_path)
> @@ -2334,7 +2323,7 @@ static void rtrs_clt_close_work(struct work_struct *work)
> static int init_conns(struct rtrs_clt_path *clt_path)
> {
> unsigned int cid;
> - int err;
> + int err, i;
>
> /*
> * On every new session connections increase reconnect counter
> @@ -2350,10 +2339,8 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> goto destroy;
>
> err = create_cm(to_clt_con(clt_path->s.con[cid]));
> - if (err) {
> - destroy_con(to_clt_con(clt_path->s.con[cid]));
> + if (err)
> goto destroy;
> - }
> }
> err = alloc_path_reqs(clt_path);
> if (err)
> @@ -2364,15 +2351,19 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> return 0;
>
> destroy:
> - while (cid--) {
> + /* Make sure we do the cleanup in the order they are created */
> + for (i = 0; i <= cid; i++) {
> struct rtrs_clt_con *con = to_clt_con(clt_path->s.con[cid]);
Yes, this line has to be adapted.
struct rtrs_clt_con *con = to_clt_con(clt_path->s.con[i]);
>
> - stop_cm(con);
> -
> - mutex_lock(&con->con_mutex);
> - destroy_con_cq_qp(con);
> - mutex_unlock(&con->con_mutex);
> - destroy_cm(con);
> + if (!con)
> + break;
> + if (con->c.cm_id) {
> + stop_cm(con);
> + mutex_lock(&con->con_mutex);
> + destroy_con_cq_qp(con);
> + mutex_unlock(&con->con_mutex);
> + destroy_cm(con);
> + }
> destroy_con(con);
> }
> /*
> --
> 2.29.2
>
This version looks fine. I will run some tests.
Thx!
>
>
> On 20/04/2023 10:00, Li Zhijian wrote:
> > On 19/04/2023 21:20, Jinpu Wang wrote:
> >> On Wed, Apr 19, 2023 at 11:53 AM Zhijian Li (Fujitsu)
> >> <lizhijian@fujitsu.com> wrote:
> >>>
> >>> Leon, Guoqing
> >>>
> >>>
> >>> On 18/04/2023 15:57, Leon Romanovsky wrote:
> >>>>>>> Currently, without this patch:
> >>>>>>> 1. PD and clt_path->s.dev are shared among connections.
> >>>>>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
> >>>>>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
> >>>>>>> clt_path->s.dev become zero, it will destroy PD.
> >>>>>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
> >>>>>> So please fix it by making sure that failure to create con[1] will
> >>>>>> release resources which were allocated. If con[1] didn't increase
> >>>>>> s.dev_ref, it shouldn't decrease it either.
> >>>>> You are right, the current patch did exactly that.
> >>>>> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
> >>>>> so that its cleanup phase will only decrease its s.dev properly.
> >>>> The has_dev is a workaround and not a solution. In proper error unwind
> >>>> sequence, you won't need extra flag.
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2023-04-21 7:06 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-10 6:43 [PATCH for-next 0/3] rtrs bugfix and cleanups Li Zhijian
2023-04-10 6:43 ` [PATCH for-next 1/3] RDMA/rtrs: Remove duplicate cq_num assignment Li Zhijian
2023-04-10 13:09 ` Guoqing Jiang
2023-04-19 10:37 ` Jinpu Wang
2023-04-10 6:43 ` [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning Li Zhijian
2023-04-10 12:08 ` Leon Romanovsky
2023-04-10 13:10 ` Guoqing Jiang
2023-04-11 2:43 ` Zhijian Li (Fujitsu)
2023-04-11 12:26 ` Leon Romanovsky
2023-04-12 1:15 ` Zhijian Li (Fujitsu)
2023-04-13 7:35 ` Guoqing Jiang
2023-04-13 8:12 ` Zhijian Li (Fujitsu)
2023-04-13 13:24 ` Leon Romanovsky
2023-04-14 15:58 ` Zhu Yanjun
2023-04-17 2:18 ` Zhijian Li (Fujitsu)
2023-04-17 18:04 ` Leon Romanovsky
2023-04-18 7:04 ` Zhijian Li (Fujitsu)
2023-04-18 7:57 ` Leon Romanovsky
2023-04-19 9:53 ` Zhijian Li (Fujitsu)
2023-04-19 13:20 ` Jinpu Wang
2023-04-20 2:00 ` Zhijian Li (Fujitsu)
2023-04-21 1:38 ` Zhijian Li (Fujitsu)
2023-04-21 6:49 ` Zhijian Li (Fujitsu)
2023-04-21 7:05 ` Jinpu Wang
2023-04-14 3:40 ` Guoqing Jiang
2023-04-14 4:25 ` Bob Pearson
2023-04-14 5:37 ` Zhijian Li (Fujitsu)
2023-04-14 6:03 ` Jinpu Wang
2023-04-14 6:47 ` Zhijian Li (Fujitsu)
2023-04-14 6:04 ` Guoqing Jiang
2023-04-14 10:09 ` Zhijian Li (Fujitsu)
2023-04-17 3:08 ` Guoqing Jiang
2023-04-18 6:47 ` Zhijian Li (Fujitsu)
2023-04-10 6:43 ` [PATCH for-next 3/3] RDMA/rtrs: Avoid use-after-free in rtrs_clt_rdma_cm_handler Li Zhijian
2023-04-10 12:10 ` Leon Romanovsky
2023-04-10 13:13 ` Guoqing Jiang
2023-04-11 1:33 ` Zhijian Li (Fujitsu)
2023-04-12 1:15 ` Guoqing Jiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).