[openib-general] OpenSM causes kernel trap

[Sean Hefty]

>OK, I think I found it.  The problem was that ib_umad_write() wrote
>through packet->msg in a few places where it should have used
>packet->msg->mad, and therefore corrupted the address of the buffer.

Yep - that appears to be the issue.

I've attached another patch that includes your fixes, plus adds some
additional code cleanup.

Signed-off-by: Sean Hefty <sean.hefty at intel.com>


Index: user_mad.c
===================================================================
--- user_mad.c	(revision 3861)
+++ user_mad.c	(working copy)
@@ -99,7 +99,6 @@
 	struct ib_mad_send_buf *msg;
 	struct list_head   list;
 	int		   length;
-	DECLARE_PCI_UNMAP_ADDR(mapping)
 	struct ib_user_mad mad;
 };
 
@@ -138,24 +137,23 @@
 			 struct ib_mad_send_wc *send_wc)
 {
 	struct ib_umad_file *file = agent->context;
-	struct ib_umad_packet *timeout, *packet = send_wc->send_buf->context[0];
+	struct ib_umad_packet *timeout;
+	struct ib_umad_packet *packet = send_wc->send_buf->context[0];
 
 	ib_destroy_ah(packet->msg->ah);
 	ib_free_send_mad(packet->msg);
 
 	if (send_wc->status == IB_WC_RESP_TIMEOUT_ERR) {
-		timeout = kmalloc(sizeof *timeout + sizeof (struct ib_mad_hdr),
-				  GFP_KERNEL);
+		timeout = kmalloc(sizeof *timeout + IB_MGMT_MAD_HDR, GFP_KERNEL);
 		if (!timeout)
 			goto out;
 
-		memset(timeout, 0, sizeof *timeout + sizeof (struct ib_mad_hdr));
+		memset(timeout, 0, sizeof *timeout + IB_MGMT_MAD_HDR);
 
-		timeout->length = sizeof (struct ib_mad_hdr);
+		timeout->length = IB_MGMT_MAD_HDR;
 		timeout->mad.hdr.id = packet->mad.hdr.id;
 		timeout->mad.hdr.status = ETIMEDOUT;
-		memcpy(timeout->mad.data, packet->mad.data,
-		       sizeof (struct ib_mad_hdr));
+		memcpy(timeout->mad.data, packet->mad.data, IB_MGMT_MAD_HDR);
 
 		if (!queue_packet(file, agent, timeout))
 				return;
@@ -245,7 +243,7 @@
 		else
 			ret = -ENOSPC;
 	} else if (copy_to_user(buf, &packet->mad,
-			      packet->length + sizeof (struct ib_user_mad)))
+				packet->length + sizeof (struct ib_user_mad)))
 		ret = -EFAULT;
 	else
 		ret = packet->length + sizeof (struct ib_user_mad);
@@ -270,22 +268,19 @@
 	struct ib_rmpp_mad *rmpp_mad;
 	u8 method;
 	__be64 *tid;
-	int ret, length, hdr_len, rmpp_hdr_size;
+	int ret, length, hdr_len, copy_offset;
 	int rmpp_active = 0;
 
 	if (count < sizeof (struct ib_user_mad))
 		return -EINVAL;
 
 	length = count - sizeof (struct ib_user_mad);
-	packet = kmalloc(sizeof *packet + sizeof(struct ib_mad_hdr) +
-			 sizeof (struct ib_rmpp_hdr), GFP_KERNEL);
+	packet = kmalloc(sizeof *packet + IB_MGMT_RMPP_HDR, GFP_KERNEL);
 	if (!packet)
 		return -ENOMEM;
 
 	if (copy_from_user(&packet->mad, buf,
-			    sizeof (struct ib_user_mad) +
-			    sizeof (struct ib_mad_hdr) +
-			    sizeof (struct ib_rmpp_hdr))) {
+			    sizeof (struct ib_user_mad) + IB_MGMT_RMPP_HDR)) {
 		ret = -EFAULT;
 		goto err;
 	}
@@ -296,8 +291,6 @@
 		goto err;
 	}
 
-	packet->length = length;
-
 	down_read(&file->agent_mutex);
 
 	agent = file->agent[packet->mad.hdr.id];
@@ -344,12 +337,10 @@
 			goto err_ah;
 		}
 		rmpp_active = 1;
+		copy_offset = IB_MGMT_RMPP_HDR;
 	} else {
-		if (length > sizeof (struct ib_mad)) {
-			ret = -EINVAL;
-			goto err_ah;
-		}
 		hdr_len = IB_MGMT_MAD_HDR;
+		copy_offset = IB_MGMT_MAD_HDR;
 	}
 
 	packet->msg = ib_create_send_mad(agent,
@@ -363,32 +354,18 @@
 	}
 
 	packet->msg->ah = ah;
-	packet->msg->timeout_ms  = packet->mad.hdr.timeout_ms;
+	packet->msg->timeout_ms = packet->mad.hdr.timeout_ms;
 	packet->msg->retries = packet->mad.hdr.retries;
 	packet->msg->context[0] = packet;
 
-	if (!rmpp_active) {
-		/* Copy message from user into send buffer */
-		if (copy_from_user(packet->msg->mad,
-				   buf + sizeof (struct ib_user_mad), length)) {
-			ret = -EFAULT;
-			goto err_msg;
-		}
-	} else {
-		rmpp_hdr_size = sizeof (struct ib_mad_hdr) +
-				sizeof (struct ib_rmpp_hdr);
-
-		/* Only copy MAD headers (RMPP header in place) */
-		memcpy(packet->msg->mad, packet->mad.data,
-		       sizeof (struct ib_mad_hdr));
-
-		/* Now, copy rest of message from user into send buffer */
-		if (copy_from_user(((struct ib_rmpp_mad *) packet->msg->mad)->data,
-				   buf + sizeof (struct ib_user_mad) + rmpp_hdr_size,
-				   length - rmpp_hdr_size)) {
-			ret = -EFAULT;
-			goto err_msg;
-		}
+	/* Copy MAD headers (RMPP header in place) */
+	memcpy(packet->msg->mad, packet->mad.data, IB_MGMT_MAD_HDR);
+	/* Now, copy rest of message from user into send buffer */
+	if (copy_from_user(packet->msg->mad + copy_offset,
+			   buf + sizeof (struct ib_user_mad) + copy_offset,
+			   length - copy_offset)) {
+		ret = -EFAULT;
+		goto err_msg;
 	}
 
 	/*
@@ -397,12 +374,12 @@
 	 * transaction ID matches the agent being used to send the
 	 * MAD.
 	 */
-	method = ((struct ib_mad_hdr *) packet->msg)->method;
+	method = ((struct ib_mad_hdr *) packet->msg->mad)->method;
 
 	if (!(method & IB_MGMT_METHOD_RESP)       &&
 	    method != IB_MGMT_METHOD_TRAP_REPRESS &&
 	    method != IB_MGMT_METHOD_SEND) {
-		tid = &((struct ib_mad_hdr *) packet->msg)->tid;
+		tid = &((struct ib_mad_hdr *) packet->msg->mad)->tid;
 		*tid = cpu_to_be64(((u64) agent->hi_tid) << 32 |
 				   (be64_to_cpup(tid) & 0xffffffff));
 	}
@@ -413,17 +390,14 @@
 
 	up_read(&file->agent_mutex);
 
-	return sizeof (struct ib_user_mad_hdr) + packet->length;
+	return count;
 
 err_msg:
 	ib_free_send_mad(packet->msg);
-
 err_ah:
 	ib_destroy_ah(ah);
-
 err_up:
 	up_read(&file->agent_mutex);
-
 err:
 	kfree(packet);
 	return ret;