| Message ID | 20191020071559.9743-5-leon@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | CM cleanup | expand |
On Sun, Oct 20, 2019 at 10:18 AM Leon Romanovsky <leon@kernel.org> wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > QPN is supplied by kernel users who controls and creates valid QPs, AFAIK this can also arrive from user-space, agree? > such flow ensures that QPN is limited to 24bits and no need to mask > already valid QPN.
On Sun, Oct 20, 2019 at 11:48:39AM +0300, Or Gerlitz wrote: > On Sun, Oct 20, 2019 at 10:18 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > QPN is supplied by kernel users who controls and creates valid QPs, > > AFAIK this can also arrive from user-space, agree? Not in this path, anyway we are masking it again while writing to the spec struct, > > > such flow ensures that QPN is limited to 24bits and no need to mask > > already valid QPN.
On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > QPN is supplied by kernel users who controls and creates valid QPs, > such flow ensures that QPN is limited to 24bits and no need to mask > already valid QPN. > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/core/cm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 7ffa16ea5fe3..2eb8e1fab962 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -2101,7 +2101,7 @@ 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); > + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); It does seem like this value comes from userspace: ucma_connect() ucma_copy_conn_param() dst->qp_num = src->qp_num rdma_connect(.., &dst) if (!id->qp) { id_priv->qp_num = conn_param->qp_num; vs cma_accept_ib() rep.qp_num = id_priv->qp_num; Maybe this needs to add some masking to ucma_copy_conn_param()? Jason
On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote: > On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > QPN is supplied by kernel users who controls and creates valid QPs, > > such flow ensures that QPN is limited to 24bits and no need to mask > > already valid QPN. > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > --- > > drivers/infiniband/core/cm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > > index 7ffa16ea5fe3..2eb8e1fab962 100644 > > --- a/drivers/infiniband/core/cm.c > > +++ b/drivers/infiniband/core/cm.c > > @@ -2101,7 +2101,7 @@ 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); > > + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); > > It does seem like this value comes from userspace: > > ucma_connect() > ucma_copy_conn_param() > dst->qp_num = src->qp_num > rdma_connect(.., &dst) > if (!id->qp) { > id_priv->qp_num = conn_param->qp_num; > > vs > > cma_accept_ib() > rep.qp_num = id_priv->qp_num; > > Maybe this needs to add some masking to ucma_copy_conn_param()? Thanks for the callstack, Or pointed it to me too, but I missed this flow. Let's create a pre-patch with QPN masking. Thanks > > Jason
On Mon, Oct 28, 2019 at 03:13:33PM +0200, Leon Romanovsky wrote: > On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote: > > On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > QPN is supplied by kernel users who controls and creates valid QPs, > > > such flow ensures that QPN is limited to 24bits and no need to mask > > > already valid QPN. > > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > drivers/infiniband/core/cm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > > > index 7ffa16ea5fe3..2eb8e1fab962 100644 > > > +++ b/drivers/infiniband/core/cm.c > > > @@ -2101,7 +2101,7 @@ 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); > > > + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); > > > > It does seem like this value comes from userspace: > > > > ucma_connect() > > ucma_copy_conn_param() > > dst->qp_num = src->qp_num > > rdma_connect(.., &dst) > > if (!id->qp) { > > id_priv->qp_num = conn_param->qp_num; > > > > vs > > > > cma_accept_ib() > > rep.qp_num = id_priv->qp_num; > > > > Maybe this needs to add some masking to ucma_copy_conn_param()? > > Thanks for the callstack, Or pointed it to me too, but I missed this flow. > Let's create a pre-patch with QPN masking. You'll need to check all the id_priv->qp_num users, I stopped when I found the above Jason
On Mon, Oct 28, 2019 at 10:44:57AM -0300, Jason Gunthorpe wrote: > On Mon, Oct 28, 2019 at 03:13:33PM +0200, Leon Romanovsky wrote: > > On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote: > > > On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > > > QPN is supplied by kernel users who controls and creates valid QPs, > > > > such flow ensures that QPN is limited to 24bits and no need to mask > > > > already valid QPN. > > > > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > > drivers/infiniband/core/cm.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > > > > index 7ffa16ea5fe3..2eb8e1fab962 100644 > > > > +++ b/drivers/infiniband/core/cm.c > > > > @@ -2101,7 +2101,7 @@ 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); > > > > + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); > > > > > > It does seem like this value comes from userspace: > > > > > > ucma_connect() > > > ucma_copy_conn_param() > > > dst->qp_num = src->qp_num > > > rdma_connect(.., &dst) > > > if (!id->qp) { > > > id_priv->qp_num = conn_param->qp_num; > > > > > > vs > > > > > > cma_accept_ib() > > > rep.qp_num = id_priv->qp_num; > > > > > > Maybe this needs to add some masking to ucma_copy_conn_param()? > > > > Thanks for the callstack, Or pointed it to me too, but I missed this flow. > > Let's create a pre-patch with QPN masking. > > You'll need to check all the id_priv->qp_num users, I stopped when I > found the above Initially, I wanted to add masking in rdma_connect(), but decided that it is not the cleanest approach, so I grepped to see rdma_conn_param users. I'll continue to grep. Thanks > > Jason
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index 7ffa16ea5fe3..2eb8e1fab962 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -2101,7 +2101,7 @@ 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); + cm_id_priv->local_qpn = cpu_to_be32(param->qp_num); out: spin_unlock_irqrestore(&cm_id_priv->lock, flags); return ret;