ip_defrag / ip_conntrack issues (was Re: [PATCH] Fix netfilter locking)

DISCUSSION Harald Welte 5 participants 8 messages Dec 18, 2000 – Dec 20, 2000 1 message not in the archive

Subsystems (2)

Networking IPv4/IPv6

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): E147qmG-0001yB-00@halfway

Harald Welte

author

2000-12-18 19:47 UTC #1 lore raw

Quoted 8 lines · David S. Miller

From: Rusty Russell <[email protected]>
Date: Mon, 18 Dec 2000 14:15:52 +1100
Alexey is right, locking is screwed (explains some reports of occasional failure during rmmod).
Patch applied, thank you.

Quoted 7 lines

I have a patch which compiles for non-linear skb mods to netfilter (NAT uses linear packets still, but connection tracking and packet filtering only linearize minimal requirements). Waiting for DaveM's solution to ip_ct_gather_frags()...
I feel it's best to just skb_clone() the skb arg to ip_defrag and this will close the whole thing, I think.

no. The clone()d skb will still have a skb->dev pointing to NULL.

The problem occurs only for locally-generated outgoing packets, which need to be fragmented:

- ip_build_xmit() discoveres it has to fragment
- ip_build_xmit_slow() generates fragments and calls
- NF_HOOK() for NF_IP_LOCAL_OUT
- ip_conntrack_local() is called, which in turn calls
- ip_conntrack_in(), which calls
- ip_ct_gather_frags(), which calls
- ip_defrag(), which calls

[now we have two possible oops - caues]

a ip_find(), which calls
a ip_frag_create(), which initialises a timer with the function a ip_expire(), which dereferences qp->iif

b ip_frag_queue(), which dereferences it in qp->iif= skb->dev->ifindex

as andi kleen pointed out:

Quoted 8 lines

Quoted 3 lines
Also is it sure that the backtrace involves ip_rcv ? A more likely guess is that it happens during the IP_LOCAL_OUT hook, when skb->dev isn't set yet, but conntrack already has to already reassemble fragments.
Actually, I do not understand how current code could even have worked in the past. Once the SKB is passed to ip_defrag, it is nobody's buisness to reference that SKB anymore. This ip_defrag call is (from

mmh... we really don't do this. We use the return value of ip_defrag(), which is what ip_frag_reasm() returns (== the new datagram consisting out of all its fragments).

Quoted 3 lines

Alexey, what have I missed? I don't like the ip_fragment.c proposed fix for this reason, what netfilter is doing with ip_defrag here looks just wrong.

Well, my conclusion is:

- the defragmentation code in the ipv4 stack assumes that skb->dev points to a valid device. It does this primaryly to send icmp reassembly errors if a fragment reassembly timeout occurs

- netfilter wants to use the ip_defrag for defragmentation, not only for incoming packets from the network at NF_IP_PRE_ROUTING, but also for locally-generated fragmented packets (at NF_IP_LOCAL_OUT). Unfortunately we don't have a valid skb->dev at this point.

I don't think that there is any skb_clone()ing needed, nor this would solve the problem. The solution is

a) netfilter conntrack (more exactly: ip_ct_gather_frags) sets skb->dev to something valid (skb->dst->dev). Dirty hack.

b) make ip_defrag(), ... aware of the case where skb->dev == NULL. Sounds like a good idea, since it is only one if(skb->dev) clause.

c) netfilter stops using ip_defrag() for this case. Bad idea, it had to reinvent the wheel :(

Quoted 2 lines

David S. Miller
[email protected]

Signature · 9 lines

Live long and prosper
- Harald Welte / [email protected]                http://www.gnumonks.org
============================================================================
GCS/E/IT d- s-: a-- C+++ UL++++$ P+++ L++++$ E--- W- N++ o? K- w--- O- M- 
V-- PS+ PE-- Y+ PGP++ t++ 5-- !X !R tv-- b+++ DI? !D G+ e* h+ r% y+(*)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Tom Leete

2000-12-19 14:35 UTC #2 lore raw

In reply to a message not in the archive

Quoted 75 lines · Harald Welte

Quoted 8 lines · David S. Miller
From: Rusty Russell <[email protected]>
Date: Mon, 18 Dec 2000 14:15:52 +1100
Alexey is right, locking is screwed (explains some reports of occasional failure during rmmod).
Patch applied, thank you.
Quoted 7 lines
I have a patch which compiles for non-linear skb mods to netfilter (NAT uses linear packets still, but connection tracking and packet filtering only linearize minimal requirements). Waiting for DaveM's solution to ip_ct_gather_frags()...
I feel it's best to just skb_clone() the skb arg to ip_defrag and this will close the whole thing, I think.
no. The clone()d skb will still have a skb->dev pointing to NULL.
The problem occurs only for locally-generated outgoing packets, which need to be fragmented:
- ip_build_xmit() discoveres it has to fragment
- ip_build_xmit_slow() generates fragments and calls
- NF_HOOK() for NF_IP_LOCAL_OUT
- ip_conntrack_local() is called, which in turn calls
- ip_conntrack_in(), which calls
- ip_ct_gather_frags(), which calls
- ip_defrag(), which calls
[now we have two possible oops - caues]
a ip_find(), which calls
a ip_frag_create(), which initialises a timer with the function a ip_expire(), which dereferences qp->iif
b ip_frag_queue(), which dereferences it in qp->iif= skb->dev->ifindex
as andi kleen pointed out:
Quoted 8 lines
Quoted 3 lines
Also is it sure that the backtrace involves ip_rcv ? A more likely guess is that it happens during the IP_LOCAL_OUT hook, when skb->dev isn't set yet, but conntrack already has to already reassemble fragments.
Actually, I do not understand how current code could even have worked in the past. Once the SKB is passed to ip_defrag, it is nobody's buisness to reference that SKB anymore. This ip_defrag call is (from
mmh... we really don't do this. We use the return value of ip_defrag(), which is what ip_frag_reasm() returns (== the new datagram consisting out of all its fragments).
Quoted 3 lines
Alexey, what have I missed? I don't like the ip_fragment.c proposed fix for this reason, what netfilter is doing with ip_defrag here looks just wrong.
Well, my conclusion is:
- the defragmentation code in the ipv4 stack assumes that skb->dev points to a valid device. It does this primaryly to send icmp reassembly errors if a fragment reassembly timeout occurs
- netfilter wants to use the ip_defrag for defragmentation, not only for incoming packets from the network at NF_IP_PRE_ROUTING, but also for locally-generated fragmented packets (at NF_IP_LOCAL_OUT). Unfortunately we don't have a valid skb->dev at this point.
I don't think that there is any skb_clone()ing needed, nor this would solve the problem. The solution is
a) netfilter conntrack (more exactly: ip_ct_gather_frags) sets skb->dev to something valid (skb->dst->dev). Dirty hack.

Quoted 7 lines

b) make ip_defrag(), ... aware of the case where skb->dev == NULL. Sounds like a good idea, since it is only one if(skb->dev) clause.
c) netfilter stops using ip_defrag() for this case. Bad idea, it had to reinvent the wheel :(
- Harald Welte / [email protected] http://www.gnumonks.org

I'm now testing this small patch based on your suggestion b). I have netfilter running, nfs mounts are behaving well, and fragmented pings dont kill the box. I made no attempt to resolve anything but the derefernce of a netdevice *dev.= NULL

The patch only deals with the ip_defrag_queue path. I have not seen the alternate one happen. It's only been up an hour, I'll put some more time on it.

Cheers,
Tom

net/ipv4/ip_fragment.c renamed from net/ipv4/ip_fragment.c~ +2 -1

@@ -485,7 +485,8 @@
 	else
 		qp->fragments = skb;
 
-	qp->iif = skb->dev->ifindex;
+	if (skb->dev)
+	        qp->iif = skb->dev->ifindex;
 	skb->dev = NULL;
 	qp->meat += skb->len;
 	atomic_add(skb->truesize, &ip_frag_mem);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-19 14:59 UTC #3 lore raw

Date: Tue, 19 Dec 2000 08:55:35 -0500
From: Tom Leete <[email protected]>

The patch only deals with the ip_defrag_queue path. I have not seen the alternate one happen. It's only been up an hour, I'll put some more time on it.

--- linux/net/ipv4/ip_fragment.c~ Tue Dec 12 06:56:29 2000
+++ linux/net/ipv4/ip_fragment.c Tue Dec 19 07:29:53 2000
@@ -485,7 +485,8 @@

This is basically what is in my tree right now. However, there was one reporter who claimed that after this kind of change he still was able to lockup/OOPS his machine by logging into X as a user who had his home directory over NFS. This was with netfilter enabled as well so it has to be the same bug.

Later,
David S. Miller
[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

[email protected]

2000-12-19 15:26 UTC #4 lore raw

Re: ip_defrag / ip_conntrack issues (was Re: [PATCH] Fix netfilter

Hello!

Quoted 2 lines

able to lockup/OOPS his machine by logging into X as a user who had his home directory over NFS.

I believe this report is to be ignored. It is fully meaningless. X has nothing to do with NFS, NFS is with X, and defragmenter is at least with one of them.

Alexey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Mohammad A. Haque

2000-12-20 02:16 UTC #5 lore raw

Re: ip_defrag / ip_conntrack issues (was Re: [PATCH] Fix netfilter

how is this meaningless?

This just confirms what I and others have found in test12 wrt to the netfilter issue.

Quoted 9 lines · [email protected] wrote:

Hello!
Quoted 2 lines
able to lockup/OOPS his machine by logging into X as a user who had his home directory over NFS.
I believe this report is to be ignored. It is fully meaningless. X has nothing to do with NFS, NFS is with X, and defragmenter is at least with one of them.

Signature · 12 lines

=====================================================================
Mohammad A. Haque                              http://www.haque.net/ 
                                               [email protected]

  "Alcohol and calculus don't mix.             Project Lead
   Don't drink and derive." --Unknown          http://wm.themes.org/
                                               [email protected]
=====================================================================
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-20 02:22 UTC #6 lore raw

Re: ip_defrag / ip_conntrack issues (was Re: [PATCH] Fix netfilter

Date: Tue, 19 Dec 2000 20:45:14 -0500
From: "Mohammad A. Haque" <[email protected]>

how is this meaningless?

This just confirms what I and others have found in test12 wrt to the netfilter issue.

Alexey is talking about test12/netfilter + our ip_fragment.c fix, not vanilla test12.

Mohammad A. Haque

2000-12-20 03:18 UTC #7 lore raw

Re: ip_defrag / ip_conntrack issues (was Re: [PATCH] Fix netfilter

In reply to [email protected]

Woops, my bad. These finally exams are getting to me.

Sorry.

Quoted 2 lines · David S. Miller

Alexey is talking about test12/netfilter + our ip_fragment.c fix, not vanilla test12.

Signature · 12 lines

=====================================================================
Mohammad A. Haque                              http://www.haque.net/ 
                                               [email protected]

  "Alcohol and calculus don't mix.             Project Lead
   Don't drink and derive." --Unknown          http://wm.themes.org/
                                               [email protected]
=====================================================================
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Tom Leete

2000-12-20 06:30 UTC #8 lore raw

In reply to a message not in the archive

Quoted 9 lines · David S. Miller

This is basically what is in my tree right now. However, there was one reporter who claimed that after this kind of change he still was able to lockup/OOPS his machine by logging into X as a user who had his home directory over NFS. This was with netfilter enabled as well so it has to be the same bug.
Later,
David S. Miller
[email protected]

2.4.0-test12+ip_fragment.c.patch is still up. I hadn't previously tried client-side mounts on that box, just exports.

I tried to reproduce the problem with nfs home directory mount + X. I am unable to generate the error -- it works for me. I think Alexey is right. There may be either coincidence or confusion in the report.

Btw, I am able to compile a kernel over an nfs mount with this. Very handy since the remote is much faster at it.

Cheers,
Tom
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/