On 12/8/2020 5:46 AM, Bernard Metzler wrote:
-----"Jason Gunthorpe" <jgg@xxxxxxxxxx> wrote: -----
To: "Kamal Heib" <kamalheib1@xxxxxxxxx>
From: "Jason Gunthorpe" <jgg@xxxxxxxxxx>
Date: 12/07/2020 09:29PM
Cc: <linux-rdma@xxxxxxxxxxxxxxx>, "Bernard Metzler"
<bmt@xxxxxxxxxxxxxx>, "Doug Ledford" <dledford@xxxxxxxxxx>
Subject: [EXTERNAL] Re: [PATCH for-rc] RDMA/siw: Fix
shift-out-of-bounds when call roundup_pow_of_two()
On Mon, Dec 07, 2020 at 11:37:28AM +0200, Kamal Heib wrote:
When running the blktests over siw the following
shift-out-of-bounds is
reported, this is happening because the passed IRD or ORD from the
ulp
could be zero which will lead to unexpected behavior when calling
roundup_pow_of_two(), fix that by blocking zero values of ORD or
IRD.
UBSAN: shift-out-of-bounds in ./include/linux/log2.h:57:13
shift exponent 64 is too large for 64-bit type 'long unsigned int'
CPU: 20 PID: 3957 Comm: kworker/u64:13 Tainted: G S 5.10.0-rc6
#2
Hardware name: Dell Inc. PowerEdge R630/02C2CP, BIOS 2.1.5
04/11/2016
Workqueue: iw_cm_wq cm_work_handler [iw_cm]
Call Trace:
dump_stack+0x99/0xcb
ubsan_epilogue+0x5/0x40
__ubsan_handle_shift_out_of_bounds.cold.11+0xb4/0xf3
? down_write+0x183/0x3d0
siw_qp_modify.cold.8+0x2d/0x32 [siw]
? __local_bh_enable_ip+0xa5/0xf0
siw_accept+0x906/0x1b60 [siw]
? xa_load+0x147/0x1f0
? siw_connect+0x17a0/0x17a0 [siw]
? lock_downgrade+0x700/0x700
? siw_get_base_qp+0x1c2/0x340 [siw]
? _raw_spin_unlock_irqrestore+0x39/0x40
iw_cm_accept+0x1f4/0x430 [iw_cm]
rdma_accept+0x3fa/0xb10 [rdma_cm]
? check_flush_dependency+0x410/0x410
? cma_rep_recv+0x570/0x570 [rdma_cm]
nvmet_rdma_queue_connect+0x1a62/0x2680 [nvmet_rdma]
? nvmet_rdma_alloc_cmds+0xce0/0xce0 [nvmet_rdma]
? lock_release+0x56e/0xcc0
? lock_downgrade+0x700/0x700
? lock_downgrade+0x700/0x700
? __xa_alloc_cyclic+0xef/0x350
? __xa_alloc+0x2d0/0x2d0
? rdma_restrack_add+0xbe/0x2c0 [ib_core]
? __ww_mutex_die+0x190/0x190
cma_cm_event_handler+0xf2/0x500 [rdma_cm]
iw_conn_req_handler+0x910/0xcb0 [rdma_cm]
? _raw_spin_unlock_irqrestore+0x39/0x40
? trace_hardirqs_on+0x1c/0x150
? cma_ib_handler+0x8a0/0x8a0 [rdma_cm]
? __kasan_kmalloc.constprop.7+0xc1/0xd0
cm_work_handler+0x121c/0x17a0 [iw_cm]
? iw_cm_reject+0x190/0x190 [iw_cm]
? trace_hardirqs_on+0x1c/0x150
process_one_work+0x8fb/0x16c0
? pwq_dec_nr_in_flight+0x320/0x320
worker_thread+0x87/0xb40
? __kthread_parkme+0xd1/0x1a0
? process_one_work+0x16c0/0x16c0
kthread+0x35f/0x430
? kthread_mod_delayed_work+0x180/0x180
ret_from_fork+0x22/0x30
Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
drivers/infiniband/sw/siw/siw_cm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/sw/siw/siw_cm.c
b/drivers/infiniband/sw/siw/siw_cm.c
index 66764f7ef072..dff0b00cc55d 100644
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1571,7 +1571,8 @@ int siw_accept(struct iw_cm_id *id, struct
iw_cm_conn_param *params)
qp->tx_ctx.gso_seg_limit = 0;
}
if (params->ord > sdev->attrs.max_ord ||
- params->ird > sdev->attrs.max_ird) {
+ params->ird > sdev->attrs.max_ird ||
+ !params->ord || !params->ird) {
siw_dbg_cep(
Are you sure this is the right place for this? Why not higher up? It
looks like the other iwarp drivers have the same problem
Jason
1) Good question. Do we want to allow applications to zero-size
rdma READ capabilities? Maybe we want, if it is recognized as a
security feature?
Do you mean zero-size RDMA Read, as in, an RDMA Read of zero bytes?
This is a valid operation specifically mentioned in the protocols.
Although it transfers no data, it does require a region protection
check at the responder, and it's something that requesting applications
may issue.
OTOH, if you mean is a zero IRD or ORD valid, yes, that too is true.
The NFS/RDMA client actually did this, because the rpcrdma protocol
permits only the server to issue RDMA operations. Therefore to reduce
resources, the client would set IRD to zero, and ORD to some small
number. The server would do the opposite (IRD=n and ORD=0)
2) In any case, siw currently does not correctly handle the case
of zero sized ORD/IRD. If we want to go with 1), some fixes to siw
are to be done. If we do not want 1), Kamal's patch is half of the
story. It handles the response side only. Initiator would have to
be fixed as well.
I'd propose allowing 1). I'd fix siw accordingly. Opinions?
Definitely allow both aspects of #1, and fix #2.
Tom.
