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

netdev-owner@vger.kernel.org wrote:

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

quoted

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

quoted

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...

Having the IWCM copy the pdata during the upcall also leaves
the greatest flexibility for the driver on how/where the pdata
is captured. The IWCM has to deal with user-mode, indefinite
delays waiting for a response and user-mode processes that die
while holding a connection request. So it makes sense for that
layer to do the allocating and copying.

quoted

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...

Having the IWCM copy the pdata during the upcall also leaves
the greatest flexibility for the driver on how/where the pdata
is captured. The IWCM has to deal with user-mode, indefinite
delays waiting for a response and user-mode processes that die
while holding a connection request. So it makes sense for that
layer to do the allocating and copying.

I've already coded and test this.  The IWCM will copy the pdata...

Steve.