RE: [openib-general] [PATCH v2 1/7] AMSO1100 Low Level Driver.

On Thu, 2006-06-15 at 08:41 -0500, Steve Wise wrote:

On Wed, 2006-06-14 at 20:35 -0500, Bob Sharp wrote:

quoted

quoted

+void c2_ae_event(struct c2_dev *c2dev, u32 mq_index)
+{
+

<snip>

quoted

quoted

+	case C2_RES_IND_EP:{
+
+		struct c2wr_ae_connection_request *req =
+			&wr->ae.ae_connection_request;
+		struct iw_cm_id *cm_id =
+			(struct iw_cm_id *)resource_user_context;
+
+		pr_debug("C2_RES_IND_EP event_id=%d\n", event_id);
+		if (event_id != CCAE_CONNECTION_REQUEST) {
+			pr_debug("%s: Invalid event_id: %d\n",
+				__FUNCTION__, event_id);
+			break;
+		}
+		cm_event.event = IW_CM_EVENT_CONNECT_REQUEST;
+		cm_event.provider_data = (void*)(unsigned

long)req->cr_handle;

quoted

+		cm_event.local_addr.sin_addr.s_addr = req->laddr;
+		cm_event.remote_addr.sin_addr.s_addr = req->raddr;
+		cm_event.local_addr.sin_port = req->lport;
+		cm_event.remote_addr.sin_port = req->rport;
+		cm_event.private_data_len =
+			be32_to_cpu(req->private_data_length);
+
+		if (cm_event.private_data_len) {

It looks to me as if pdata is leaking here since it is not tracked and
the upper layers do not free it.  Also, if pdata is freed after the call
to cm_id->event_handler returns, it exposes an issue in user space where
the private data is garbage.  I suspect the iwarp cm should be copying
this data before it returns.

Good catch.  

Yes, I think the IWCM should copy the private data in the upcall.  If it
does, then the amso driver doesn't need to kmalloc()/copy at all.  It
can pass a ptr to its MQ entry directly...

Now that I've looked more into this, I'm not sure there's a simple way
for the IWCM to copy the pdata on the upcall.  Currently, the IWCM's
event upcall, cm_event_handler(), simply queues the work for processing
on a workqueue thread.  So there's no per-event logic at all there.
Lemme think on this more.  Stay tuned.  

Either way, the amso driver has a memory leak...

Steve.