[openib-general] [Fwd: [PATCH] RDMA/iwcm: Fix memory leak]

[Tom Tucker]

Krishna: 

Maybe I'm missing something, but whether

if (len)
    kfree(ptr)

or

if (ptr)
    kfree(ptr)

is correct is contingent upon how you couple the two variables. But I don't'
think this has anything to do with the Roland's point.

I think "Pandora's box" was opened when Steve suggested that it's just good
policy to check for nul before calling free...and in general it is good
defensive programming.

However, Roland's point is that in the kernel, it's contingent upon us all
to know and leverage the error checking done by the services we use. If
kfree checks for nul, we don't have to....and shouldn't check it.

Kittens are cute... really ... who can argue with that? What 'len' allows us
to assume about 'ptr' is a little more ... well... fuzzy.


On 11/9/06 11:11 PM, "Krishna Kumar2" <krkumar2 at in.ibm.com> wrote:

> That is valid only if the drivers also comply. Eg if driver has two
> stack variables private_data and private_data_len, and it sets
> only private_data_len to zero. Then when calling the upper layer,
> it sets the event->private_data to its local private_data (uninitialized)
> and event->private_data_len to its local private_data_len (zero).
> Here we have to check the private_data_len before touching
> private_data or risk bug/panic.
> 
> thanks,
> 
> - KK
> 
> Tom Tucker <tom at opengridcomputing.com> wrote on 11/10/2006 10:20:18 AM:
> 
>> 
>> If it's truly nul or a ptr, we don't need to (and shouldn't) check, just
>> call kfree. If it's unitialized, we can't tell anyway and it's a bug --
>> right? 
>> 
>> Am I missing something?
>> 
>> On 11/9/06 10:41 PM, "Krishna Kumar2" <krkumar2 at in.ibm.com> wrote:
>> 
>>> Though the amso driver (c2_ae_event) is setting the private_data and
>>> private_data_len together for connect request and connect result, so
>>> the check may not be necessary. But if the semantics prefer checking
>>> to make sure, we should follow that (esp if other future drivers may
>>> also simply set private_data_len to zero without modifying
>>> private_data).
>>> 
>>> I did it this way since cm_conn_rep_handler() had the same check :)
>>> 
>>> thanks,
>>> 
>>> - KK
>>> 
>>>> I think the semantics are that the pointer is only used if
>>>> private_data_len > 0.  Otherwise, it is undefined.  So I think we
> should
>>>> keep the check.  Plus I don't like calling kfree() with a NULL
> pointer.
>>>> It just seems wrong...
>>>> 
>>>> ;-)
>>>> 
>>>> 
>>>> On Thu, 2006-11-09 at 14:59 -0800, Roland Dreier wrote:
>>>>>>>    if (iw_event->private_data_len)
>>>>>>>       kfree(iw_event->private_data);
>>>>>> 
>>>>>> Kfree checks for a null value, so is the private_data_len check
>>> necessary?
>>>>> 
>>>>> Could private_data be a junk pointer if private_data_len == 0 ?
>>>>> 
>>>>>  - R.
>>>> 
>>>> 
>>>> _______________________________________________
>>>> openib-general mailing list
>>>> openib-general at openib.org
>>>> http://openib.org/mailman/listinfo/openib-general
>>>> 
>>>> To unsubscribe, please visit
>>> http://openib.org/mailman/listinfo/openib-general
>>>> 
>>> 
>>> 
>>> _______________________________________________
>>> openib-general mailing list
>>> openib-general at openib.org
>>> http://openib.org/mailman/listinfo/openib-general
>>> 
>>> To unsubscribe, please visit
> http://openib.org/mailman/listinfo/openib-general
>>> 
>> 
>> 
>