* [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two()
@ 2020-12-07 9:37 Kamal Heib
2020-12-07 20:27 ` Jason Gunthorpe
0 siblings, 1 reply; 5+ messages in thread
From: Kamal Heib @ 2020-12-07 9:37 UTC (permalink / raw)
To: linux-rdma; +Cc: Bernard Metzler, Doug Ledford, Jason Gunthorpe, Kamal Heib
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@gmail.com>
---
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
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ 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(
cep,
"[QP %u]: ord %d (max %d), ird %d (max %d)\n",
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two()
2020-12-07 9:37 [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two() Kamal Heib
@ 2020-12-07 20:27 ` Jason Gunthorpe
2020-12-08 10:46 ` Bernard Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2020-12-07 20:27 UTC (permalink / raw)
To: Kamal Heib; +Cc: linux-rdma, Bernard Metzler, Doug Ledford
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@gmail.com>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two()
2020-12-07 20:27 ` Jason Gunthorpe
@ 2020-12-08 10:46 ` Bernard Metzler
2020-12-08 15:16 ` Tom Talpey
0 siblings, 1 reply; 5+ messages in thread
From: Bernard Metzler @ 2020-12-08 10:46 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Kamal Heib, linux-rdma, Doug Ledford
-----"Jason Gunthorpe" <jgg@nvidia.com> wrote: -----
>To: "Kamal Heib" <kamalheib1@gmail.com>
>From: "Jason Gunthorpe" <jgg@nvidia.com>
>Date: 12/07/2020 09:29PM
>Cc: <linux-rdma@vger.kernel.org>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Doug Ledford" <dledford@redhat.com>
>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@gmail.com>
>> 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?
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?
Thanks!
Bernard.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two()
2020-12-08 10:46 ` Bernard Metzler
@ 2020-12-08 15:16 ` Tom Talpey
2020-12-08 16:16 ` Bernard Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Tom Talpey @ 2020-12-08 15:16 UTC (permalink / raw)
To: Bernard Metzler, Jason Gunthorpe; +Cc: Kamal Heib, linux-rdma, Doug Ledford
On 12/8/2020 5:46 AM, Bernard Metzler wrote:
> -----"Jason Gunthorpe" <jgg@nvidia.com> wrote: -----
>
>> To: "Kamal Heib" <kamalheib1@gmail.com>
>> From: "Jason Gunthorpe" <jgg@nvidia.com>
>> Date: 12/07/2020 09:29PM
>> Cc: <linux-rdma@vger.kernel.org>, "Bernard Metzler"
>> <bmt@zurich.ibm.com>, "Doug Ledford" <dledford@redhat.com>
>> 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@gmail.com>
>>> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two()
2020-12-08 15:16 ` Tom Talpey
@ 2020-12-08 16:16 ` Bernard Metzler
0 siblings, 0 replies; 5+ messages in thread
From: Bernard Metzler @ 2020-12-08 16:16 UTC (permalink / raw)
To: Tom Talpey; +Cc: Jason Gunthorpe, Kamal Heib, linux-rdma, Doug Ledford
-----"Tom Talpey" <tom@talpey.com> wrote: -----
>To: "Bernard Metzler" <BMT@zurich.ibm.com>, "Jason Gunthorpe"
><jgg@nvidia.com>
>From: "Tom Talpey" <tom@talpey.com>
>Date: 12/08/2020 04:19PM
>Cc: "Kamal Heib" <kamalheib1@gmail.com>, linux-rdma@vger.kernel.org,
>"Doug Ledford" <dledford@redhat.com>
>Subject: [EXTERNAL] Re: [PATCH for-rc] RDMA/siw: Fix
>shift-out-of-bounds when call roundup_pow_of_two()
>
>On 12/8/2020 5:46 AM, Bernard Metzler wrote:
>> -----"Jason Gunthorpe" <jgg@nvidia.com> wrote: -----
>>
>>> To: "Kamal Heib" <kamalheib1@gmail.com>
>>> From: "Jason Gunthorpe" <jgg@nvidia.com>
>>> Date: 12/07/2020 09:29PM
>>> Cc: <linux-rdma@vger.kernel.org>, "Bernard Metzler"
>>> <bmt@zurich.ibm.com>, "Doug Ledford" <dledford@redhat.com>
>>> 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@gmail.com>
>>>> 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.
>
Yes, that's what I meant. I think so too, this should be allowed.
One side might for example want to forbid the peer to send READ
requests, since it does not want to expose any buffer for remote
reading, so it want to set IRD to zero.
I don't know what other providers are doing, but let me fix that
for siw.
>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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-08 16:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-07 9:37 [PATCH for-rc] RDMA/siw: Fix shift-out-of-bounds when call roundup_pow_of_two() Kamal Heib
2020-12-07 20:27 ` Jason Gunthorpe
2020-12-08 10:46 ` Bernard Metzler
2020-12-08 15:16 ` Tom Talpey
2020-12-08 16:16 ` Bernard Metzler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox