diff mbox series

[rdma-rc] RDMA/cm: Fix missing RDMA_CM_EVENT_REJECTED event after receiving REJ message

Message ID 20200406173242.1465911-1-leon@kernel.org (mailing list archive)
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series [rdma-rc] RDMA/cm: Fix missing RDMA_CM_EVENT_REJECTED event after receiving REJ message | expand

Commit Message

Leon Romanovsky April 6, 2020, 5:32 p.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

The cm_reset_to_idle() call before formatting event changed the CM_ID
state from IB_CM_REQ_RCVD to be IB_CM_IDLE. It caused to wrong value
of CM_REJ_MESSAGE_REJECTED field.

The result of that was that rdma_reject() calls in the passive side
didn't generate RDMA_CM_EVENT_REJECTED event in the active side.

Fixes: 81ddb41f876d ("RDMA/cm: Allow ib_send_cm_rej() to be done under lock")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

--
2.25.1

Comments

Jason Gunthorpe April 6, 2020, 5:45 p.m. UTC | #1 
On Mon, Apr 06, 2020 at 08:32:42PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> The cm_reset_to_idle() call before formatting event changed the CM_ID
> state from IB_CM_REQ_RCVD to be IB_CM_IDLE. It caused to wrong value
> of CM_REJ_MESSAGE_REJECTED field.
> 
> The result of that was that rdma_reject() calls in the passive side
> didn't generate RDMA_CM_EVENT_REJECTED event in the active side.
> 
> Fixes: 81ddb41f876d ("RDMA/cm: Allow ib_send_cm_rej() to be done under lock")
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/cm.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index bbbfa77dbce7..06f8eeba423a 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -1843,11 +1843,9 @@ static void cm_format_mra(struct cm_mra_msg *mra_msg,
> 
>  static void cm_format_rej(struct cm_rej_msg *rej_msg,
>  			  struct cm_id_private *cm_id_priv,
> -			  enum ib_cm_rej_reason reason,
> -			  void *ari,
> -			  u8 ari_length,
> -			  const void *private_data,
> -			  u8 private_data_len)
> +			  enum ib_cm_rej_reason reason, void *ari,
> +			  u8 ari_length, const void *private_data,
> +			  u8 private_data_len, enum ib_cm_state state)
>  {
>  	lockdep_assert_held(&cm_id_priv->lock);
> 
> @@ -1855,7 +1853,7 @@ static void cm_format_rej(struct cm_rej_msg *rej_msg,
>  	IBA_SET(CM_REJ_REMOTE_COMM_ID, rej_msg,
>  		be32_to_cpu(cm_id_priv->id.remote_id));
> 
> -	switch(cm_id_priv->id.state) {
> +	switch (state) {
>  	case IB_CM_REQ_RCVD:
>  		IBA_SET(CM_REJ_LOCAL_COMM_ID, rej_msg, be32_to_cpu(0));
>  		IBA_SET(CM_REJ_MESSAGE_REJECTED, rej_msg, CM_MSG_RESPONSE_REQ);
> @@ -1920,8 +1918,9 @@ static void cm_dup_req_handler(struct cm_work *work,
>  			      cm_id_priv->private_data_len);
>  		break;
>  	case IB_CM_TIMEWAIT:
> -		cm_format_rej((struct cm_rej_msg *) msg->mad, cm_id_priv,
> -			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0);
> +		cm_format_rej((struct cm_rej_msg *)msg->mad, cm_id_priv,
> +			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0,
> +			      cm_id_priv->id.state);

This can just be IB_CM_TIMEWAIT instead of cm_id_priv->id.state

Jason
Leon Romanovsky April 6, 2020, 6:38 p.m. UTC | #2 
On Mon, Apr 06, 2020 at 02:45:56PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 06, 2020 at 08:32:42PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > The cm_reset_to_idle() call before formatting event changed the CM_ID
> > state from IB_CM_REQ_RCVD to be IB_CM_IDLE. It caused to wrong value
> > of CM_REJ_MESSAGE_REJECTED field.
> >
> > The result of that was that rdma_reject() calls in the passive side
> > didn't generate RDMA_CM_EVENT_REJECTED event in the active side.
> >
> > Fixes: 81ddb41f876d ("RDMA/cm: Allow ib_send_cm_rej() to be done under lock")
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/cm.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index bbbfa77dbce7..06f8eeba423a 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -1843,11 +1843,9 @@ static void cm_format_mra(struct cm_mra_msg *mra_msg,
> >
> >  static void cm_format_rej(struct cm_rej_msg *rej_msg,
> >  			  struct cm_id_private *cm_id_priv,
> > -			  enum ib_cm_rej_reason reason,
> > -			  void *ari,
> > -			  u8 ari_length,
> > -			  const void *private_data,
> > -			  u8 private_data_len)
> > +			  enum ib_cm_rej_reason reason, void *ari,
> > +			  u8 ari_length, const void *private_data,
> > +			  u8 private_data_len, enum ib_cm_state state)
> >  {
> >  	lockdep_assert_held(&cm_id_priv->lock);
> >
> > @@ -1855,7 +1853,7 @@ static void cm_format_rej(struct cm_rej_msg *rej_msg,
> >  	IBA_SET(CM_REJ_REMOTE_COMM_ID, rej_msg,
> >  		be32_to_cpu(cm_id_priv->id.remote_id));
> >
> > -	switch(cm_id_priv->id.state) {
> > +	switch (state) {
> >  	case IB_CM_REQ_RCVD:
> >  		IBA_SET(CM_REJ_LOCAL_COMM_ID, rej_msg, be32_to_cpu(0));
> >  		IBA_SET(CM_REJ_MESSAGE_REJECTED, rej_msg, CM_MSG_RESPONSE_REQ);
> > @@ -1920,8 +1918,9 @@ static void cm_dup_req_handler(struct cm_work *work,
> >  			      cm_id_priv->private_data_len);
> >  		break;
> >  	case IB_CM_TIMEWAIT:
> > -		cm_format_rej((struct cm_rej_msg *) msg->mad, cm_id_priv,
> > -			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0);
> > +		cm_format_rej((struct cm_rej_msg *)msg->mad, cm_id_priv,
> > +			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0,
> > +			      cm_id_priv->id.state);
>
> This can just be IB_CM_TIMEWAIT instead of cm_id_priv->id.state

It can, do you want me to resend it? Or maybe you can change it while
applying?

Thanks

>
> Jason
Jason Gunthorpe April 14, 2020, 6:51 p.m. UTC | #3 
On Mon, Apr 06, 2020 at 08:32:42PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> The cm_reset_to_idle() call before formatting event changed the CM_ID
> state from IB_CM_REQ_RCVD to be IB_CM_IDLE. It caused to wrong value
> of CM_REJ_MESSAGE_REJECTED field.
> 
> The result of that was that rdma_reject() calls in the passive side
> didn't generate RDMA_CM_EVENT_REJECTED event in the active side.
> 
> Fixes: 81ddb41f876d ("RDMA/cm: Allow ib_send_cm_rej() to be done under lock")
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> --
> 2.25.1

Applied to for-rc, thanks

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index bbbfa77dbce7..06f8eeba423a 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -1843,11 +1843,9 @@  static void cm_format_mra(struct cm_mra_msg *mra_msg,

 static void cm_format_rej(struct cm_rej_msg *rej_msg,
 			  struct cm_id_private *cm_id_priv,
-			  enum ib_cm_rej_reason reason,
-			  void *ari,
-			  u8 ari_length,
-			  const void *private_data,
-			  u8 private_data_len)
+			  enum ib_cm_rej_reason reason, void *ari,
+			  u8 ari_length, const void *private_data,
+			  u8 private_data_len, enum ib_cm_state state)
 {
 	lockdep_assert_held(&cm_id_priv->lock);

@@ -1855,7 +1853,7 @@  static void cm_format_rej(struct cm_rej_msg *rej_msg,
 	IBA_SET(CM_REJ_REMOTE_COMM_ID, rej_msg,
 		be32_to_cpu(cm_id_priv->id.remote_id));

-	switch(cm_id_priv->id.state) {
+	switch (state) {
 	case IB_CM_REQ_RCVD:
 		IBA_SET(CM_REJ_LOCAL_COMM_ID, rej_msg, be32_to_cpu(0));
 		IBA_SET(CM_REJ_MESSAGE_REJECTED, rej_msg, CM_MSG_RESPONSE_REQ);
@@ -1920,8 +1918,9 @@  static void cm_dup_req_handler(struct cm_work *work,
 			      cm_id_priv->private_data_len);
 		break;
 	case IB_CM_TIMEWAIT:
-		cm_format_rej((struct cm_rej_msg *) msg->mad, cm_id_priv,
-			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0);
+		cm_format_rej((struct cm_rej_msg *)msg->mad, cm_id_priv,
+			      IB_CM_REJ_STALE_CONN, NULL, 0, NULL, 0,
+			      cm_id_priv->id.state);
 		break;
 	default:
 		goto unlock;
@@ -2931,6 +2930,7 @@  static int cm_send_rej_locked(struct cm_id_private *cm_id_priv,
 			      u8 ari_length, const void *private_data,
 			      u8 private_data_len)
 {
+	enum ib_cm_state state = cm_id_priv->id.state;
 	struct ib_mad_send_buf *msg;
 	int ret;

@@ -2940,7 +2940,7 @@  static int cm_send_rej_locked(struct cm_id_private *cm_id_priv,
 	    (ari && ari_length > IB_CM_REJ_ARI_LENGTH))
 		return -EINVAL;

-	switch (cm_id_priv->id.state) {
+	switch (state) {
 	case IB_CM_REQ_SENT:
 	case IB_CM_MRA_REQ_RCVD:
 	case IB_CM_REQ_RCVD:
@@ -2952,7 +2952,8 @@  static int cm_send_rej_locked(struct cm_id_private *cm_id_priv,
 		if (ret)
 			return ret;
 		cm_format_rej((struct cm_rej_msg *)msg->mad, cm_id_priv, reason,
-			      ari, ari_length, private_data, private_data_len);
+			      ari, ari_length, private_data, private_data_len,
+			      state);
 		break;
 	case IB_CM_REP_SENT:
 	case IB_CM_MRA_REP_RCVD:
@@ -2961,7 +2962,8 @@  static int cm_send_rej_locked(struct cm_id_private *cm_id_priv,
 		if (ret)
 			return ret;
 		cm_format_rej((struct cm_rej_msg *)msg->mad, cm_id_priv, reason,
-			      ari, ari_length, private_data, private_data_len);
+			      ari, ari_length, private_data, private_data_len,
+			      state);
 		break;
 	default:
 		pr_debug("%s: local_id %d, cm_id->state: %d\n", __func__,