Re: kernel crash [problem with helpers]

[Patrick McHardy <kaber@trash.net>]

[-- 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;