* [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
* 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 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
* [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
* 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 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 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 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 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-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 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 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
* 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 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 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: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-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 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
* [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 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 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 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
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).