netdev - Re: Warning when unloading the nf_conntack module (regression?)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Pine.LNX.4.64.0808042130570.23688@bizon.gios.gov.pl>
Date:	Mon, 4 Aug 2008 21:41:10 +0200 (CEST)
From:	Krzysztof Oledzki <ole@....pl>
To:	Arjan van de Ven <arjan@...radead.org>
cc:	netdev@...r.kernel.org, kaber@...sh.net,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: Warning when unloading the nf_conntack module (regression?)



On Mon, 4 Aug 2008, Krzysztof Oledzki wrote:

>
>
> On Mon, 4 Aug 2008, Krzysztof Oledzki wrote:
>
>> 
>> 
>> On Sun, 3 Aug 2008, Krzysztof Oledzki wrote:
>> 
>>> 
>>> 
>>> On Sun, 3 Aug 2008, Arjan van de Ven wrote:
>>> 
>>>> The warning below started showing up on kerneloops.org in the top 20 and 
>>>> it appears to
>>>> be new in 2.6.27-rc (e.g. a regression)...
>>>> 
>>>> It happens when nf_conntrack is rmmod'd
>>>> 
>>>> 
>>>> The reports:
>>>> http://www.kerneloops.org/search.php?search=nf_conntrack_acct_fini
>>>> 
>>>> The warning:
>>>> 
>>>> WARNING: at kernel/sysctl.c:1966 unregister_sysctl_table+0xcc/0x103()
>>>> 
>>>> Modules : nf_conntrack(-)
>>>> 
>>>> Call Trace:
>>>> [<ffffffff81043bc8>] warn_on_slowpath+0x65/0x98
>>>> [<ffffffff8104abdf>] unregister_sysctl_table+0xcc/0x103
>>>> [<ffffffffa0306655>] nf_conntrack_acct_fini+0x15/0x23 [nf_conntrack]
>>>> [<ffffffffa03018a1>] nf_conntrack_cleanup+0x84/0x86 [nf_conntrack]
>>>> [<ffffffffa0306944>] nf_conntrack_standalone_fini+0x40/0x42 
>>>> [nf_conntrack]
>>>> [<ffffffff810700d0>] sys_delete_module+0x202/0x263
>>>> [<ffffffff8101034a>] system_call_fastpath+0x16/0x1b
>>> 
>>> Thanks. It seems I'm the person who introduced it. I'll look at it ASAP.
>> 
>> Probably spoken too fast. This problem was introduced in 2.6.26-git15, 
>> about one week after my accounting rework had been included. Obviously 
>> there is something wrong with netfilter sysctl handling, as starting 
>> with this kernel version sysctl reports duplicated net.netfilter:
>> 
>> # find /proc/sys/net/|grep net/netf
>> /proc/sys/net/netfilter
>> /proc/sys/net/netfilter/nf_conntrack_generic_timeout
>> /proc/sys/net/netfilter/nf_conntrack_acct
>> /proc/sys/net/netfilter
>> /proc/sys/net/netfilter/nf_conntrack_generic_timeout
>> /proc/sys/net/netfilter/nf_conntrack_acct
>> 
>> # sysctl -a|grep net.netfilter
>> net.netfilter.nf_conntrack_generic_timeout = 600
>> net.netfilter.nf_conntrack_acct = 1
>> net.netfilter.nf_conntrack_generic_timeout = 600
>> net.netfilter.nf_conntrack_acct = 1
>> 
>> Still investigating.
>
> BTW: It also happens when I revert my patch:
>
> sysctl -a|grep net.netfilter
>
> net.netfilter.nf_conntrack_generic_timeout = 600
> net.netfilter.nf_conntrack_generic_timeout = 600

And the winner is... 9043476f726802f4b00c96d0c4f418dde48d1304:

[PATCH] sanitize proc_sysctl

* keep references to ctl_table_head and ctl_table in /proc/sys inodes
* grab the former during operations, use the latter for access to
   entry if that succeeds
* have ->d_compare() check if table should be seen for one who does lookup;
   that allows us to avoid flipping inodes - if we have the same name resolve
   to different things, we'll just keep several dentries and ->d_compare()
   will reject the wrong ones.
* have ->lookup() and ->readdir() scan the table of our inode first, then
   walk all ctl_table_header and scan ->attached_by for those that are
   attached to our directory.
* implement ->getattr().
* get rid of insane amounts of tree-walking
* get rid of the need to know dentry in ->permission() and of the contortions
   induced by that.

Signed-off-by: Al Viro <viro@...iv.linux.org.uk>

With this patch "sysctl -a|grep net.netfilter" shows only 
net.netfilter.nf_conntrack_generic_timeout and 
net.netfilter.nf_conntrack_acct, both are duplicate btw:

# sysctl -a 2>/dev/null|grep netf
net.ipv4.netfilter.ip_conntrack_generic_timeout = 600
net.netfilter.nf_conntrack_generic_timeout = 600
net.netfilter.nf_conntrack_acct = 1
net.netfilter.nf_conntrack_generic_timeout = 600
net.netfilter.nf_conntrack_acct = 1

Without that commit I get full sysctl tree:

# sysctl -a 2>/dev/null|grep netf
net.ipv4.netfilter.ip_conntrack_generic_timeout = 600
net.netfilter.nf_conntrack_generic_timeout = 600
net.netfilter.nf_conntrack_acct = 1
net.netfilter.nf_conntrack_max = 32768
net.netfilter.nf_conntrack_count = 0
net.netfilter.nf_conntrack_buckets = 8192
net.netfilter.nf_conntrack_checksum = 1
net.netfilter.nf_conntrack_log_invalid = 0
net.netfilter.nf_conntrack_expect_max = 128

And of course no WARNING at unloading as it comes from that patch 
directly:

-       for (i = 1; table && (i <= depth); i++) {
-               ancestor = proc_sys_ancestor(dentry, i);
-               table = proc_sys_lookup_table_one(table, &ancestor->d_name);
-               if (table)
-                       table = table->child;
+       if (table && !table->child) {
+               WARN_ON(1);
+               goto out;
         }

OK, how we should proceed next? Is sysctl API misused somewhere in the 
netfilter code and/or in my 584015727a3b88b46602b20077b46cd04f8b4ab3 
patch? Or maybe 9043476f726802f4b00c96d0c4f418dde48d1304 commit is buggy?

Best regards,

 				Krzysztof Olędzki