[rdma-next,4/6] RDMA/cm: Delete useless QPN masking
diff mbox series

Message ID 20191020071559.9743-5-leon@kernel.org
State Changes Requested
Headers show
Series
  • CM cleanup
Related show

Commit Message

Leon Romanovsky Oct. 20, 2019, 7:15 a.m. UTC
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(-)

Comments

Or Gerlitz Oct. 20, 2019, 8:48 a.m. UTC | #1
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.
Leon Romanovsky Oct. 20, 2019, 8:54 a.m. UTC | #2
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.
Jason Gunthorpe Oct. 28, 2019, 12:52 p.m. UTC | #3
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
Leon Romanovsky Oct. 28, 2019, 1:13 p.m. UTC | #4
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
Jason Gunthorpe Oct. 28, 2019, 1:44 p.m. UTC | #5
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
Leon Romanovsky Oct. 28, 2019, 2:02 p.m. UTC | #6
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

Patch
diff mbox series

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;