[Openswan dev] tcpdump and UDP encap on 2.6

[Openswan dev] tcpdump and UDP encap on 2.6

[Michael Richardson]

[-- Attachment #1.1: Type: text/plain, Size: 2614 bytes --]


Herbert, I think that there is a bug/mis-feature in net/ipv4/udp.c.
The skb is modified without having checked if it is in fact shared/cloned.
The result is that tcpdump sees the wrong thing. This can be confusing:

First, get the latest tcpdump 3.9 beta (-096), which decodes UDP port
4500 packets. 

If I tcpdump on the incoming interface, without the ESP_IN_UDP option
set (openswan "ikeping" has an option to turn this on):

west:/testing/klips/west-natt-01# jobs
[2]-  Running                 tcpdump-3.9 -i eth1 -n -p &
[3]+  Running                 ipsec ikeping --listen --ikeport 4500 &
west:/testing/klips/west-natt-01# received 36() packet from 192.1.2.23/4500 of len: 116
        rcookie=78563412_0f000000 icookie=353bc42c_e2464cf2 msgid=8cf7b22e
        np=239  version=13.7    xchg=(36)
18:11:00.673351 IP 192.1.2.23.4500 > 192.1.2.45.4500: UDP-encap: ESP(spi=0x12345678,seq=0xf), length 116

I'm clearly getting an UDP encapsulated packet.

west:/testing/klips/west-natt-01# jobs
[2]-  Running                 tcpdump-3.9 -i eth1 -n -p &
[3]+  Running                 ipsec ikeping --listen --ikeport 4500 --nat-t &

west:/testing/klips/west-natt-01# 
18:12:01.795291 IP 192.1.2.23 > 192.1.2.45: ESP(spi=0x11941194,seq=0x7c0000), length 116
    
Notice how the packet has been mangled before being passed to tcpdump.
This is a problem for anyone trying to debug what's going on.

I think that this fixes the problem. I must admit to being a bit
ignorant as to which PRIO might be appropriate here. Also is there a
good FAQ on the difference between cloned SKBs vs shared SKBs? 

--- /distros/kernel/linux-2.6.11.2/net/ipv4/udp.c	2005-03-09 03:11:09.000000000 -0500
+++ linux/net/ipv4/udp.c	2005-04-08 14:22:53.000000000 -0400
@@ -897,8 +897,9 @@
  *	0  if we should drop this packet
  * 	-1 if it should get processed by xfrm4_rcv_encap
  */
-static int udp_encap_rcv(struct sock * sk, struct sk_buff *skb)
+static int udp_encap_rcv(struct sock * sk, struct sk_buff **pskb)
 {
+	struct sk_buff *skb = *pskb;
 #ifndef CONFIG_XFRM
 	return 1; 
 #else
@@ -968,11 +969,14 @@
 	 * transport header to point to ESP.  Keep UDP on the stack
 	 * for later.
 	 */
+	skb = skb_unshare(skb, 0);
 	skb->h.raw = skb_pull(skb, len);
 
 	/* modify the protocol (it's ESP!) */
 	iph->protocol = IPPROTO_ESP;
 
+	*pskb = skb;
+
 	/* and let the caller know to send this into the ESP processor... */
 	return -1;
 #endif
@@ -1010,7 +1014,7 @@
 		 */
 		int ret;
 
-		ret = udp_encap_rcv(sk, skb);
+		ret = udp_encap_rcv(sk, &skb);
 		if (ret == 0) {
 			/* Eat the packet .. */
 			kfree_skb(skb);




    

    




[-- Attachment #1.2: Type: application/pgp-signature, Size: 306 bytes --]

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

_______________________________________________
Dev mailing list
Dev@openswan.org
http://lists.openswan.org/mailman/listinfo/dev

[Openswan dev] [IPSEC] COW skb header in UDP decap

[Herbert Xu]

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

Hi Michael:

On Fri, Apr 08, 2005 at 02:27:33PM -0400, Michael Richardson wrote:
> 
> Herbert, I think that there is a bug/mis-feature in net/ipv4/udp.c.
> The skb is modified without having checked if it is in fact shared/cloned.
> The result is that tcpdump sees the wrong thing. This can be confusing:

Indeed, it's not just confusing but wrong :) This violates the
laws of skb's.
 
> I think that this fixes the problem. I must admit to being a bit

Thanks for the patch.  I think we can use something a little lighter
than skb_unshare.  The following patch just makes the header part of
the skb writeable.  This is needed since we modify the IP headers
just a few lines below.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

> ignorant as to which PRIO might be appropriate here. Also is there a
> good FAQ on the difference between cloned SKBs vs shared SKBs? 

There might be, but the source is the ultimate documentation :)

Shared skb's are the same object held by multiple users.  No one
must write to either the metadata or the data without the consent
of everyone involved.

Clone skb's only share the data.  The metadata can be written freely.

To get a completely standalone skb from a cloned skb, you can call
skb_copy.  However, it is pretty heavy weight and also copies the
metadata which isn't necessary for skb's which are cloned but not
shared.

That's why we have pskb_expand_head (and its wrapper skb_cow) which
can be used when you only need to modify the bits between skb->head
and skb->tail (this is what we call the header of the skb, don't
confuse it with the protocol header of a packet).

As it doesn't copy the metadata, you can't call it on shared skb's.
However, you can always clone it and then do pskb_expand_head.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 460 bytes --]

===== net/ipv4/udp.c 1.78 vs edited =====
--- 1.78/net/ipv4/udp.c	2005-03-27 09:04:35 +10:00
+++ edited/net/ipv4/udp.c	2005-04-09 10:39:05 +10:00
@@ -955,6 +955,8 @@
 	 * header and optional ESP marker bytes) and then modify the
 	 * protocol to ESP, and then call into the transform receiver.
 	 */
+	if (skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
+		return 0;
 
 	/* Now we can update and verify the packet length... */
 	iph = skb->nh.iph;

[-- Attachment #3: Type: text/plain, Size: 129 bytes --]

_______________________________________________
Dev mailing list
Dev@openswan.org
http://lists.openswan.org/mailman/listinfo/dev

[Openswan dev] Re: [IPSEC] COW skb header in UDP decap

[David S. Miller]

On Sat, 9 Apr 2005 11:01:24 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> > I think that this fixes the problem. I must admit to being a bit
> 
> Thanks for the patch.  I think we can use something a little lighter
> than skb_unshare.  The following patch just makes the header part of
> the skb writeable.  This is needed since we modify the IP headers
> just a few lines below.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks a lot Herbert.

Re: [Openswan dev] Re: [IPSEC] COW skb header in UDP decap

[Paul Wouters]

On Tue, 19 Apr 2005, David S. Miller wrote:

>> Thanks for the patch.  I think we can use something a little lighter
>> than skb_unshare.  The following patch just makes the header part of
>> the skb writeable.  This is needed since we modify the IP headers
>> just a few lines below.
>>
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Applied, thanks a lot Herbert.

Which reminds me. I still have hanging 'ip xfrm state' commands on 2.6.11.7

Paul

Re: [Openswan dev] Re: [IPSEC] COW skb header in UDP decap

[Herbert Xu]

On Wed, Apr 20, 2005 at 04:10:01PM +0200, Paul Wouters wrote:
> 
> Which reminds me. I still have hanging 'ip xfrm state' commands on 2.6.11.7

What does strace say?
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt