lkml.org 
[lkml]   [2006]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRE: [openib-general] [PATCH v2 1/7] AMSO1100 Low Level Driver.
From
Date
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...

Steve.


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

\
 
 \ /
  Last update: 2006-06-15 16:06    [W:0.032 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site