diff mbox

[RE-RESEND,V2,for-next,4/5] IB/core: Make sure that the PSN does not overflow

Message ID 1423137232-24587-5-git-send-email-ogerlitz@mellanox.com (mailing list archive)
State Rejected
Headers show

Commit Message

Or Gerlitz Feb. 5, 2015, 11:53 a.m. UTC
From: Majd Dibbiny <majd@mellanox.com>

The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
space applications shouldn't use the 8 most significant bits in the 32 bits
variables to avoid overflow in modify_qp.

Fixed the PSN generation by the RDMA-CM to mask out the 8 most significant bits,
also mask out these bits in uverbs for attributes provided by user-space.

Signed-off-by: Majd Dibbiny <majd@mellanox.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/infiniband/core/cma.c        |    1 +
 drivers/infiniband/core/uverbs_cmd.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Ira Weiny Feb. 5, 2015, 10:27 p.m. UTC | #1
> Subject: [PATCH RE-RESEND V2 for-next 4/5] IB/core: Make sure that the PSN
> does not overflow

Is there a particular bug  which this fixes?

Ira

> 
> From: Majd Dibbiny <majd@mellanox.com>
> 
> The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
> space applications shouldn't use the 8 most significant bits in the 32 bits
> variables to avoid overflow in modify_qp.
> 
> Fixed the PSN generation by the RDMA-CM to mask out the 8 most significant
> bits, also mask out these bits in uverbs for attributes provided by user-space.
> 
> Signed-off-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  drivers/infiniband/core/cma.c        |    1 +
>  drivers/infiniband/core/uverbs_cmd.c |    4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index d570030..fab0ee5 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -512,6 +512,7 @@ struct rdma_cm_id
> *rdma_create_id(rdma_cm_event_handler event_handler,
>  	INIT_LIST_HEAD(&id_priv->listen_list);
>  	INIT_LIST_HEAD(&id_priv->mc_list);
>  	get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
> +	id_priv->seq_num &= 0xffffff;
> 
>  	return &id_priv->id;
>  }
> diff --git a/drivers/infiniband/core/uverbs_cmd.c
> b/drivers/infiniband/core/uverbs_cmd.c
> index 532d8eb..ecb6430 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2053,8 +2053,8 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file
> *file,
>  	attr->path_mtu 		  = cmd.path_mtu;
>  	attr->path_mig_state 	  = cmd.path_mig_state;
>  	attr->qkey 		  = cmd.qkey;
> -	attr->rq_psn 		  = cmd.rq_psn;
> -	attr->sq_psn 		  = cmd.sq_psn;
> +	attr->rq_psn              = cmd.rq_psn & 0xffffff;
> +	attr->sq_psn              = cmd.sq_psn & 0xffffff;
>  	attr->dest_qp_num 	  = cmd.dest_qp_num;
>  	attr->qp_access_flags 	  = cmd.qp_access_flags;
>  	attr->pkey_index 	  = cmd.pkey_index;
> --
> 1.7.1
> 
> --
> 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
--
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
Or Gerlitz Feb. 6, 2015, 3:28 p.m. UTC | #2
On Fri, Feb 6, 2015 at 12:27 AM, Weiny, Ira <ira.weiny@intel.com> wrote:
>> Subject: [PATCH RE-RESEND V2 for-next 4/5] IB/core: Make sure that the PSN
>> does not overflow
>
> Is there a particular bug  which this fixes?

Not something that I am aware too (but we can take a look @ our
regression data-base - Majd, please do), more of maintenance fix.
--
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
Yann Droneaud Feb. 16, 2015, 9:22 p.m. UTC | #3
Hi,

Le jeudi 05 février 2015 à 13:53 +0200, Or Gerlitz a écrit :
> From: Majd Dibbiny <majd@mellanox.com>
> 
> The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
> space applications shouldn't use the 8 most significant bits in the 32 bits
> variables to avoid overflow in modify_qp.
> 
> Fixed the PSN generation by the RDMA-CM to mask out the 8 most significant bits,
> also mask out these bits in uverbs for attributes provided by user-space.
> 
> Signed-off-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  drivers/infiniband/core/cma.c        |    1 +
>  drivers/infiniband/core/uverbs_cmd.c |    4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index d570030..fab0ee5 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -512,6 +512,7 @@ struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
>  	INIT_LIST_HEAD(&id_priv->listen_list);
>  	INIT_LIST_HEAD(&id_priv->mc_list);
>  	get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
> +	id_priv->seq_num &= 0xffffff;
>  
>  	return &id_priv->id;
>  }
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 532d8eb..ecb6430 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2053,8 +2053,8 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
>  	attr->path_mtu 		  = cmd.path_mtu;
>  	attr->path_mig_state 	  = cmd.path_mig_state;
>  	attr->qkey 		  = cmd.qkey;
> -	attr->rq_psn 		  = cmd.rq_psn;
> -	attr->sq_psn 		  = cmd.sq_psn;
> +	attr->rq_psn              = cmd.rq_psn & 0xffffff;
> +	attr->sq_psn              = cmd.sq_psn & 0xffffff;
>  	attr->dest_qp_num 	  = cmd.dest_qp_num;
>  	attr->qp_access_flags 	  = cmd.qp_access_flags;
>  	attr->pkey_index 	  = cmd.pkey_index;

0xffffff could be made a #define

Question: userspace is allowed to ask for a PSN on 32bits, but it will 
be silently truncated, is it going to puzzle applications ?

Anyway, it would have been better to return an error in
the first place ... not sure if we can do it now ...

Regards.
Ira Weiny Feb. 16, 2015, 10:06 p.m. UTC | #4
> 

> 0xffffff could be made a #define

> 

> Question: userspace is allowed to ask for a PSN on 32bits, but it will be silently

> truncated, is it going to puzzle applications ?

> 

> Anyway, it would have been better to return an error in the first place ... not

> sure if we can do it now ...

> 


Yes I'm still not sure what bug this fixes.  Or did you ever find out?

Ira
diff mbox

Patch

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index d570030..fab0ee5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -512,6 +512,7 @@  struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
 	INIT_LIST_HEAD(&id_priv->listen_list);
 	INIT_LIST_HEAD(&id_priv->mc_list);
 	get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
+	id_priv->seq_num &= 0xffffff;
 
 	return &id_priv->id;
 }
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 532d8eb..ecb6430 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2053,8 +2053,8 @@  ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
 	attr->path_mtu 		  = cmd.path_mtu;
 	attr->path_mig_state 	  = cmd.path_mig_state;
 	attr->qkey 		  = cmd.qkey;
-	attr->rq_psn 		  = cmd.rq_psn;
-	attr->sq_psn 		  = cmd.sq_psn;
+	attr->rq_psn              = cmd.rq_psn & 0xffffff;
+	attr->sq_psn              = cmd.sq_psn & 0xffffff;
 	attr->dest_qp_num 	  = cmd.dest_qp_num;
 	attr->qp_access_flags 	  = cmd.qp_access_flags;
 	attr->pkey_index 	  = cmd.pkey_index;