diff mbox series

[rdma-next,3/6] IB/cm: Remove "mad_agent" parameter of ib_modify_mad

Message ID 20210318100309.670344-4-leon@kernel.org (mailing list archive)
State Superseded
Delegated to: Jason Gunthorpe
Headers show
Series Fix memory corruption in CM | expand

Commit Message

Leon Romanovsky March 18, 2021, 10:03 a.m. UTC 
From: Mark Zhang <markzhang@nvidia.com>

The mad_agent parameter is redundant since the struct ib_mad_send_buf
already has a pointer of it.

Signed-off-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cm.c  | 29 +++++++++++++++++++++--------
 drivers/infiniband/core/mad.c | 10 ++++++----
 include/rdma/ib_mad.h         |  6 ++----
 3 files changed, 29 insertions(+), 16 deletions(-)

Comments

Jason Gunthorpe March 29, 2021, 12:41 p.m. UTC | #1 
On Thu, Mar 18, 2021 at 12:03:06PM +0200, Leon Romanovsky wrote:

> +static void cm_send_free_msg(struct ib_mad_send_buf *msg)
> +{
> +	struct cm_id_private *cm_id_priv;
> +
> +	cm_id_priv = msg->context[0];
> +	if (!cm_id_priv || cm_id_priv->msg != msg) {
> +		cm_free_msg(msg);
> +		return;
> +	}
> +
> +	spin_lock_irq(&cm_id_priv->lock);
> +	cm_free_msg(msg);
> +	cm_id_priv->msg = NULL;
> +	spin_unlock_irq(&cm_id_priv->lock);
> +}

Either the whole sequence should be inside the lock or nothing should
be in the lock..

Oh this is all messed up and needs a big fix. Review and include this
in the series and drop the above function.

https://github.com/jgunthorpe/linux/commits/for-markz

Jason
Leon Romanovsky April 1, 2021, 8:12 a.m. UTC | #2 
On Mon, Mar 29, 2021 at 09:41:01AM -0300, Jason Gunthorpe wrote:
> On Thu, Mar 18, 2021 at 12:03:06PM +0200, Leon Romanovsky wrote:
> 
> > +static void cm_send_free_msg(struct ib_mad_send_buf *msg)
> > +{
> > +	struct cm_id_private *cm_id_priv;
> > +
> > +	cm_id_priv = msg->context[0];
> > +	if (!cm_id_priv || cm_id_priv->msg != msg) {
> > +		cm_free_msg(msg);
> > +		return;
> > +	}
> > +
> > +	spin_lock_irq(&cm_id_priv->lock);
> > +	cm_free_msg(msg);
> > +	cm_id_priv->msg = NULL;
> > +	spin_unlock_irq(&cm_id_priv->lock);
> > +}
> 
> Either the whole sequence should be inside the lock or nothing should
> be in the lock..

I see the race now, thanks for pointing it.

> 
> Oh this is all messed up and needs a big fix. Review and include this
> in the series and drop the above function.
> 
> https://github.com/jgunthorpe/linux/commits/for-markz

We will take a look on it.

Thanks

> 
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 2cee5352c620..d481ebd281e1 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -3128,16 +3128,14 @@  static int cm_mra_handler(struct cm_work *work)
 	case IB_CM_REQ_SENT:
 		if (IBA_GET(CM_MRA_MESSAGE_MRAED, mra_msg) !=
 			    CM_MSG_RESPONSE_REQ ||
-		    ib_modify_mad(cm_id_priv->av.port->mad_agent,
-				  cm_id_priv->msg, timeout))
+		    ib_modify_mad(cm_id_priv->msg, timeout))
 			goto out;
 		cm_id_priv->id.state = IB_CM_MRA_REQ_RCVD;
 		break;
 	case IB_CM_REP_SENT:
 		if (IBA_GET(CM_MRA_MESSAGE_MRAED, mra_msg) !=
 			    CM_MSG_RESPONSE_REP ||
-		    ib_modify_mad(cm_id_priv->av.port->mad_agent,
-				  cm_id_priv->msg, timeout))
+		    ib_modify_mad(cm_id_priv->msg, timeout))
 			goto out;
 		cm_id_priv->id.state = IB_CM_MRA_REP_RCVD;
 		break;
@@ -3145,8 +3143,7 @@  static int cm_mra_handler(struct cm_work *work)
 		if (IBA_GET(CM_MRA_MESSAGE_MRAED, mra_msg) !=
 			    CM_MSG_RESPONSE_OTHER ||
 		    cm_id_priv->id.lap_state != IB_CM_LAP_SENT ||
-		    ib_modify_mad(cm_id_priv->av.port->mad_agent,
-				  cm_id_priv->msg, timeout)) {
+		    ib_modify_mad(cm_id_priv->msg, timeout)) {
 			if (cm_id_priv->id.lap_state == IB_CM_MRA_LAP_RCVD)
 				atomic_long_inc(&work->port->
 						counter_group[CM_RECV_DUPLICATES].
@@ -3737,6 +3734,22 @@  static void cm_process_send_error(struct ib_mad_send_buf *msg,
 	cm_free_msg(msg);
 }
 
+static void cm_send_free_msg(struct ib_mad_send_buf *msg)
+{
+	struct cm_id_private *cm_id_priv;
+
+	cm_id_priv = msg->context[0];
+	if (!cm_id_priv || cm_id_priv->msg != msg) {
+		cm_free_msg(msg);
+		return;
+	}
+
+	spin_lock_irq(&cm_id_priv->lock);
+	cm_free_msg(msg);
+	cm_id_priv->msg = NULL;
+	spin_unlock_irq(&cm_id_priv->lock);
+}
+
 static void cm_send_handler(struct ib_mad_agent *mad_agent,
 			    struct ib_mad_send_wc *mad_send_wc)
 {
@@ -3766,13 +3779,13 @@  static void cm_send_handler(struct ib_mad_agent *mad_agent,
 	switch (mad_send_wc->status) {
 	case IB_WC_SUCCESS:
 	case IB_WC_WR_FLUSH_ERR:
-		cm_free_msg(msg);
+		cm_send_free_msg(msg);
 		break;
 	default:
 		if (msg->context[0] && msg->context[1])
 			cm_process_send_error(msg, mad_send_wc->status);
 		else
-			cm_free_msg(msg);
+			cm_send_free_msg(msg);
 		break;
 	}
 }
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 31a97cf1ef81..e7ff4420777e 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -2464,16 +2464,18 @@  find_send_wr(struct ib_mad_agent_private *mad_agent_priv,
 	return NULL;
 }
 
-int ib_modify_mad(struct ib_mad_agent *mad_agent,
-		  struct ib_mad_send_buf *send_buf, u32 timeout_ms)
+int ib_modify_mad(struct ib_mad_send_buf *send_buf, u32 timeout_ms)
 {
 	struct ib_mad_agent_private *mad_agent_priv;
 	struct ib_mad_send_wr_private *mad_send_wr;
 	unsigned long flags;
 	int active;
 
-	mad_agent_priv = container_of(mad_agent, struct ib_mad_agent_private,
-				      agent);
+	if (!send_buf)
+		return -EINVAL;
+
+	mad_agent_priv = container_of(send_buf->mad_agent,
+				      struct ib_mad_agent_private, agent);
 	spin_lock_irqsave(&mad_agent_priv->lock, flags);
 	mad_send_wr = find_send_wr(mad_agent_priv, send_buf);
 	if (!mad_send_wr || mad_send_wr->status != IB_WC_SUCCESS) {
diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
index 69b824dc7820..465b0d0bdaf8 100644
--- a/include/rdma/ib_mad.h
+++ b/include/rdma/ib_mad.h
@@ -719,15 +719,13 @@  void ib_free_recv_mad(struct ib_mad_recv_wc *mad_recv_wc);
 
 /**
  * ib_modify_mad - Modifies an outstanding send MAD operation.
- * @mad_agent: Specifies the registration associated with sent MAD.
  * @send_buf: Indicates the MAD to modify.
  * @timeout_ms: New timeout value for sent MAD.
  *
  * This call will reset the timeout value for a sent MAD to the specified
  * value.
  */
-int ib_modify_mad(struct ib_mad_agent *mad_agent,
-		  struct ib_mad_send_buf *send_buf, u32 timeout_ms);
+int ib_modify_mad(struct ib_mad_send_buf *send_buf, u32 timeout_ms);
 
 /**
  * ib_cancel_mad - Cancels an outstanding send MAD operation.
@@ -738,7 +736,7 @@  int ib_modify_mad(struct ib_mad_agent *mad_agent,
  */
 static inline void ib_cancel_mad(struct ib_mad_send_buf *send_buf)
 {
-	ib_modify_mad(send_buf->mad_agent, send_buf, 0);
+	ib_modify_mad(send_buf, 0);
 }
 
 /**