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

netdev-owner@xxxxxxxxxxxxxxx wrote:
On Thu, 2006-06-15 at 08:41 -0500, Steve Wise wrote:
On Wed, 2006-06-14 at 20:35 -0500, Bob Sharp wrote:

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

<snip>

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages