Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Harald Welte]

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

On Tue, Sep 28, 2004 at 01:05:12PM -0700, David S. Miller wrote:
 
> If this is true, we need to fix this soon.

Agreed.  

I think I've seen something similar in the past, though it was some
broken code in my experimental tree ;). ip_conntrack on an NFS server
has always had a bad history, I have to admit :(

Big question number 1:
First of all, why would a local loopback nfs mount do any fragmenting
in the first place?  'lo' has a MTU of 16k, so why would any piece of
code in the output path build fragments, even with rsize/wsize=8192?

The conntrack message basically means that at NF_IP_PRE_ROUTING we
suddenly see fragmented packets.  This "can never happen" since at the
same PRE_ROUTING hook we defragment just before
via ip_conntrack_defrag() -> ip_ct_gather_frags() -> ip_defrag()

Big question number 2:
How can a fragment survive ip_defrag() ?

Big question number 3:
What is this magic 8k limit?  ip_conntrack doesn't impose any arbitrary
size limitations to packets... and we just use the normal
defragmentation code.

I'm more than puzzled about this, need to do some testing.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Patrick McHardy]

Harald Welte wrote:

>The conntrack message basically means that at NF_IP_PRE_ROUTING we
>suddenly see fragmented packets.  This "can never happen" since at the
>same PRE_ROUTING hook we defragment just before
>via ip_conntrack_defrag() -> ip_ct_gather_frags() -> ip_defrag()
>
>Big question number 2:
>How can a fragment survive ip_defrag() ?
>

Pretty simple:

static unsigned int ip_conntrack_defrag(unsigned int hooknum,
                                        struct sk_buff **pskb,
                                        const struct net_device *in,
                                        const struct net_device *out,
                                        int (*okfn)(struct sk_buff *))
{
        /* Previously seen (loopback)?  Ignore.  Do this before
           fragment check. */
        if ((*pskb)->nfct)
                return NF_ACCEPT;

I'll send a patch later.

Regards
Patrick

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Patrick McHardy]

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

Harald Welte wrote:

>The conntrack message basically means that at NF_IP_PRE_ROUTING we
>suddenly see fragmented packets.  This "can never happen" since at the
>same PRE_ROUTING hook we defragment just before
>via ip_conntrack_defrag() -> ip_ct_gather_frags() -> ip_defrag()
>  
>

Here is the patch. Untracked and already tracked (loopback) packets
are not defragmented, but the check in ip_conntrack_in for
untracked/already tracked is after the check for fragments.
This patch moves it up.




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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/09/29 01:26:22+02:00 kaber@coreworks.de 
#   [NETFILTER]: move check for already tracked/untracked before fragment check
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/netfilter/ip_conntrack_core.c
#   2004/09/29 01:25:49+02:00 kaber@coreworks.de +6 -6
#   [NETFILTER]: move check for already tracked/untracked before fragment check
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
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-09-29 01:27:34 +02:00
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	2004-09-29 01:27:34 +02:00
@@ -688,6 +688,12 @@
 	int set_reply;
 	int ret;
 
+	/* Previously seen (loopback or untracked)?  Ignore. */
+	if ((*pskb)->nfct) {
+		CONNTRACK_STAT_INC(ignore);
+		return NF_ACCEPT;
+	}
+
 	/* Never happen */
 	if ((*pskb)->nh.iph->frag_off & htons(IP_OFFSET)) {
 		if (net_ratelimit()) {
@@ -714,12 +720,6 @@
 		       (*pskb)->sk, (*pskb)->pkt_type);
 	}
 #endif
-
-	/* Previously seen (loopback or untracked)?  Ignore. */
-	if ((*pskb)->nfct) {
-		CONNTRACK_STAT_INC(ignore);
-		return NF_ACCEPT;
-	}
 
 	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
 

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[David S. Miller]

On Wed, 29 Sep 2004 01:35:53 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Harald Welte wrote:
> 
> >The conntrack message basically means that at NF_IP_PRE_ROUTING we
> >suddenly see fragmented packets.  This "can never happen" since at the
> >same PRE_ROUTING hook we defragment just before
> >via ip_conntrack_defrag() -> ip_ct_gather_frags() -> ip_defrag()
> >  
> >
> 
> Here is the patch. Untracked and already tracked (loopback) packets
> are not defragmented, but the check in ip_conntrack_in for
> untracked/already tracked is after the check for fragments.
> This patch moves it up.

Applied, thanks a lot Patrick.

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Harald Welte]

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

On Wed, Sep 29, 2004 at 01:35:53AM +0200, Patrick McHardy wrote:
> Harald Welte wrote:
> 
> >The conntrack message basically means that at NF_IP_PRE_ROUTING we
> >suddenly see fragmented packets.  This "can never happen" since at the
> >same PRE_ROUTING hook we defragment just before
> >via ip_conntrack_defrag() -> ip_ct_gather_frags() -> ip_defrag()
> > 
> >
> 
> Here is the patch. Untracked and already tracked (loopback) packets
> are not defragmented, but the check in ip_conntrack_in for
> untracked/already tracked is after the check for fragments.
> This patch moves it up.

Sometimes I feel like I'm blind.  Thanks, Patrick.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Henrik Nordstrom]

On Wed, 29 Sep 2004, Harald Welte wrote:

>> Here is the patch. Untracked and already tracked (loopback) packets
>> are not defragmented, but the check in ip_conntrack_in for
>> untracked/already tracked is after the check for fragments.
>> This patch moves it up.
>
> Sometimes I feel like I'm blind.  Thanks, Patrick.

Hmm..

loopback traffic is seen twice by netfilter, once when being sent, once 
when being received. If a packet larger than the loopback MTU is sent this 
will get fragmented (by conntrack) and then reinjected..  has it been 
fixed so the reinjected traffic (which is the same skbuff) has it's 
conntrack state cleared?

I remember this issue coming up before wrt NAT where loopback packets were 
seen as NEW twice, but I don't remember what the fix was.

Regards
Henrik

Re: Fw: [Bug 133788] New: ip_conntrack_in: Frag of proto 17

[Harald Welte]

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

On Wed, Sep 29, 2004 at 10:41:31AM +0200, Henrik Nordstrom wrote:

> will get fragmented (by conntrack) and then reinjected..  has it been 
> fixed so the reinjected traffic (which is the same skbuff) has it's 
> conntrack state cleared?

no, the nfct remains the same, so we just ignore it from conntrack point
of view (NF_ACCEPT very early, see patrick's patch).

> I remember this issue coming up before wrt NAT where loopback packets were 
> seen as NEW twice, but I don't remember what the fix was.

No, this shouldn't happen (now anymore?), since the nfct of
LOCAL_OUT/POST_ROUTING is still present in PRE_ROUTING/LOCAL_IN.

I am not sure whether we can make this 'optimiziation'.  Thinking of TCP
window tracking and someone having packet filter rules for the loopback
device...  But at the moment I cannot think why conntrack'ing the packet
twice would make any difference at all.

NAT on loopback connections.  *sigh*.  I think this is beyond my
imagination at this point.

Most firewalls I've deployed have NOTRACK rules for loopback traffic, so
I tend not to see those issues.

> Regards
> Henrik

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]