On Mon, Jun 11, 2018 at 04:13:23PM +0000, Parav Pandit wrote: > Hi Michal, > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > > owner@xxxxxxxxxxxxxxx] On Behalf Of Michal Kalderon > > Sent: Monday, June 11, 2018 2:20 AM > > To: michal.kalderon@xxxxxxxxxx; Jason Gunthorpe <jgg@xxxxxxxxxxxx>; > > dledford@xxxxxxxxxx > > Cc: linux-rdma@xxxxxxxxxxxxxxx; yuval.bason@xxxxxxxxxx; Michal Kalderon > > <Michal.Kalderon@xxxxxxxxxx>; Ariel Elior <Ariel.Elior@xxxxxxxxxx> > > Subject: [PATCH for-rc] RDMA/qedr: Fix Null pointer dereference when running > > over iWARP without RDMA-CM > > > > 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 > > +++ 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)) { > > 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. > > 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. > > 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. This comment will make more sense when Parav's next series is posted.. But I think the iWarp team needs to get their heads together and figure out how to create a struct ib_gid_attr for iWarp that includes the proper netdev. I seriously doubt iWarp will work fully properly re namespaces and other difficult races without this.. If enough iWarp folks are coming the plumbers conference in November it would make a good RDMA mini-conf topic. Jason -- 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
