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

From: Caitlin Bestler
Date: Thu Jun 15 2006 - 17:47:10 EST


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/