Re: [PATCH 2.6.17-rc6 7/9] Remove some of the kmemleak false positives

From: Ingo Molnar
Date: Mon Jun 12 2006 - 07:40:28 EST



* Pekka J Enberg <penberg@xxxxxxxxxxxxxx> wrote:

> On Mon, 12 Jun 2006, Ingo Molnar wrote:
> > While it's always good to reduce the amount of false positives, i dont
> > think it's unacceptable for inclusion right now. A few dozen annotations
> > out of 7000+ allocation call sites isnt a big maintainance problem - and
> > the benefits of automatic leak-checking are really huge.
>
> Did you look at the call sites? It seems clear that kmemleak doesn't
> support existing kernel coding style yet (see below) which means we're
> not covering all false positives.

but "supporting existing kernel coding style as-is" is not a must-have
criterium for inclusion. While preserving semantics is strongly
encouraged of course, a patch can change semantics (or can introduce
restrictions) as long as it's common-sense or there is no other way out.
The question is benefit vs. disadvantage, not a rigid "does it change
semantics" rule.

Just look at Sparse annotations: they add maintainance overhead, but the
overhead is well worth the trouble: they avoid bugs and they also serve
as documentation/specification. And Sparse annotations are alot more
numerous than kmemleak annotations will ever be!

> On Mon, 12 Jun 2006, Ingo Molnar wrote:
> > What i'd like to see though are clear explanations about why an
> > allocation is not considered a leak, in terms of comments added to the
> > code. That will also help us reduce the number of annotations later on.
>
> I found at least two unacceptable false positive classes:
>
> - arch/i386/kernel/setup.c:
> False positive because res pointer is stored in a global instance of
> struct resource.

there's no good way around this one but to annotate it in one way or
another.

We could express the non-leak nature of this allocation in a cleaner way
by introducing the following API:

memleak_boot_time_alloc(res);

Here kmemleak should build a global list of such allocations, with the
following rules: these allocations must not be freed after that point,
but these allocations will be searched for further pointers.

Via this method we will both document the special nature of these
allocations and we'll enforce that it's not freed. (not that there is a
high likelyhood of freeing a buffer whose pointer nobody knows - the
purpose is rather to make sure that the annotation is correct)

> - drivers/base/platform.c and fs/ext3/dir.c:
> False positive because we allocate memory for struct + some extra
> stuff.
>
> At least the latter can be fixed as outlined by Catalin in another
> mail.

yes.

my "document the exceptions" request was to make sure that such special
rules are not forgotten. As long as they are documented clearly, if
there's some common pattern between a number of them then they can be
moved into the kmemleak infrastructure, to further reduce the annotation
overhead.

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