nat/conntrack helper locking problem?

nat/conntrack helper locking problem?

[Henrik Nordstrom]

Whenever I load/unload the ftp or irc nat/conntrack helpers with NF_DEBUG
enabled in 2.4.22 with current p-o-m base I receive the following warning:

ASSERT ip_conntrack_core.c:632 &ip_conntrack_lock not readlocked
ASSERT ip_conntrack_core.c:632 &ip_conntrack_lock not readlocked

Is this something to worry about or just a corner case which will not 
cause any problems?

Regards
Henrik

Re: nat/conntrack helper locking problem?

[Patrick McHardy]

Henrik Nordstrom wrote:

>Whenever I load/unload the ftp or irc nat/conntrack helpers with NF_DEBUG
>enabled in 2.4.22 with current p-o-m base I receive the following warning:
>
>ASSERT ip_conntrack_core.c:632 &ip_conntrack_lock not readlocked
>ASSERT ip_conntrack_core.c:632 &ip_conntrack_lock not readlocked
>
>Is this something to worry about or just a corner case which will not 
>cause any problems?
>  
>

I couldn't find the exact line in either 2.4 or 2.6, could you post it ?

Thanks,
Patrick

Re: nat/conntrack helper locking problem?

[Henrik Nordstrom]

On Thu, 20 Nov 2003, Patrick McHardy wrote:

> I couldn't find the exact line in either 2.4 or 2.6, could you post it ?

It would be this

>ASSERT ip_conntrack_core.c:632 &ip_conntrack_lock not readlocked

struct ip_conntrack_helper *ip_ct_find_helper(const struct ip_conntrack_tuple *tuple)
{
        return LIST_FIND(&helpers, helper_cmp,
                         struct ip_conntrack_helper *,
                         tuple);
}


which makes sense as LIST_FIND uses ASSERT_READ_LOCK.

#define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)

The warning have been isolated to the nat helpers only. The conntrack 
helpers are silent.

The full trace is

#0  printk (fmt=0xa2812140 "ASSERT %s:%u %s not readlocked\n") at 
printk.c:416
#1  0xa280efec in ip_ct_find_helper (tuple=0xa282aab4)
    at ip_conntrack_core.c:630
#2  0xa281dd24 in ip_nat_helper_register (me=0xa282aaa0) at 
ip_nat_helper.c:474
#3  0xa282a724 in init () at ip_nat_ftp.c:327
#4  0xa0010eb3 in sys_init_module (name_user=0x807cfe8 "ip_nat_ftp", 
    mod_user=0x80a7578) at module.c:563

this is not 100% the exact same kernel so the line number differs 
slightly, but it is the exact same error.

Regards
Henrik

Re: nat/conntrack helper locking problem?

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

It's a relative uncritical error, ip_nat_helper_register races with
ip_conntrack_helper_unregister. The attached patch should
fix it.

Best regards,
Patrick

Henrik Nordstrom wrote:

>The warning have been isolated to the nat helpers only. The conntrack 
>helpers are silent.
>
>The full trace is
>
>#0  printk (fmt=0xa2812140 "ASSERT %s:%u %s not readlocked\n") at 
>printk.c:416
>#1  0xa280efec in ip_ct_find_helper (tuple=0xa282aab4)
>    at ip_conntrack_core.c:630
>#2  0xa281dd24 in ip_nat_helper_register (me=0xa282aaa0) at 
>ip_nat_helper.c:474
>#3  0xa282a724 in init () at ip_nat_ftp.c:327
>#4  0xa0010eb3 in sys_init_module (name_user=0x807cfe8 "ip_nat_ftp", 
>    mod_user=0x80a7578) at module.c:563
>
>this is not 100% the exact same kernel so the line number differs 
>slightly, but it is the exact same error.
>
>Regards
>Henrik
>  
>


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 684 bytes --]

===== net/ipv4/netfilter/ip_nat_helper.c 1.11 vs edited =====
--- 1.11/net/ipv4/netfilter/ip_nat_helper.c	Mon Oct 13 21:40:48 2003
+++ edited/net/ipv4/netfilter/ip_nat_helper.c	Thu Nov 20 12:48:17 2003
@@ -469,10 +469,13 @@
 	int ret = 0;
 
 	if (me->me && !(me->flags & IP_NAT_HELPER_F_STANDALONE)) {
-		struct ip_conntrack_helper *ct_helper;
+		struct ip_conntrack_helper *ct_helper = NULL;
 		
-		if ((ct_helper = ip_ct_find_helper(&me->tuple))
-		    && ct_helper->me) {
+		READ_LOCK(&ip_conntrack_lock);
+		ct_helper = ip_ct_find_helper(&me->tuple);
+		READ_UNLOCK(&ip_conntrack_lock);
+
+		if (ct_helper && ct_helper->me) {
 			__MOD_INC_USE_COUNT(ct_helper->me);
 		} else {
 

Re: nat/conntrack helper locking problem?

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

Sorry Harald the last patch added an unneccessary NULL assignment,
this patch is better. It fixes ip_nat_helper_register calling 
ip_ct_find_helper
without holding ip_conntrack_lock.

Best regards,
Patrick

Patrick McHardy wrote:

> It's a relative uncritical error, ip_nat_helper_register races with
> ip_conntrack_helper_unregister. The attached patch should
> fix it.
>


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 617 bytes --]

===== net/ipv4/netfilter/ip_nat_helper.c 1.11 vs edited =====
--- 1.11/net/ipv4/netfilter/ip_nat_helper.c	Mon Oct 13 21:40:48 2003
+++ edited/net/ipv4/netfilter/ip_nat_helper.c	Thu Nov 20 12:57:22 2003
@@ -471,8 +471,11 @@
 	if (me->me && !(me->flags & IP_NAT_HELPER_F_STANDALONE)) {
 		struct ip_conntrack_helper *ct_helper;
 		
-		if ((ct_helper = ip_ct_find_helper(&me->tuple))
-		    && ct_helper->me) {
+		READ_LOCK(&ip_conntrack_lock);
+		ct_helper = ip_ct_find_helper(&me->tuple);
+		READ_UNLOCK(&ip_conntrack_lock);
+
+		if (ct_helper && ct_helper->me) {
 			__MOD_INC_USE_COUNT(ct_helper->me);
 		} else {
 

Re: nat/conntrack helper locking problem?

[Henrik Nordstrom]

On Thu, 20 Nov 2003, Patrick McHardy wrote:

> It's a relative uncritical error, ip_nat_helper_register races with
> ip_conntrack_helper_unregister. The attached patch should
> fix it.

The same change needs to be done in both functions I presume.. the warning 
is given on both register and unregister.

I will give it a try. 

Hmm.. to be correct doesn't the lock need to be held until the module
counter have been increased in ip_nat_helper_register? And does this lock
really solve the race in the first place? What prevents
ip_conntrack_helper_unregister from unregistering the conntrack helper
when the lock is released? The module use count check has already been
long passed at that time..

But as you say it is a relatively uncritical error. The chance for 
concurrent ip_conntrack_helper_unregister and ip_nat_helper_register is 
close to nil (of not non-existent)

Regards
Henrik

Re: nat/conntrack helper locking problem?

[Patrick McHardy]

Henrik Nordstrom wrote:

>On Thu, 20 Nov 2003, Patrick McHardy wrote:
>
>  
>
>>It's a relative uncritical error, ip_nat_helper_register races with
>>ip_conntrack_helper_unregister. The attached patch should
>>fix it.
>>    
>>
>
>The same change needs to be done in both functions I presume.. the warning 
>is given on both register and unregister.
>
>I will give it a try. 
>
>Hmm.. to be correct doesn't the lock need to be held until the module
>counter have been increased in ip_nat_helper_register? And does this lock
>really solve the race in the first place? What prevents
>ip_conntrack_helper_unregister from unregistering the conntrack helper
>when the lock is released? The module use count check has already been
>long passed at that time..
>
>But as you say it is a relatively uncritical error. The chance for 
>concurrent ip_conntrack_helper_unregister and ip_nat_helper_register is 
>close to nil (of not non-existent)
>  
>

Your right the fix is incorrect, I should have taken more time for it.
Will do so right away ..

Patrick

>Regards
>Henrik
>  
>

Re: nat/conntrack helper locking problem?

[Henrik Nordstrom]

On Thu, 20 Nov 2003, Patrick McHardy wrote:

> Your right the fix is incorrect, I should have taken more time for it.
> Will do so right away ..

Also it did not compile... need to #include 
<linux/netfilter_ipv4/ip_conntrack_core.h> to have ip_conntrack_lock 
defined.

Anyway, knowing what the warning is I am not worried about it. The race 
window in question can not ever happen in our use as the modules are 
explicitly loaded by modprobe one at a time and never removed.

Regards
Henrik