| Message ID | 20200217205714.26937-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | fb3063d31995cc4cf1d47a406bb61d6fb1b1d58d |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Fix configuration of atomic queue pair attributes | expand |
Sorry, when max_rd_atomic will be set to 0? Thanks, Zhu Yanjun On Tue, Feb 18, 2020 at 4:59 AM Bart Van Assche <bvanassche@acm.org> wrote: > > From the comment above the definition of the roundup_pow_of_two() macro: > > The result is undefined when n == 0. > > Hence only pass positive values to roundup_pow_of_two(). This patch fixes > the following UBSAN complaint: > > UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13 > shift exponent 64 is too large for 64-bit type 'long unsigned int' > Call Trace: > dump_stack+0xa5/0xe6 > ubsan_epilogue+0x9/0x26 > __ubsan_handle_shift_out_of_bounds.cold+0x4c/0xf9 > rxe_qp_from_attr.cold+0x37/0x5d [rdma_rxe] > rxe_modify_qp+0x59/0x70 [rdma_rxe] > _ib_modify_qp+0x5aa/0x7c0 [ib_core] > ib_modify_qp+0x3b/0x50 [ib_core] > cma_modify_qp_rtr+0x234/0x260 [rdma_cm] > __rdma_accept+0x1a7/0x650 [rdma_cm] > nvmet_rdma_cm_handler+0x1286/0x14cd [nvmet_rdma] > cma_cm_event_handler+0x6b/0x330 [rdma_cm] > cma_ib_req_handler+0xe60/0x22d0 [rdma_cm] > cm_process_work+0x30/0x140 [ib_cm] > cm_req_handler+0x11f4/0x1cd0 [ib_cm] > cm_work_handler+0xb8/0x344e [ib_cm] > process_one_work+0x569/0xb60 > worker_thread+0x7a/0x5d0 > kthread+0x1e6/0x210 > ret_from_fork+0x24/0x30 > > Cc: Moni Shoua <monis@mellanox.com> > Fixes: 8700e3e7c485 ("Soft RoCE driver") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/sw/rxe/rxe_qp.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c > index ec21f616ac98..6c11c3aeeca6 100644 > --- a/drivers/infiniband/sw/rxe/rxe_qp.c > +++ b/drivers/infiniband/sw/rxe/rxe_qp.c > @@ -590,15 +590,16 @@ int rxe_qp_from_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask, > int err; > > if (mask & IB_QP_MAX_QP_RD_ATOMIC) { > - int max_rd_atomic = __roundup_pow_of_two(attr->max_rd_atomic); > + int max_rd_atomic = attr->max_rd_atomic ? > + roundup_pow_of_two(attr->max_rd_atomic) : 0; > > qp->attr.max_rd_atomic = max_rd_atomic; > atomic_set(&qp->req.rd_atomic, max_rd_atomic); > } > > if (mask & IB_QP_MAX_DEST_RD_ATOMIC) { > - int max_dest_rd_atomic = > - __roundup_pow_of_two(attr->max_dest_rd_atomic); > + int max_dest_rd_atomic = attr->max_dest_rd_atomic ? > + roundup_pow_of_two(attr->max_dest_rd_atomic) : 0; > > qp->attr.max_dest_rd_atomic = max_dest_rd_atomic; >
On Tue, Feb 18, 2020 at 05:53:56PM +0800, Zhu Yanjun wrote: > Sorry, when max_rd_atomic will be set to 0? User can set it. ib_uverbs_ex_modify_qp() -> modify_qp() -> ib_modify_qp_with_udata() -> _ib_modify_qp() -> ib_security_modify_qp() -> .modify_q() -> rxe_modify_qp() -> rxe_qp_from_attr() > > Thanks, > Zhu Yanjun > > On Tue, Feb 18, 2020 at 4:59 AM Bart Van Assche <bvanassche@acm.org> wrote: > > > > From the comment above the definition of the roundup_pow_of_two() macro: > > > > The result is undefined when n == 0. > > > > Hence only pass positive values to roundup_pow_of_two(). This patch fixes > > the following UBSAN complaint: > > > > UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13 > > shift exponent 64 is too large for 64-bit type 'long unsigned int' > > Call Trace: > > dump_stack+0xa5/0xe6 > > ubsan_epilogue+0x9/0x26 > > __ubsan_handle_shift_out_of_bounds.cold+0x4c/0xf9 > > rxe_qp_from_attr.cold+0x37/0x5d [rdma_rxe] > > rxe_modify_qp+0x59/0x70 [rdma_rxe] > > _ib_modify_qp+0x5aa/0x7c0 [ib_core] > > ib_modify_qp+0x3b/0x50 [ib_core] > > cma_modify_qp_rtr+0x234/0x260 [rdma_cm] > > __rdma_accept+0x1a7/0x650 [rdma_cm] > > nvmet_rdma_cm_handler+0x1286/0x14cd [nvmet_rdma] > > cma_cm_event_handler+0x6b/0x330 [rdma_cm] > > cma_ib_req_handler+0xe60/0x22d0 [rdma_cm] > > cm_process_work+0x30/0x140 [ib_cm] > > cm_req_handler+0x11f4/0x1cd0 [ib_cm] > > cm_work_handler+0xb8/0x344e [ib_cm] > > process_one_work+0x569/0xb60 > > worker_thread+0x7a/0x5d0 > > kthread+0x1e6/0x210 > > ret_from_fork+0x24/0x30 > > > > Cc: Moni Shoua <monis@mellanox.com> > > Fixes: 8700e3e7c485 ("Soft RoCE driver") > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > --- > > drivers/infiniband/sw/rxe/rxe_qp.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c > > index ec21f616ac98..6c11c3aeeca6 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_qp.c > > +++ b/drivers/infiniband/sw/rxe/rxe_qp.c > > @@ -590,15 +590,16 @@ int rxe_qp_from_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask, > > int err; > > > > if (mask & IB_QP_MAX_QP_RD_ATOMIC) { > > - int max_rd_atomic = __roundup_pow_of_two(attr->max_rd_atomic); > > + int max_rd_atomic = attr->max_rd_atomic ? > > + roundup_pow_of_two(attr->max_rd_atomic) : 0; > > > > qp->attr.max_rd_atomic = max_rd_atomic; > > atomic_set(&qp->req.rd_atomic, max_rd_atomic); > > } > > > > if (mask & IB_QP_MAX_DEST_RD_ATOMIC) { > > - int max_dest_rd_atomic = > > - __roundup_pow_of_two(attr->max_dest_rd_atomic); > > + int max_dest_rd_atomic = attr->max_dest_rd_atomic ? > > + roundup_pow_of_two(attr->max_dest_rd_atomic) : 0; > > > > qp->attr.max_dest_rd_atomic = max_dest_rd_atomic; > >
Thanks. I am fine with it. On Tue, Feb 18, 2020 at 9:09 PM Leon Romanovsky <leonro@mellanox.com> wrote: > > On Tue, Feb 18, 2020 at 05:53:56PM +0800, Zhu Yanjun wrote: > > Sorry, when max_rd_atomic will be set to 0? > > User can set it. > ib_uverbs_ex_modify_qp() > -> modify_qp() > -> ib_modify_qp_with_udata() > -> _ib_modify_qp() > -> ib_security_modify_qp() > -> .modify_q() > -> rxe_modify_qp() > -> rxe_qp_from_attr() > > > > > Thanks, > > Zhu Yanjun > > > > On Tue, Feb 18, 2020 at 4:59 AM Bart Van Assche <bvanassche@acm.org> wrote: > > > > > > From the comment above the definition of the roundup_pow_of_two() macro: > > > > > > The result is undefined when n == 0. > > > > > > Hence only pass positive values to roundup_pow_of_two(). This patch fixes > > > the following UBSAN complaint: > > > > > > UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13 > > > shift exponent 64 is too large for 64-bit type 'long unsigned int' > > > Call Trace: > > > dump_stack+0xa5/0xe6 > > > ubsan_epilogue+0x9/0x26 > > > __ubsan_handle_shift_out_of_bounds.cold+0x4c/0xf9 > > > rxe_qp_from_attr.cold+0x37/0x5d [rdma_rxe] > > > rxe_modify_qp+0x59/0x70 [rdma_rxe] > > > _ib_modify_qp+0x5aa/0x7c0 [ib_core] > > > ib_modify_qp+0x3b/0x50 [ib_core] > > > cma_modify_qp_rtr+0x234/0x260 [rdma_cm] > > > __rdma_accept+0x1a7/0x650 [rdma_cm] > > > nvmet_rdma_cm_handler+0x1286/0x14cd [nvmet_rdma] > > > cma_cm_event_handler+0x6b/0x330 [rdma_cm] > > > cma_ib_req_handler+0xe60/0x22d0 [rdma_cm] > > > cm_process_work+0x30/0x140 [ib_cm] > > > cm_req_handler+0x11f4/0x1cd0 [ib_cm] > > > cm_work_handler+0xb8/0x344e [ib_cm] > > > process_one_work+0x569/0xb60 > > > worker_thread+0x7a/0x5d0 > > > kthread+0x1e6/0x210 > > > ret_from_fork+0x24/0x30 > > > > > > Cc: Moni Shoua <monis@mellanox.com> > > > Fixes: 8700e3e7c485 ("Soft RoCE driver") > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > > --- > > > drivers/infiniband/sw/rxe/rxe_qp.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c > > > index ec21f616ac98..6c11c3aeeca6 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_qp.c > > > +++ b/drivers/infiniband/sw/rxe/rxe_qp.c > > > @@ -590,15 +590,16 @@ int rxe_qp_from_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask, > > > int err; > > > > > > if (mask & IB_QP_MAX_QP_RD_ATOMIC) { > > > - int max_rd_atomic = __roundup_pow_of_two(attr->max_rd_atomic); > > > + int max_rd_atomic = attr->max_rd_atomic ? > > > + roundup_pow_of_two(attr->max_rd_atomic) : 0; > > > > > > qp->attr.max_rd_atomic = max_rd_atomic; > > > atomic_set(&qp->req.rd_atomic, max_rd_atomic); > > > } > > > > > > if (mask & IB_QP_MAX_DEST_RD_ATOMIC) { > > > - int max_dest_rd_atomic = > > > - __roundup_pow_of_two(attr->max_dest_rd_atomic); > > > + int max_dest_rd_atomic = attr->max_dest_rd_atomic ? > > > + roundup_pow_of_two(attr->max_dest_rd_atomic) : 0; > > > > > > qp->attr.max_dest_rd_atomic = max_dest_rd_atomic; > > >
On Wed, Feb 19, 2020 at 08:50:09AM +0800, Zhu Yanjun wrote: > Thanks. I am fine with it. > I'm fine too. Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Mon, Feb 17, 2020 at 12:57:14PM -0800, Bart Van Assche wrote: > >From the comment above the definition of the roundup_pow_of_two() macro: > > The result is undefined when n == 0. > > Hence only pass positive values to roundup_pow_of_two(). This patch fixes > the following UBSAN complaint: > > UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13 > shift exponent 64 is too large for 64-bit type 'long unsigned int' > Call Trace: > dump_stack+0xa5/0xe6 > ubsan_epilogue+0x9/0x26 > __ubsan_handle_shift_out_of_bounds.cold+0x4c/0xf9 > rxe_qp_from_attr.cold+0x37/0x5d [rdma_rxe] > rxe_modify_qp+0x59/0x70 [rdma_rxe] > _ib_modify_qp+0x5aa/0x7c0 [ib_core] > ib_modify_qp+0x3b/0x50 [ib_core] > cma_modify_qp_rtr+0x234/0x260 [rdma_cm] > __rdma_accept+0x1a7/0x650 [rdma_cm] > nvmet_rdma_cm_handler+0x1286/0x14cd [nvmet_rdma] > cma_cm_event_handler+0x6b/0x330 [rdma_cm] > cma_ib_req_handler+0xe60/0x22d0 [rdma_cm] > cm_process_work+0x30/0x140 [ib_cm] > cm_req_handler+0x11f4/0x1cd0 [ib_cm] > cm_work_handler+0xb8/0x344e [ib_cm] > process_one_work+0x569/0xb60 > worker_thread+0x7a/0x5d0 > kthread+0x1e6/0x210 > ret_from_fork+0x24/0x30 > > Cc: Moni Shoua <monis@mellanox.com> > Fixes: 8700e3e7c485 ("Soft RoCE driver") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/sw/rxe/rxe_qp.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c index ec21f616ac98..6c11c3aeeca6 100644 --- a/drivers/infiniband/sw/rxe/rxe_qp.c +++ b/drivers/infiniband/sw/rxe/rxe_qp.c @@ -590,15 +590,16 @@ int rxe_qp_from_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask, int err; if (mask & IB_QP_MAX_QP_RD_ATOMIC) { - int max_rd_atomic = __roundup_pow_of_two(attr->max_rd_atomic); + int max_rd_atomic = attr->max_rd_atomic ? + roundup_pow_of_two(attr->max_rd_atomic) : 0; qp->attr.max_rd_atomic = max_rd_atomic; atomic_set(&qp->req.rd_atomic, max_rd_atomic); } if (mask & IB_QP_MAX_DEST_RD_ATOMIC) { - int max_dest_rd_atomic = - __roundup_pow_of_two(attr->max_dest_rd_atomic); + int max_dest_rd_atomic = attr->max_dest_rd_atomic ? + roundup_pow_of_two(attr->max_dest_rd_atomic) : 0; qp->attr.max_dest_rd_atomic = max_dest_rd_atomic;
From the comment above the definition of the roundup_pow_of_two() macro: The result is undefined when n == 0. Hence only pass positive values to roundup_pow_of_two(). This patch fixes the following UBSAN complaint: UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13 shift exponent 64 is too large for 64-bit type 'long unsigned int' Call Trace: dump_stack+0xa5/0xe6 ubsan_epilogue+0x9/0x26 __ubsan_handle_shift_out_of_bounds.cold+0x4c/0xf9 rxe_qp_from_attr.cold+0x37/0x5d [rdma_rxe] rxe_modify_qp+0x59/0x70 [rdma_rxe] _ib_modify_qp+0x5aa/0x7c0 [ib_core] ib_modify_qp+0x3b/0x50 [ib_core] cma_modify_qp_rtr+0x234/0x260 [rdma_cm] __rdma_accept+0x1a7/0x650 [rdma_cm] nvmet_rdma_cm_handler+0x1286/0x14cd [nvmet_rdma] cma_cm_event_handler+0x6b/0x330 [rdma_cm] cma_ib_req_handler+0xe60/0x22d0 [rdma_cm] cm_process_work+0x30/0x140 [ib_cm] cm_req_handler+0x11f4/0x1cd0 [ib_cm] cm_work_handler+0xb8/0x344e [ib_cm] process_one_work+0x569/0xb60 worker_thread+0x7a/0x5d0 kthread+0x1e6/0x210 ret_from_fork+0x24/0x30 Cc: Moni Shoua <monis@mellanox.com> Fixes: 8700e3e7c485 ("Soft RoCE driver") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/sw/rxe/rxe_qp.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)