| Message ID | 20180611072020.4046-1-Michal.Kalderon@cavium.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
Hi Michal, > -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Michal Kalderon > Sent: Monday, June 11, 2018 2:20 AM > To: michal.kalderon@cavium.com; Jason Gunthorpe <jgg@mellanox.com>; > dledford@redhat.com > Cc: linux-rdma@vger.kernel.org; yuval.bason@cavium.com; Michal Kalderon > <Michal.Kalderon@cavium.com>; Ariel Elior <Ariel.Elior@cavium.com> > 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@cavium.com> > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > --- > 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@mellanox.com 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. -- 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 Mon, Jun 11, 2018 at 04:13:23PM +0000, Parav Pandit wrote: > Hi Michal, > > > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > > owner@vger.kernel.org] On Behalf Of Michal Kalderon > > Sent: Monday, June 11, 2018 2:20 AM > > To: michal.kalderon@cavium.com; Jason Gunthorpe <jgg@mellanox.com>; > > dledford@redhat.com > > Cc: linux-rdma@vger.kernel.org; yuval.bason@cavium.com; Michal Kalderon > > <Michal.Kalderon@cavium.com>; Ariel Elior <Ariel.Elior@cavium.com> > > 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@cavium.com> > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > 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@mellanox.com > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 11, 2018 at 10:20:20AM +0300, Michal Kalderon wrote: > 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. > > Fixes: 3e44e0ee0893 ("IB/providers: Avoid null netdev check for RoCE") > Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > --- > drivers/infiniband/hw/qedr/verbs.c | 3 +++ > 1 file changed, 3 insertions(+) Applied to for-rc, thanks Jason -- 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
> From: Parav Pandit [mailto:parav@mellanox.com] > 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@cavium.com> > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > --- > > 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@mellanox.com > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > Sent: Monday, June 11, 2018 7:36 PM > To: Parav Pandit <parav@mellanox.com> > Cc: Kalderon, Michal <Michal.Kalderon@cavium.com>; > dledford@redhat.com; linux-rdma@vger.kernel.org; Bason, Yuval > <Yuval.Bason@cavium.com>; Elior, Ariel <Ariel.Elior@cavium.com>; Steve > Wise <swise@chelsio.com>; Faisal Latif <faisal.latif@intel.com>; Shiraz > Saleem <shiraz.saleem@intel.com> > 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@vger.kernel.org [mailto:linux-rdma- > > > owner@vger.kernel.org] On Behalf Of Michal Kalderon > > > Sent: Monday, June 11, 2018 2:20 AM > > > To: michal.kalderon@cavium.com; Jason Gunthorpe > <jgg@mellanox.com>; > > > dledford@redhat.com > > > Cc: linux-rdma@vger.kernel.org; yuval.bason@cavium.com; Michal > > > Kalderon <Michal.Kalderon@cavium.com>; Ariel Elior > > > <Ariel.Elior@cavium.com> > > > 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@cavium.com> > > > Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> > > > 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@mellanox.com > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 12, 2018 at 09:44:44AM +0000, Kalderon, Michal wrote: > > 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. 'gid_attr' is the name of the struct that links the AH/QP/etc to a source IP/GID/netdev. It is an unfortunate name for iwarp, but it is still what iwarp needs to use. Jason -- 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/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; + if (attr_mask & IB_QP_PATH_MTU) { if (attr->path_mtu < IB_MTU_256 || attr->path_mtu > IB_MTU_4096) {
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. Fixes: 3e44e0ee0893 ("IB/providers: Avoid null netdev check for RoCE") Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com> --- drivers/infiniband/hw/qedr/verbs.c | 3 +++ 1 file changed, 3 insertions(+)