On Tue, Mar 27, 2012 at 07:35:33PM -0700, Eric W. Biederman wrote:

Djalal Harouni [off-list ref] writes:

quoted

On Mon, Mar 26, 2012 at 03:50:30PM -0700, Eric W. Biederman wrote:

[...]

quoted

Anyway they seem false positive ones, since keeping a reference to
sysctl_header as in my previous (ugly) patch will quiet the last two
ones.

Ok thanks. If that is what it is.  Then clean way to quite this will
ultimately be converting these table to be compatible with my brand
new register_sysctl() and using that to register them.

In fact I am pretty certain we can just do:
register_sysctl("net/ipv4/route", ipv4_route_table);
register_sysctl("net/ipv4/neigh", empty);

Thanks, I've tested this and it works, however it seems that there are still
memleaks, please see below.

instead of:
register_sysctl_paths(ipv4_path, ipv4_skeleton);

And kill ipv4_skeleton and ipv4_path as they are now unused.

Right, and use the path directly from the .data section.

There was a tremendous cleanup and speed up that came with not allowing
sysctl tables to support .child entries in the core, and the older
registration routines break apart the tables and return a compatilibty
sysctl_table_header if we do that, and I believe we are just
leaking that compatibility sysctl_table_header.

Thanks for the explanation, however I don't understand all of this since
I'm not familiar with the old/new code, but I want to report some notes
(I'm not sure if they are correct).


1) memleaks:
After testing your proposed change:

register_sysctl("net/ipv4/route", ipv4_route_table);

for ipv4_route_table it is ok, no memleak is reported since the returned
ctl_table_header is referenced by the ctl_node nodes. This will silence
the memleak report.

for:
register_sysctl("net/ipv4/neigh", empty);

Here we'll still have the memleak, I don't know if this is the "leaking
that compatibility sysctl_table_header" you are referring to. But here the
returned ctl_table_header will not be referenced by the ctl_node nodes
since we have an empty ctl_table. see init_header()


So is there real memleaks that got hidden by the false positive ones ?
or perhaps these are real memleaks that were never spotted ?


2) From the __register_sysctl_table(), if the ctl_table is empty then
nr_entries == 0.

later we have:
header = kzalloc(sizeof(struct ctl_table_header) +
                 sizeof(struct ctl_node)*nr_entries, GFP_KERNEL);
...
node = (struct ctl_node *)(header + 1);
init_header(header, root, set, node, table);
...

node points to somewhere, but its use in this situations seems restricted.


3) Why we always alloc ctl_table_header in __register_sysctl_table() at
the beginning ? if I'm right, the current design is:
"each directory should have its ctl_table_header", so if some ctl_tables
share the same directory then they should also share the same
ctl_table_header ?

In this case the code can be improved, check if it's a new dir, then alloc
a new ctl_table_header, otherwise just return the old one and increment
the counter and insert ctl_table entries there.

I say this since currently doing:
register_sysctl("net/ipv4/neigh", empty);
register_sysctl("net/ipv4/neigh", empty);
register_sysctl("net/ipv4/neigh", empty);

Will allocate three ctl_table_header however at the second call the code
successfully detects that this is _not_ a new dir, but it will return the
ctl_table_header that was allocated at the beginning of
__register_sysctl_table().


Is this also related to the "leaking that compatibility sysctl_table_header" ?


4) Is the order of sysctl locking inside insert_links() correct ?


If I'm missing something, please just point-me to an http-link and I'll try
to take a closer look when time permits, otherwise someone who knows this
should just take a look at it.


Sorry for the delay (just got some free time).

Thanks.

-- 
tixxdz
http://opendz.org