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

[Krishna Kumar2]

Hi Tom,

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

That is correct. I stated somewhat differently by saying that the two 
variables
may not be initialized together, eg, if there is no data, the driver can 
initialize
the private_data_len to zero instead of setting that as well as 
private_data to
NULL (redundant). How it is done at the driver is not known at this time, 
unless
we specify how it should be done at the access layer, so that future 
drivers
have to be written to this standard (amso seems to do both and no check is
necessary). Eg a patch like :

out:
+       BUG_ON((private_data_len == 0 && private_data != NULL) ||
+                  (private_data_len && private_data == NULL)
        kfree(private_data);

can catch wrong driver implentations early on. Maybe that is something 
that can
be added ?

Thanks,

- KK

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