Re: Possible memory leaks in proc_sysctl.c

[Eric W. Biederman]

Catalin Marinas <catalin.marinas@xxxxxxx> writes:

> On Wed, Apr 18, 2012 at 02:22:09PM +0100, Eric W. Biederman wrote:
>> Catalin Marinas <catalin.marinas@xxxxxxx> writes:
>> > Following your commit f728019bb (sysctl: register only tables of sysctl
>> > files), I get several kmemleak reports. They all seem to be header
>> > allocations with kzalloc() in __register_sysctl_table() and
>> > __register_sysctl_paths(). The patch isn't simple to quickly figure out
>> > what may be wrong.
>> 
>> Due to a change in the data structure places where we register the
>> sysctl permanently and ignore the result from the register_sysctl_...
>> family of functions now report this leak.
>
> But is the header (or subheader, basically any pointer inside the
> kmalloc'ed object) never referenced from anywhere? I'm just trying to
> understand why kmemleak reports it as it seems that the header object is
> inserted in a ctl_dir.

It is never reference from anywhere because we never free the structure.
The job of the header is to be the structure that tells us how to free
things.

I see a couple of things going on.
- For compatibility the header that is returned is a dummy that just
  points to the real headers.

- Even without the compatibility we can get the same symptom if
  we register an empty directory.

So simply saying kmemleak shut up this is deliberate in these few cases
where we don't intend to unregister the structure and have a deliberate
leak seems the clean and maintainable way to go.

>> majianpeng has done a good of getting kmemleak_not_leak annotations into
>> the net tree, and I have one of his patches pending to put into my
>> sysctl tree (see below).
>
> If the header is referenced from somewhere, we can tell kmemleak where
> it is referenced from and avoid the not_leak annotations. But I'm not
> familiar with this code to be sure.

Nope.  There honestly are no references.  We reference lower parts of
the structure be we don't have a back pointer in all cases.

If we were good citizens and kept a reference to the returned
sysctl_header so we could unregister sysctls when our module unloads
(as the api is designed to do) we wouldn't have these warnings.  As
it is we have just been getting lucky in the past.   So I think just
saying kmemleak shut up I know I am being bad is reasonable.

I can change how we are registering things and get rid of the code that
where we there are no references today.  But then someone might refactor
the code tomorrow and problems might show up again.  Shrug.  So saying
I mean to leak this things don't worry about it seems clean.

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