> From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] > Sent: Monday, June 11, 2018 7:36 PM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Kalderon, Michal <Michal.Kalderon@xxxxxxxxxx>; > dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Bason, Yuval > <Yuval.Bason@xxxxxxxxxx>; Elior, Ariel <Ariel.Elior@xxxxxxxxxx>; Steve > Wise <swise@xxxxxxxxxxx>; Faisal Latif <faisal.latif@xxxxxxxxx>; Shiraz > Saleem <shiraz.saleem@xxxxxxxxx> > Subject: Re: [PATCH for-rc] RDMA/qedr: Fix Null pointer dereference when > running over iWARP without RDMA-CM > > 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.. I agree, we need to revisit whether iWARP needs a gid_attr structure at all as it is not based on gids and what the alternative would be. > > 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
