> From: Parav Pandit [mailto:parav@xxxxxxxxxxxx] > Sent: Monday, June 11, 2018 7:13 PM > > Hi Michal, > > > > Some RoCE specific code in qedr_modify_qp was run over an iWARP device > > when running perftest benchmarks without the -R option. > > > > The commit 3e44e0ee0893 ("IB/providers: Avoid null netdev check for > > RoCE") exposed this. Dropping the check for NULL pointer on ndev in > > qedr_modify_qp lead to a null pointer dereference when running over > > iWARP. Before the code would identify ndev as being NULL and return an > error. > Previously get_gid_info_from_table() with valid gid index would return > success and was doing things conditionally if ndev was set. > > > > > Fixes: 3e44e0ee0893 ("IB/providers: Avoid null netdev check for RoCE") > > Signed-off-by: Ariel Elior <Ariel.Elior@xxxxxxxxxx> > > Signed-off-by: Michal Kalderon <Michal.Kalderon@xxxxxxxxxx> > > --- > > drivers/infiniband/hw/qedr/verbs.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > > b/drivers/infiniband/hw/qedr/verbs.c > > index 3f9afc0..f86223a 100644 > > --- a/drivers/infiniband/hw/qedr/verbs.c > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > @@ -1957,6 +1957,9 @@ int qedr_modify_qp(struct ib_qp *ibqp, struct > > ib_qp_attr *attr, > > } > > > > if (attr_mask & (IB_QP_AV | IB_QP_PATH_MTU)) { > > + if (rdma_protocol_iwarp(&dev->ibdev, 1)) > > + return -EINVAL; > > + > RB: parav@xxxxxxxxxxxx > > A check like rdma_protocol_roce() similar to what you have already for stack > check makes it more clear that this is for roce. > That likely preserves old behavior, I think. > > - if (attr_mask & (IB_QP_AV | IB_QP_PATH_MTU)) { > + if (attr_mask & (IB_QP_AV | IB_QP_PATH_MTU) && > + rdma_protocol_roce(&dev->ibdev, 1)) { I don't want the function to continue at all in this case, just exit, asking if roce would continue onwards. > > Many checks in this function are for RoCE such as IB_QP_RETRY_CNT, > IB_QP_RNR_RETRY, IB_QP_RQ_PSN, IB_QP_MIN_RNR_TIMER, > IB_QP_SQ_PSN, IB_QP_QKEY, IB_QP_PKEY_INDEX. > > So for for-next, (not for-rc), > 1. You should refactor the function to split code for roce and iwarp so that > right checks and settings are done for right transport. > It makes code more clear. > I totally agree and that was the plan, should have emphasized that in the patch description. > 2. I would expect that ib_modify_qp_is_ok() should be extended for iWarp > as well, so that when IB_QP_AV | IB_QP_PATH_MTU is set, provider driver > doesn't have to do this check to return failure error code. I will review. > > 3. ib_modify_qp_is_ok() already has input parameter to pass link_layer. It > should be extended, to honor do right state specific mask checks for right link > layer. Sounds reasonable, will take a look. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
