diff mbox series

[rdma-next,6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

Message ID 20191020071559.9743-7-leon@kernel.org (mailing list archive)
State Superseded
Headers show
Series CM cleanup | expand

Commit Message

Leon Romanovsky Oct. 20, 2019, 7:15 a.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

Reuse recently introduced private_data_len to get IBTA REJ message
private data size.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.20.1

Comments

Bart Van Assche Oct. 24, 2019, 5:01 p.m. UTC | #1 
On 10/20/19 12:15 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Reuse recently introduced private_data_len to get IBTA REJ message
> private data size.
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index daf811abf40a..e66366de11e9 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
>  	case IB_CM_REJ_RECEIVED:
>  		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
>  				 event->private_data,
> -				 IB_CM_REJ_PRIVATE_DATA_SIZE);
> +				 event->private_data_len);
>  		break;
>  	case IB_CM_RTU_RECEIVED:
>  	case IB_CM_USER_ESTABLISHED:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Jason Gunthorpe Oct. 28, 2019, 1:13 p.m. UTC | #2 
On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index daf811abf40a..e66366de11e9 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
>  	case IB_CM_REJ_RECEIVED:
>  		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
>  				 event->private_data,
> -				 IB_CM_REJ_PRIVATE_DATA_SIZE);
> +				 event->private_data_len);

So, I took a look and found a heck of a lot more places assuming the
size of private data that really should be checked if we are going to
introduce a buffer length here.

This is the first couple I noticed, but there were many many more and
they all should be handled..

--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2677,9 +2677,14 @@ static void srp_ib_cm_rej_handler(struct ib_cm_id *cm_id,
                break;
 
        case IB_CM_REJ_CONSUMER_DEFINED:
+               if (event->private_data_len < sizeof(struct srp_login_rej)) {
+                       ch->status = -ECONNRESET;
+                       break;
+               }
+

--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -985,12 +985,15 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id,
 {
        struct ipoib_cm_tx *p = cm_id->context;
        struct ipoib_dev_priv *priv = ipoib_priv(p->dev);
        struct ipoib_cm_data *data = event->private_data;
        struct sk_buff_head skqueue;
        struct ib_qp_attr qp_attr;
        int qp_attr_mask, ret;
        struct sk_buff *skb;
 
+       if (event->private_data_len < sizeof(*data))
+               return -EINVAL;
+


Thanks,
Jason
Leon Romanovsky Oct. 28, 2019, 1:19 p.m. UTC | #3 
On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index daf811abf40a..e66366de11e9 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
> >  	case IB_CM_REJ_RECEIVED:
> >  		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
> >  				 event->private_data,
> > -				 IB_CM_REJ_PRIVATE_DATA_SIZE);
> > +				 event->private_data_len);
>
> So, I took a look and found a heck of a lot more places assuming the
> size of private data that really should be checked if we are going to
> introduce a buffer length here.

But we are not interested to make it dynamic, "private_data_len" has
constant size according to IBTA spec, I just don't want users to be
aware of this.

Why should we add the below checks if it wasn't before?

>
> This is the first couple I noticed, but there were many many more and
> they all should be handled..
>
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -2677,9 +2677,14 @@ static void srp_ib_cm_rej_handler(struct ib_cm_id *cm_id,
>                 break;
>
>         case IB_CM_REJ_CONSUMER_DEFINED:
> +               if (event->private_data_len < sizeof(struct srp_login_rej)) {
> +                       ch->status = -ECONNRESET;
> +                       break;
> +               }
> +
>
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -985,12 +985,15 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id,
>  {
>         struct ipoib_cm_tx *p = cm_id->context;
>         struct ipoib_dev_priv *priv = ipoib_priv(p->dev);
>         struct ipoib_cm_data *data = event->private_data;
>         struct sk_buff_head skqueue;
>         struct ib_qp_attr qp_attr;
>         int qp_attr_mask, ret;
>         struct sk_buff *skb;
>
> +       if (event->private_data_len < sizeof(*data))
> +               return -EINVAL;
> +
>
>
> Thanks,
> Jason
Jason Gunthorpe Oct. 28, 2019, 1:43 p.m. UTC | #4 
On Mon, Oct 28, 2019 at 03:19:17PM +0200, Leon Romanovsky wrote:
> On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > index daf811abf40a..e66366de11e9 100644
> > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
> > >  	case IB_CM_REJ_RECEIVED:
> > >  		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
> > >  				 event->private_data,
> > > -				 IB_CM_REJ_PRIVATE_DATA_SIZE);
> > > +				 event->private_data_len);
> >
> > So, I took a look and found a heck of a lot more places assuming the
> > size of private data that really should be checked if we are going to
> > introduce a buffer length here.
> 
> But we are not interested to make it dynamic, "private_data_len" has
> constant size according to IBTA spec, I just don't want users to be
> aware of this.
> 
> Why should we add the below checks if it wasn't before?

Why are we doing any of this then?

Jason
Leon Romanovsky Oct. 28, 2019, 2:06 p.m. UTC | #5 
On Mon, Oct 28, 2019 at 10:43:51AM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 28, 2019 at 03:19:17PM +0200, Leon Romanovsky wrote:
> > On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> > > On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > index daf811abf40a..e66366de11e9 100644
> > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > @@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
> > > >  	case IB_CM_REJ_RECEIVED:
> > > >  		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
> > > >  				 event->private_data,
> > > > -				 IB_CM_REJ_PRIVATE_DATA_SIZE);
> > > > +				 event->private_data_len);
> > >
> > > So, I took a look and found a heck of a lot more places assuming the
> > > size of private data that really should be checked if we are going to
> > > introduce a buffer length here.
> >
> > But we are not interested to make it dynamic, "private_data_len" has
> > constant size according to IBTA spec, I just don't want users to be
> > aware of this.
> >
> > Why should we add the below checks if it wasn't before?
>
> Why are we doing any of this then?

The final goal will be to present all CM messages as binary blobs with
access through specific GET/SET helpers, those including access to
private data. It is needed to completely eliminate be32 madness in
IB/core code.

Thanks

>
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index daf811abf40a..e66366de11e9 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2609,7 +2609,7 @@  static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
 		break;
 	case IB_CM_RTU_RECEIVED:
 	case IB_CM_USER_ESTABLISHED: