kernel crash [problem with helpers]

kernel crash [problem with helpers]

[Raivis Bucis]

ip_conntrack assumes that expect->expectant->helper allways will be non null, 
but that is not allways the case.

When tftp conntrack is used, on first received UDP packet, tfp helper adds 
expectation. If it gets NATed later to some other port, 
ip_conntrack_alter_reply will set its helper to NULL, but expectation is 
still kept, and when expected conntrack is created, its master conntrack 
doesn't have helper anymore. This leads to kernel oops in list_conntracks 
when reading "/proc/net/ip_conntrack", etc.

Therefore I propose following fix:

diff -u -r1.13 ip_conntrack_core.c
--- ip_conntrack_core.c 9 Jan 2004 07:52:10 -0000       1.13
+++ ip_conntrack_core.c 26 May 2004 11:05:23 -0000
@@ -1139,10 +1139,13 @@
        DUMP_TUPLE(newreply);

        conntrack->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
-       if (!conntrack->master)
-               conntrack->helper = LIST_FIND(&helpers, helper_cmp,
-                                             struct ip_conntrack_helper *,
-                                             newreply);
+       if (!conntrack->master) {
+               struct ip_conntrack_helper *newhelper;
+               newhelper = LIST_FIND(&helpers, helper_cmp,
+                                     struct ip_conntrack_helper *,
+                                     newreply);
+               if (newhelper) conntrack->helper = newhelper;
+       }
        WRITE_UNLOCK(&ip_conntrack_lock);

        return 1;


Or maybe, we should check whether it has expectations before looking for new 
helper.

Raivis Bucis

Re: kernel crash [problem with helpers]

[Patrick McHardy]

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

Raivis Bucis wrote:
> ip_conntrack assumes that expect->expectant->helper allways will be non null, 
> but that is not allways the case.
> 
> When tftp conntrack is used, on first received UDP packet, tfp helper adds 
> expectation. If it gets NATed later to some other port, 
> ip_conntrack_alter_reply will set its helper to NULL, but expectation is 
> still kept, and when expected conntrack is created, its master conntrack 
> doesn't have helper anymore. This leads to kernel oops in list_conntracks 
> when reading "/proc/net/ip_conntrack", etc.

Thanks for tracking this down.

> 
> Therefore I propose following fix:
> 
> diff -u -r1.13 ip_conntrack_core.c
> --- ip_conntrack_core.c 9 Jan 2004 07:52:10 -0000       1.13
> +++ ip_conntrack_core.c 26 May 2004 11:05:23 -0000
> @@ -1139,10 +1139,13 @@
>         DUMP_TUPLE(newreply);
> 
>         conntrack->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
> -       if (!conntrack->master)
> -               conntrack->helper = LIST_FIND(&helpers, helper_cmp,
> -                                             struct ip_conntrack_helper *,
> -                                             newreply);
> +       if (!conntrack->master) {
> +               struct ip_conntrack_helper *newhelper;
> +               newhelper = LIST_FIND(&helpers, helper_cmp,
> +                                     struct ip_conntrack_helper *,
> +                                     newreply);
> +               if (newhelper) conntrack->helper = newhelper;
> +       }
>         WRITE_UNLOCK(&ip_conntrack_lock);
> 
>         return 1;
> 
> 
> Or maybe, we should check whether it has expectations before looking for new 
> helper.

Yes, your second solution is better. Changing helpers is fine as long as
no expectations exist. I have added this patch instead.

Regards
Patrick

> 
> Raivis Bucis


[-- Attachment #2: linux.patch --]
[-- Type: text/x-patch, Size: 1063 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/05/27 02:01:28+02:00 kaber@trash.net 
#   [NETFILTER]: Don't assign new helper after NAT when there are already expectations present
# 
# net/ipv4/netfilter/ip_conntrack_core.c
#   2004/05/27 02:01:19+02:00 kaber@trash.net +2 -4
#   [NETFILTER]: Don't assign new helper after NAT when there are already expectations present
# 
diff -Nru a/net/ipv4/netfilter/ip_conntrack_core.c b/net/ipv4/netfilter/ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	2004-05-27 02:03:11 +02:00
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	2004-05-27 02:03:11 +02:00
@@ -1127,10 +1127,8 @@
 	DUMP_TUPLE(newreply);
 
 	conntrack->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
-	if (!conntrack->master)
-		conntrack->helper = LIST_FIND(&helpers, helper_cmp,
-					      struct ip_conntrack_helper *,
-					      newreply);
+	if (!conntrack->master && list_empty(&conntrack->sibling_list))
+		conntrack->helper = ip_ct_find_helper(newreply);
 	WRITE_UNLOCK(&ip_conntrack_lock);
 
 	return 1;