From: Olaf Kirch <hidden> Date: 2004-08-18 09:13:52
Hi,
here's a patch that keeps us from crashing on removal of ip_conntrack.
This problem came up during IBM's testing of SLES.
I'm not sure if this issue has been submitted already.
Problem description courtesy of David Stevens:
It appears that conntrack, when loaded, is queueing the fragments
for reassembly pre-routing (ie, when skb->dst is 0) and giving
the fully reassembled packet to the pre-routing code which will
set skb->dst before using it.
IP without conntrack does the queueing of fragments and reassembly
post-routing, so skb->dst in that case is set for all fragments
and the reassembled packet.
In the failure scenario, it appears that conntrack has queued
some of the fragments (w/ skb->dst=0, esp. in the offset=0 first
fragment) and then the conntrack module is removed. Arrival
of a fragment afterward will queue and reassemble the entire
packet post-routing, but the first frag still has skb->dst 0,
so it'll blow up
To fix this, the patch below simply drops such skbs. A different fix
could be to change the conntrack module to flush out all unassembled
fragments when unloaded; an alternative patch for this is attached as
well (this one is completely untested).
Cheers
Olaf
--
Olaf Kirch | The Hardware Gods hate me.
okir@suse.de |
---------------+
From: Harald Welte <hidden> Date: 2004-08-19 10:11:59
On Wed, Aug 18, 2004 at 11:13:52AM +0200, Olaf Kirch wrote:
Hi,
here's a patch that keeps us from crashing on removal of ip_conntrack.
This problem came up during IBM's testing of SLES.
Thanks for this detailed bugreport and fix.
I'm not sure if this issue has been submitted already.
Not that I'm aware of.
To fix this, the patch below simply drops such skbs. A different fix
could be to change the conntrack module to flush out all unassembled
fragments when unloaded; an alternative patch for this is attached as
well (this one is completely untested).
Since I don't want to put any more conntrack-specific code into the core
network stack, I'd rather go for the 'alternative patch'.
I'm not sure whether it's worth the effort to combine the two, i.e. only
flush entries with skb->dst == NULL.
But especially since module unloading is EXPERIMENTAL anyway, I think
it's ok when we completely flush the fragemnt queue.
Dave, is this fine with you? What solution would you prefer?
Cheers
Olaf
--
- Harald Welte [off-list ref] 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
From: David S. Miller <hidden> Date: 2004-08-19 15:14:28
On Thu, 19 Aug 2004 16:55:58 +0200
Patrick McHardy [off-list ref] wrote:
These are Olaf's patches. I agree with Harald that the second
patch is better. I've fixed it up so it applies with the recent
ip_fragment.c changes.
I have a better idea.
Instead of setting skb->dst to NULL, it should set it to some
NULL destination entry which just frees up the packets. Then
no special case handling. skb->dst==NULL packets should never
get into the fragment queue to begin with.
From: Patrick McHardy <hidden> Date: 2004-08-21 15:10:20
David S. Miller wrote:
I have a better idea.
Instead of setting skb->dst to NULL, it should set it to some
NULL destination entry which just frees up the packets. Then
no special case handling. skb->dst==NULL packets should never
get into the fragment queue to begin with.
The problem is that conntrack unload can cause packets without a
dst_entry to appear in ip_local_deliver, which is already after
the call to dst_input.
Regards
Patrick
From: David S. Miller <hidden> Date: 2004-08-22 05:13:44
On Sat, 21 Aug 2004 17:10:20 +0200
Patrick McHardy [off-list ref] wrote:
David S. Miller wrote:
quoted
I have a better idea.
Instead of setting skb->dst to NULL, it should set it to some
NULL destination entry which just frees up the packets. Then
no special case handling. skb->dst==NULL packets should never
get into the fragment queue to begin with.
The problem is that conntrack unload can cause packets without a
dst_entry to appear in ip_local_deliver, which is already after
the call to dst_input.
How can it call ip_local_deliver() without a valid skb->dst?
That function is only invoked via skb->dst->input(skb) which
by implication means that skb->dst is non-NULL.
Actually there is a call via ip_mr_input() but that code also
has a precondition that skb->dst is non-NULL too. I say this
due to the unchecked skb->dst accesses it makes early on.
Please explain. I don't question that it happens, just show
me how :-)
From: Patrick McHardy <hidden> Date: 2004-08-22 12:58:01
David S. Miller wrote:
On Sat, 21 Aug 2004 17:10:20 +0200
Patrick McHardy [off-list ref] wrote:
quoted
The problem is that conntrack unload can cause packets without a
dst_entry to appear in ip_local_deliver, which is already after
the call to dst_input.
How can it call ip_local_deliver() without a valid skb->dst?
That function is only invoked via skb->dst->input(skb) which
by implication means that skb->dst is non-NULL.
Actually there is a call via ip_mr_input() but that code also
has a precondition that skb->dst is non-NULL too. I say this
due to the unchecked skb->dst accesses it makes early on.
Please explain. I don't question that it happens, just show
me how :-)
The first fragment (offset=0) is given to ip_defrag by conntrack
at PRE_ROUTING, without a dst_entry. Then conntrack is unloaded.
Further fragments are now queued in ip_local_deliver. When the
packet is reassembled and "continues" its way from
ip_local_deliver, it doesn't have a dst_entry.
The opposite way is of course also possible, packets queued in
ip_local_deliver can jump and appear in the PRE_ROUTING hook
when conntrack is loaded, but that way doesn't seem to cause
problems.
Regards
Patrick
From: David S. Miller <hidden> Date: 2004-08-23 05:03:31
On Sun, 22 Aug 2004 14:58:01 +0200
Patrick McHardy [off-list ref] wrote:
The first fragment (offset=0) is given to ip_defrag by conntrack
at PRE_ROUTING, without a dst_entry. Then conntrack is unloaded.
Further fragments are now queued in ip_local_deliver. When the
packet is reassembled and "continues" its way from
ip_local_deliver, it doesn't have a dst_entry.
The opposite way is of course also possible, packets queued in
ip_local_deliver can jump and appear in the PRE_ROUTING hook
when conntrack is loaded, but that way doesn't seem to cause
problems.
Thanks for the explanation Patrick.
Let me brain storm on this on Monday (tomorrow).
From: David Stevens <hidden> Date: 2004-08-23 21:18:24
BTW, since some of the frags (esp. the one that triggers the problem)
are added post-routing, a valid dst is available. It just isn't the first
frag in the particular scenario.
So, one solution would be to set skb->dst for the head (if NULL) based
on a non-null fragment skb->dst. I believe that would prevent the problem
case without dropping the fragment, since it'll be processed post-routing
only if one of the frags is.
When I was looking at it, I wondered if conntrack really has a need to
reassemble itself, though. Couldn't it let IP do the reassembling and
just ignore offset != 0 frags? The offset==0 frags will have enough
protocol header to identify by port (a requirement for ICMP). But I don't
know this code well enough to know if conntrack does actually need
to reassemble for some good reason. Superficially, I wouldn't think
there'd be a reason for it.
+-DLS
So, one solution would be to set skb->dst for the head (if NULL) based
on a non-null fragment skb->dst. I believe that would prevent the problem
case without dropping the fragment, since it'll be processed post-routing
only if one of the frags is.
This would be more performant than dropping the frags, and
requiring a retransmit (or lack thereof, depending on protocol).
When I was looking at it, I wondered if conntrack really has a need to
reassemble itself, though. Couldn't it let IP do the reassembling and
I asked Harald this when I met him last, and he said it
does need to. So I don't think this (having conntrack
reassemble) is avoidable, unfortunately.
Of course, fragmentation, on the other hand, :), ...
thanks,
Nivedita
From: Patrick McHardy <hidden> Date: 2004-08-24 00:45:41
David Stevens wrote:
BTW, since some of the frags (esp. the one that triggers the problem)
are added post-routing, a valid dst is available. It just isn't the first
frag in the particular scenario.
So, one solution would be to set skb->dst for the head (if NULL) based
on a non-null fragment skb->dst. I believe that would prevent the problem
case without dropping the fragment, since it'll be processed post-routing
only if one of the frags is.
The fragments which jumped from PRE_ROUTING to ip_local_deliver will miss
ip options processing.
When I was looking at it, I wondered if conntrack really has a need to
reassemble itself, though. Couldn't it let IP do the reassembling and
just ignore offset != 0 frags? The offset==0 frags will have enough
protocol header to identify by port (a requirement for ICMP). But I don't
know this code well enough to know if conntrack does actually need
to reassemble for some good reason. Superficially, I wouldn't think
there'd be a reason for it.
The NAT code needs to handle all fragments, so they can't be skipped.
Handling fragments in conntrack and NAT would be possible without helpers,
but to scan for patterns in fragments you need state for each fragmented
packet for each connection.
Regards
Patrick
From: David Stevens <hidden> Date: 2004-08-24 21:28:07
Then it appears that simply dropping the packet when the
skb->dst == 0 isn't quite right, since per-frag option processing
wouldn't be done in the case where conntrack is removed, but
the first frag does have skb->dst set (but not some of the others).
In that case, it appears that conntrack needs to flush the entire
frag queue when it's unloaded. That shouldn't happen much,
so maybe that's not such a bad idea.
+-DLS
From: "David S. Miller" <davem@davemloft.net> Date: 2004-08-29 06:15:29
On Tue, 24 Aug 2004 15:28:07 -0600
David Stevens [off-list ref] wrote:
In that case, it appears that conntrack needs to flush the entire
frag queue when it's unloaded. That shouldn't happen much,
so maybe that's not such a bad idea.
I think I agree with David now that I've read through this a few
times. Can someone send me a patch which does this?
Does 2.4.x have this problem too? I thought it didn't.
From: Patrick McHardy <hidden> Date: 2004-08-29 19:36:28
David S. Miller wrote:
I think I agree with David now that I've read through this a few
times. Can someone send me a patch which does this?
Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.
Does 2.4.x have this problem too? I thought it didn't.
I'll have a look, but I think it does.
Regards
Patrick
From: "David S. Miller" <davem@davemloft.net> Date: 2004-08-29 19:57:08
On Sun, 29 Aug 2004 21:36:28 +0200
Patrick McHardy [off-list ref] wrote:
David S. Miller wrote:
quoted
I think I agree with David now that I've read through this a few
times. Can someone send me a patch which does this?
Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.
While we're doing this, is your patch similar to one of the two
original ones that Olaf Kirch posted? I want to give him proper
attribution, in whatever form is reasonable, that's all.
quoted
Does 2.4.x have this problem too? I thought it didn't.
From: Patrick McHardy <hidden> Date: 2004-08-29 20:06:36
David S. Miller wrote:
On Sun, 29 Aug 2004 21:36:28 +0200
Patrick McHardy [off-list ref] wrote:
quoted
Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.
While we're doing this, is your patch similar to one of the two
original ones that Olaf Kirch posted? I want to give him proper
attribution, in whatever form is reasonable, that's all.
It is based on his second patch. He didn't sign off, otherwise I just
would have kept his Signed-off-by: line.
Regards
Patrick
From: Patrick McHardy <hidden> Date: 2004-08-29 21:48:28
Patrick McHardy wrote:
Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.
The patch is racy, another CPU could already have passed the check
for ip_ct_no_defrag and queue the packet after __ip_evictor calculated
the work to do, so the packet (or another one) will not get evicted.
This patch on top calls synchronize_net() to prevent this.
quoted hunk
@@ -1181,6 +1183,12 @@
#ifdef CONFIG_NETFILTER_DEBUG
unsigned int olddebug = skb->nf_debug;
#endif
+
+ if (unlikely(ip_ct_no_defrag)) {
+ kfree_skb(skb);
+ return NULL;
+ }
+
if (sk) {
sock_hold(sk);
skb_orphan(skb);
diff -Nru a/net/ipv4/netfilter/ip_conntrack_standalone.c b/net/ipv4/netfilter/ip_conntrack_standalone.c
From: Patrick McHardy <hidden> Date: 2004-08-29 21:58:06
David S. Miller wrote:
quoted
quoted
Does 2.4.x have this problem too? I thought it didn't.
I'll have a look, but I think it does.
2.4 has the same problem. Before I post the patch, 2.4 seems to be
missing this patch, do you already have it queued or should I send
a 2.4 version first ?
ChangeSet@1.1853, 2004-08-18 14:28:05-07:00, davem@nuts.davemloft.net
[IPV4]: Fix theoretical loop on SMP in ip_evictor().
Snapshot the amount of work to do, and just do it.
In this way we avoid a theoretical loop whereby
one cpu sits in ip_evictor() tossing fragments
while another keeps adding a fragment just as we
bring ip_frag_mem down below the low threshold.
Signed-off-by: David S. Miller [off-list ref]
From: "David S. Miller" <davem@davemloft.net> Date: 2004-08-29 23:38:21
On Sun, 29 Aug 2004 23:58:06 +0200
Patrick McHardy [off-list ref] wrote:
Before I post the patch, 2.4 seems to be
missing this patch, do you already have it queued or should I send
a 2.4 version first ?
ChangeSet@1.1853, 2004-08-18 14:28:05-07:00, davem@nuts.davemloft.net
[IPV4]: Fix theoretical loop on SMP in ip_evictor().
I pushed this off to Marcelo, he just didn't pull from my
tree yet, which is at:
bk://kernel.bkbits.net/davem/net-2.4
Where you'll find those fixes as:
ChangeSet@1.1498.1.2, 2004-08-18 14:26:09-07:00, davem@nuts.davemloft.net
[IPV4]: Fix theoretical loop on SMP in ip_evictor().
Snapshot the amount of work to do, and just do it.
In this way we avoid a theoretical loop whereby
one cpu sits in ip_evictor() tossing fragments
while another keeps adding a fragment just as we
bring ip_frag_mem down below the low threshold.
Signed-off-by: David S. Miller [off-list ref]
ChangeSet@1.1498.1.3, 2004-08-18 14:31:35-07:00, davem@nuts.davemloft.net
[IPV6]: ip6_evictor() has same problem as ip_evictor().
Signed-off-by: David S. Miller [off-list ref]
From: Patrick McHardy <hidden> Date: 2004-08-30 00:50:10
David S. Miller wrote:
On Sun, 29 Aug 2004 23:58:06 +0200
Patrick McHardy [off-list ref] wrote:
quoted
Before I post the patch, 2.4 seems to be
missing this patch, do you already have it queued or should I send
a 2.4 version first ?
ChangeSet@1.1853, 2004-08-18 14:28:05-07:00, davem@nuts.davemloft.net
[IPV4]: Fix theoretical loop on SMP in ip_evictor().
I pushed this off to Marcelo, he just didn't pull from my
tree yet, which is at:
bk://kernel.bkbits.net/davem/net-2.4
Where you'll find those fixes as:
ChangeSet@1.1498.1.2, 2004-08-18 14:26:09-07:00, davem@nuts.davemloft.net
[IPV4]: Fix theoretical loop on SMP in ip_evictor().
Great, here is the patch for 2.4 for the conntrack problem.
From: Olaf Kirch <hidden> Date: 2004-08-30 07:57:10
On Sun, Aug 29, 2004 at 09:36:28PM +0200, Patrick McHardy wrote:
David S. Miller wrote:
quoted
I think I agree with David now that I've read through this a few
times. Can someone send me a patch which does this?
Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.
I was actually going to test-drive my patch today, but you were
faster. Thanks!
Olad
--
Olaf Kirch | The Hardware Gods hate me.
okir@suse.de |
---------------+