| Message ID | 20170718203539.6777-1-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jul 18, 2017 at 11:35:39PM +0300, Yuval Shaia wrote: > These messages are warning so let's print them accordingly. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe_av.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c > index 5bddf469361b..272db3792c50 100644 > --- a/drivers/infiniband/sw/rxe/rxe_av.c > +++ b/drivers/infiniband/sw/rxe/rxe_av.c > @@ -39,7 +39,7 @@ int rxe_av_chk_attr(struct rxe_dev *rxe, struct rdma_ah_attr *attr) > struct rxe_port *port; > > if (rdma_ah_get_port_num(attr) != 1) { > - pr_info("invalid port_num = %d\n", rdma_ah_get_port_num(attr)); > + pr_warn("invalid port_num = %d\n", rdma_ah_get_port_num(attr)); > return -EINVAL; > } How this code can be executed? IB/core ensures that port_num is in range. You can remove this check. > > @@ -49,7 +49,7 @@ int rxe_av_chk_attr(struct rxe_dev *rxe, struct rdma_ah_attr *attr) > u8 sgid_index = rdma_ah_read_grh(attr)->sgid_index; > > if (sgid_index > port->attr.gid_tbl_len) { > - pr_info("invalid sgid index = %d\n", sgid_index); > + pr_warn("invalid sgid index = %d\n", sgid_index); > return -EINVAL; > } > } > -- > 2.13.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> How this code can be executed? IB/core ensures that port_num is in range. > You can remove this check. > In this case ah_attr can come from the application and a validity check is necessary. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 19, 2017 at 10:11:50AM +0300, Moni Shoua wrote: > > How this code can be executed? IB/core ensures that port_num is in range. > > You can remove this check. > > > In this case ah_attr can come from the application and a validity > check is necessary. It is a bug if it comes directly without rdma_is_port_valid() check in the IB/core. Currently modify_qp and create_ah are checking it and ensuring that user won't provide illegal port number. Thanks
On Wed, Jul 19, 2017 at 10:51:19AM +0300, Leon Romanovsky wrote: > On Wed, Jul 19, 2017 at 10:11:50AM +0300, Moni Shoua wrote: > > > How this code can be executed? IB/core ensures that port_num is in range. > > > You can remove this check. > > > > > In this case ah_attr can come from the application and a validity > > check is necessary. > > It is a bug if it comes directly without rdma_is_port_valid() check in the > IB/core. Currently modify_qp and create_ah are checking it and ensuring that > user won't provide illegal port number. Not sure i see the whole picture but something does not fit, will appreciate a guidance here. 1. Application calls ibv_modify_qp which in turn fills out a cmd object and "calls" ib_uverbs.ib_uverbs_modify_qp. 2. ib_uverbs_modify_qp copy the cmd from userspace and calls modify_qp. 3. modify_qp, among some other stuff, verifies port validity (which prove your point) and calls driver's modify_qp hook (in our case rxe_modify_qp). Is the above correct? What i do not understand is the check that is done in step #2 since port_num is not set when moving from state INIT to state RTR. RXE on the other hands validate port_num only when needed (mask & IB_QP_AV). Looks like the check in step #2 is wrong. What am i missing here? Yuval > > Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 19, 2017 at 02:04:42PM +0300, Yuval Shaia wrote: > On Wed, Jul 19, 2017 at 10:51:19AM +0300, Leon Romanovsky wrote: > > On Wed, Jul 19, 2017 at 10:11:50AM +0300, Moni Shoua wrote: > > > > How this code can be executed? IB/core ensures that port_num is in range. > > > > You can remove this check. > > > > > > > In this case ah_attr can come from the application and a validity > > > check is necessary. > > > > It is a bug if it comes directly without rdma_is_port_valid() check in the > > IB/core. Currently modify_qp and create_ah are checking it and ensuring that > > user won't provide illegal port number. > > Not sure i see the whole picture but something does not fit, will > appreciate a guidance here. > > 1. Application calls ibv_modify_qp which in turn fills out a cmd object and > "calls" ib_uverbs.ib_uverbs_modify_qp. > 2. ib_uverbs_modify_qp copy the cmd from userspace and calls modify_qp. > 3. modify_qp, among some other stuff, verifies port validity (which prove > your point) and calls driver's modify_qp hook (in our case rxe_modify_qp). > > Is the above correct? > > What i do not understand is the check that is done in step #2 since > port_num is not set when moving from state INIT to state RTR. > RXE on the other hands validate port_num only when needed (mask & > IB_QP_AV). > > Looks like the check in step #2 is wrong. > What am i missing here? Maybe you missing the Mustafa's patch? https://patchwork.kernel.org/patch/9841241/ Thanks > > Yuval > > > > > Thanks > >
On Wed, Jul 19, 2017 at 08:26:13PM +0300, Leon Romanovsky wrote: > On Wed, Jul 19, 2017 at 02:04:42PM +0300, Yuval Shaia wrote: > > On Wed, Jul 19, 2017 at 10:51:19AM +0300, Leon Romanovsky wrote: > > > On Wed, Jul 19, 2017 at 10:11:50AM +0300, Moni Shoua wrote: > > > > > How this code can be executed? IB/core ensures that port_num is in range. > > > > > You can remove this check. > > > > > > > > > In this case ah_attr can come from the application and a validity > > > > check is necessary. > > > > > > It is a bug if it comes directly without rdma_is_port_valid() check in the > > > IB/core. Currently modify_qp and create_ah are checking it and ensuring that > > > user won't provide illegal port number. > > > > Not sure i see the whole picture but something does not fit, will > > appreciate a guidance here. > > > > 1. Application calls ibv_modify_qp which in turn fills out a cmd object and > > "calls" ib_uverbs.ib_uverbs_modify_qp. > > 2. ib_uverbs_modify_qp copy the cmd from userspace and calls modify_qp. > > 3. modify_qp, among some other stuff, verifies port validity (which prove > > your point) and calls driver's modify_qp hook (in our case rxe_modify_qp). > > > > Is the above correct? > > > > What i do not understand is the check that is done in step #2 since > > port_num is not set when moving from state INIT to state RTR. > > RXE on the other hands validate port_num only when needed (mask & > > IB_QP_AV). > > > > Looks like the check in step #2 is wrong. > > What am i missing here? > > Maybe you missing the Mustafa's patch? > https://patchwork.kernel.org/patch/9841241/ Exactly!! :) Thanks, So, will post v1 of this patch with the removal of the redundant check. > > Thanks > > > > > Yuval > > > > > > > > Thanks > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c index 5bddf469361b..272db3792c50 100644 --- a/drivers/infiniband/sw/rxe/rxe_av.c +++ b/drivers/infiniband/sw/rxe/rxe_av.c @@ -39,7 +39,7 @@ int rxe_av_chk_attr(struct rxe_dev *rxe, struct rdma_ah_attr *attr) struct rxe_port *port; if (rdma_ah_get_port_num(attr) != 1) { - pr_info("invalid port_num = %d\n", rdma_ah_get_port_num(attr)); + pr_warn("invalid port_num = %d\n", rdma_ah_get_port_num(attr)); return -EINVAL; } @@ -49,7 +49,7 @@ int rxe_av_chk_attr(struct rxe_dev *rxe, struct rdma_ah_attr *attr) u8 sgid_index = rdma_ah_read_grh(attr)->sgid_index; if (sgid_index > port->attr.gid_tbl_len) { - pr_info("invalid sgid index = %d\n", sgid_index); + pr_warn("invalid sgid index = %d\n", sgid_index); return -EINVAL; } }
These messages are warning so let's print them accordingly. Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- drivers/infiniband/sw/rxe/rxe_av.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)