| Message ID | 20191121181313.129430-4-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Organize code according to IBTA layout | expand |
> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index 0274e9b704be..57e68491a2fd 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id, > dst->retry_count = src->retry_count; > dst->rnr_retry_count = src->rnr_retry_count; > dst->srq = src->srq; > - dst->qp_num = src->qp_num; > + dst->qp_num = src->qp_num & 0xFFFFFF; Isn't src->qp_num coming from userspace? Why not return -EINVAL in such a case? Ira
On Thu, Nov 21, 2019 at 01:38:24PM -0800, Ira Weiny wrote: > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > > index 0274e9b704be..57e68491a2fd 100644 > > --- a/drivers/infiniband/core/ucma.c > > +++ b/drivers/infiniband/core/ucma.c > > @@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id, > > dst->retry_count = src->retry_count; > > dst->rnr_retry_count = src->rnr_retry_count; > > dst->srq = src->srq; > > - dst->qp_num = src->qp_num; > > + dst->qp_num = src->qp_num & 0xFFFFFF; > > Isn't src->qp_num coming from userspace? Why not return -EINVAL in such a > case? I afraid that it will break userspace application, we had similar case in uverbs, where we added WARN_ON() while masked PSN and got endless amount of bug reports from our enterprise colleagues who didn't clear memory above those 24bits and saw kernel splats. Thanks > > Ira >
> > On Thu, Nov 21, 2019 at 01:38:24PM -0800, Ira Weiny wrote: > > > diff --git a/drivers/infiniband/core/ucma.c > > > b/drivers/infiniband/core/ucma.c index 0274e9b704be..57e68491a2fd > > > 100644 > > > --- a/drivers/infiniband/core/ucma.c > > > +++ b/drivers/infiniband/core/ucma.c > > > @@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct > rdma_cm_id *id, > > > dst->retry_count = src->retry_count; > > > dst->rnr_retry_count = src->rnr_retry_count; > > > dst->srq = src->srq; > > > - dst->qp_num = src->qp_num; > > > + dst->qp_num = src->qp_num & 0xFFFFFF; > > > > Isn't src->qp_num coming from userspace? Why not return -EINVAL in > > such a case? > > I afraid that it will break userspace application, we had similar case in uverbs, > where we added WARN_ON() while masked PSN and got endless amount of > bug reports from our enterprise colleagues who didn't clear memory above > those 24bits and saw kernel splats. I want to say there is less change of that here because librdmacm should be handling most of these numbers within the library. I have no dog in this fight so... Ira
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index c341a68b6f97..efa2d329da30 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -2102,7 +2102,10 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id, cm_id_priv->initiator_depth = param->initiator_depth; cm_id_priv->responder_resources = param->responder_resources; cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg); - cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF); + WARN_ONCE(param->qp_num & 0xFF000000, + "IBTA declares QPN to be 24 bits, but it is 0x%X\n", + param->qp_num); + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); out: spin_unlock_irqrestore(&cm_id_priv->lock, flags); return ret; diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index 0274e9b704be..57e68491a2fd 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1045,7 +1045,7 @@ static void ucma_copy_conn_param(struct rdma_cm_id *id, dst->retry_count = src->retry_count; dst->rnr_retry_count = src->rnr_retry_count; dst->srq = src->srq; - dst->qp_num = src->qp_num; + dst->qp_num = src->qp_num & 0xFFFFFF; dst->qkey = (id->route.addr.src_addr.ss_family == AF_IB) ? src->qkey : 0; }