From: Krzysztof Halasa <khc@pm.waw.pl> Date: 2006-11-29 02:09:18
Patrick McHardy [off-list ref] writes:
It might be the case that your network device has a
hard_header_len > LL_MAX_HEADER, which could trigger
a corruption.
Hmm... GRE tunnels add 24 bytes... I just noticed the following code in
include/linux/netdevice.h:
/*
* Compute the worst case header length according to the protocols
* used.
*/
#if !defined(CONFIG_AX25) && !defined(CONFIG_AX25_MODULE) && !defined(CONFIG_TR)
#define LL_MAX_HEADER 32
#else
#if defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)
#define LL_MAX_HEADER 96
#else
#define LL_MAX_HEADER 48
#endif
#endif
#if !defined(CONFIG_NET_IPIP) && \
!defined(CONFIG_IPV6) && !defined(CONFIG_IPV6_MODULE)
#define MAX_HEADER LL_MAX_HEADER
#else
#define MAX_HEADER (LL_MAX_HEADER + 48)
#endif
I don't use AX25, Token Ring, the old IPIP tunnels nor IPv6 here, but
I wonder if GRE tunnel (which is basically another, more compatible
form of IPIP) need the same treatment as IPIP.
I've confirmed that REJECTs over GRE tunnel caused that corruption.
Please try this patch on top of the REJECT patch (ideally after
verifying that the REJECT patch is really introducing the
corruption).
That was certain. The patch fixed the problem, confirmed with current
git tree as well. Thanks for looking at it.
I'm not sure about LL_MAX_HEADER (and/or MAX_HEADER) though. Should it
be changed as well?
There are many devices adding data to header space, perhaps tacking
devices doesn't count as the skb is being linearized in
dev->hard_start_xmit() or equivalent path?
--
Krzysztof Halasa
From: Patrick McHardy <hidden> Date: 2006-11-29 02:28:31
Krzysztof Halasa wrote:
Patrick McHardy [off-list ref] writes:
quoted
It might be the case that your network device has a
hard_header_len > LL_MAX_HEADER, which could trigger
a corruption.
Hmm... GRE tunnels add 24 bytes... I just noticed the following code in
include/linux/netdevice.h:
/*
* Compute the worst case header length according to the protocols
* used.
*/
#if !defined(CONFIG_NET_IPIP) && \
!defined(CONFIG_IPV6) && !defined(CONFIG_IPV6_MODULE)
#define MAX_HEADER LL_MAX_HEADER
#else
#define MAX_HEADER (LL_MAX_HEADER + 48)
#endif
I don't use AX25, Token Ring, the old IPIP tunnels nor IPv6 here, but
I wonder if GRE tunnel (which is basically another, more compatible
form of IPIP) need the same treatment as IPIP.
Both ipip and gre do this:
dev->hard_header_len = LL_MAX_HEADER + sizeof(struct iphdr);
which explains it. It is a bug in the REJECT target, but I was
wondering whether you were really seeing this. It looks like
it makes sense to add GRE to the MAX_HEADER case above though.
quoted
Please try this patch on top of the REJECT patch (ideally after
verifying that the REJECT patch is really introducing the
corruption).
That was certain. The patch fixed the problem, confirmed with current
git tree as well. Thanks for looking at it.
Thanks. Dave, please apply this patch.
[NETFILTER]: ipt_REJECT: fix memory corruption
On devices with hard_header_len > LL_MAX_HEADER ip_route_me_harder()
reallocates the skb, leading to memory corruption when using the stale
tcph pointer to update the checksum.
Signed-off-by: Patrick McHardy <redacted>
From: David Miller <davem@davemloft.net> Date: 2006-11-29 04:25:42
From: Patrick McHardy <redacted>
Date: Wed, 29 Nov 2006 03:28:25 +0100
[NETFILTER]: ipt_REJECT: fix memory corruption
On devices with hard_header_len > LL_MAX_HEADER ip_route_me_harder()
reallocates the skb, leading to memory corruption when using the stale
tcph pointer to update the checksum.
Signed-off-by: Patrick McHardy <redacted>
Applied, thanks Patrick.
And based upon your discovery wrt. MAX_HEADER I'm also
applying the following.
commit 93e3a20d6c67a09b867431e7d5b3e7bc97154fab
Author: David S. Miller [off-list ref]
Date: Tue Nov 28 20:24:10 2006 -0800
[NET]: Fix MAX_HEADER setting.
MAX_HEADER is either set to LL_MAX_HEADER or LL_MAX_HEADER + 48, and
this is controlled by a set of CONFIG_* ifdef tests.
It is trying to use LL_MAX_HEADER + 48 when any of the tunnels are
enabled which set hard_header_len like this:
dev->hard_header_len = LL_MAX_HEADER + sizeof(struct xxx);
The correct set of tunnel drivers which do this are:
ipip
ip_gre
ip6_tunnel
sit
so make the ifdef test match.
Noticed by Patrick McHardy.
Signed-off-by: David S. Miller [off-list ref]
From: David Miller <davem@davemloft.net> Date: 2006-11-29 05:04:18
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Nov 2006 15:56:57 +1100
David Miller [off-list ref] wrote:
quoted
Longer term this is really messy, we should handle this some
other way.
Definitely. I'm not sure whether 48 is enough even for recursive
tunnels. This should really just be a hint. It's OK to spend a
bit of time reallocating skb's if it's too small, but it's not OK
to die.
The recursive tunnel case is handled by the PMTU reductions
in the route, isn't it?
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-11-29 06:52:18
On Tue, Nov 28, 2006 at 09:04:16PM -0800, David Miller wrote:
quoted
Definitely. I'm not sure whether 48 is enough even for recursive
tunnels. This should really just be a hint. It's OK to spend a
bit of time reallocating skb's if it's too small, but it's not OK
to die.
The recursive tunnel case is handled by the PMTU reductions
in the route, isn't it?
Oh I wasn't suggesting that the current code is broken.
I'm just emphasising that LL_MAX_HEADER is by no means the *maximum*
header size in a Linux system. Anybody should be able to load a
new NIC module with a hard header size exceeding what LL_MAX_HEADER
is and the system should still function (albeit slower since every
packet sent down that device has to be reallocated).
In particular, nested tunnels is one such device which anybody can
construct without writing a kernel module.
As to getting rid of those ifdefs, here is one idea. We keep a
read-mostly global variable that represents the actual current
maximum LL header size. Everytime a new device appears (or if
its hard header size changes) we update this variable if needed.
Hmm, we don't actually update the hard header size should the
underlying device change for tunnels. Good thing the tunnels
only use that as a hint and reallocate if necessary :)
This is not optimal in that it never decreases, but it's certainly
better than a compile-time constant (e.g., people using distribution
kernels don't necessarily use tunnels).
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
commit 93e3a20d6c67a09b867431e7d5b3e7bc97154fab
Author: David S. Miller [off-list ref]
Date: Tue Nov 28 20:24:10 2006 -0800
[NET]: Fix MAX_HEADER setting.
MAX_HEADER is either set to LL_MAX_HEADER or LL_MAX_HEADER + 48, and
this is controlled by a set of CONFIG_* ifdef tests.
...
Noticed by Patrick McHardy.
And if we talk about names:
+ Spotted by Krzysztof Halasa.
probably wouldn't be too exaggerated...
From: David Miller <davem@davemloft.net> Date: 2006-12-01 04:22:07
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Nov 2006 17:51:46 +1100
I'm just emphasising that LL_MAX_HEADER is by no means the *maximum*
header size in a Linux system.
But it is the maximum "link level" singular header size.
It is MAX_HEADER which is the hack and the main issue.
What MAX_HEADER's setting is trying to do is optimistically allocate
enough for a single level of tunnelling. It does not handle nested
tunneling at all, of course.
As to getting rid of those ifdefs, here is one idea. We keep a
read-mostly global variable that represents the actual current
maximum LL header size. Everytime a new device appears (or if
its hard header size changes) we update this variable if needed.
Hmm, we don't actually update the hard header size should the
underlying device change for tunnels. Good thing the tunnels
only use that as a hint and reallocate if necessary :)
This is not optimal in that it never decreases, but it's certainly
better than a compile-time constant (e.g., people using distribution
kernels don't necessarily use tunnels).
I like this idea for the most part. It also deals nicely with, as you
alude to, how the MAX_HEADER scheme uses the space even if you don't
configure any tunnels at all.
Actually, I wonder how antiquated this all is. I bet we could get rid
of MAX_HEADER, then if we have to realloc headroom, we adjust some
per-device header thing which will behave like your global value idea
does. On the next allocation, we'll do the right thing. Although I
cannot come up with a scheme that works without reintroducing another
net_device pointer to sk_buff, which seems necessary to handle arbitrary
nesting. :-/
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-12-01 04:38:15
On Thu, Nov 30, 2006 at 08:22:06PM -0800, David Miller wrote:
What MAX_HEADER's setting is trying to do is optimistically allocate
enough for a single level of tunnelling. It does not handle nested
tunneling at all, of course.
Agreed, I should've said MAX_HEADER.
Actually, I wonder how antiquated this all is. I bet we could get rid
of MAX_HEADER, then if we have to realloc headroom, we adjust some
per-device header thing which will behave like your global value idea
does. On the next allocation, we'll do the right thing. Although I
cannot come up with a scheme that works without reintroducing another
net_device pointer to sk_buff, which seems necessary to handle arbitrary
nesting. :-/
Actually the scarier part is that TCP as well as ip_route_me_harder
doesn't guarantee enough headroom for IPsec. Fortunately TCP reserves
enough room (128 bytes) by default that it's unlikely to break with
non-nested IPsec. But it's still pretty nasty.
So in general when allocating packets we have two scenarios:
1) The dst is known and fixed, i.e., all datagram protocols. This is
the easy case where the headroom is known exactly beforehand.
2) The dst is unknown or may vary, this includes TCP, SCTP and DCCP.
This is where we currently use MAX_HEADER plus some protocol-specific
headroom.
Right now the normal (non-IPsec) dst output path always checks for
sufficient headroom and reallocates if necessary (ip_finish_output2).
I propose that we make IPsec do the same thing.
This change will make the stack safe from underflow crashes in IPsec.
There is also the ip_route_me_harder path where the dst varies. It
also tries to reallocate the packet if there isn't enough headroom for
the new dst. As long both IPsec and the normal path does the headroom
check, this can in fact be removed.
We can then make it more optimal because in the cases of TCP/SCTP/DCCP
we usually have a dst object. The only problem of course is that it
may vary. However, the common case by far is that the dst stays
constant. So we can optimise for it by getting the headroom from the
current dst and rely on the last-ditch reallocation to fix things up
if needed.
For standard MTU-sized packets this discussion is moot since we have
2K of memory in each chunk. However, for ACKs it could save a bit of
memory.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: David Miller <davem@davemloft.net> Date: 2006-12-12 01:34:00
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 1 Dec 2006 15:37:55 +1100
So in general when allocating packets we have two scenarios:
1) The dst is known and fixed, i.e., all datagram protocols. This is
the easy case where the headroom is known exactly beforehand.
2) The dst is unknown or may vary, this includes TCP, SCTP and DCCP.
This is where we currently use MAX_HEADER plus some protocol-specific
headroom.
Right now the normal (non-IPsec) dst output path always checks for
sufficient headroom and reallocates if necessary (ip_finish_output2).
I propose that we make IPsec do the same thing.
Agreed.
For standard MTU-sized packets this discussion is moot since we have
2K of memory in each chunk. However, for ACKs it could save a bit of
memory.
For linear MTU-sized SKBs yes, but TCP data packets are going out %99
of the time with paged data these days and thus suffers from the same
set of issues and potential savings.