Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

[Andrew Morton]

John McGowan <jmcgowan@inch.com> wrote:
>
> Kernel 2.6.13. Breaks libpcap.
> 
> Fedora Core 2, gcc 3.3.3, Pentium III (933MHz)
> 
> I had written about my dismay that traceproto and tcptraceroute
> no longer worked and suspected that libnet was broken.
> 
> It seems that it is libpcap that is broken by kernel 2.6.13 and
> tcpdump itself no longer works.
> Well, it works ... but not correctly.
> 
>  Capture data, then look for ICMP messages
>  (e.g. Time Exceeded errors as in a traceroute)
>  by filtering the file.
>  
>   tcpdump -w 1.cap
>   tcpdump -f "ip proto \icmp" -r 1.cap
> 
> That works.
> 
> 
>  Filter incoming data, looking for ICMP messages:
>  
>   tcpdump -f "ip proto \icmp"
>  
> Well, that catches nothing.
> 
> 
> I tried recompiling (source RPM, Fedora Core 2) tcpdump
> (libpcap, tcpdump, etc.) and reinstalling. That did not
> fix the problem with tcpdump.
> 
> It also broke a tethereal script I was using (which I changed
> to capture all packets, which works as indicated above, and
> then used a '-R', read, filter to display the one's I want).
> 

(cc netdev)

Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

[Herbert Xu]

Andrew Morton <akpm@osdl.org> wrote:
> 
>>  Filter incoming data, looking for ICMP messages:
>>  
>>   tcpdump -f "ip proto \icmp"
>>  
>> Well, that catches nothing.

We aren't handling the reading of specific fields like the IP protocol
field correctly.  This patch should make it work again.

I tried to move this logic into the new load_pointer function but it
all came out messy so I simply rolled it back.

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

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
--
diff --git a/net/core/filter.c b/net/core/filter.c
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -36,7 +36,7 @@
 #include <linux/filter.h>
 
 /* No hurry in this branch */
-static void *__load_pointer(struct sk_buff *skb, int k)
+static void *load_pointer(struct sk_buff *skb, int k)
 {
 	u8 *ptr = NULL;
 
@@ -50,18 +50,6 @@ static void *__load_pointer(struct sk_bu
 	return NULL;
 }
 
-static inline void *load_pointer(struct sk_buff *skb, int k,
-                                 unsigned int size, void *buffer)
-{
-	if (k >= 0)
-		return skb_header_pointer(skb, k, size, buffer);
-	else {
-		if (k >= SKF_AD_OFF)
-			return NULL;
-		return __load_pointer(skb, k);
-	}
-}
-
 /**
  *	sk_run_filter	- 	run a filter on a socket
  *	@skb: buffer to run the filter on
@@ -177,7 +165,13 @@ int sk_run_filter(struct sk_buff *skb, s
 		case BPF_LD|BPF_W|BPF_ABS:
 			k = fentry->k;
  load_w:
-			ptr = load_pointer(skb, k, 4, &tmp);
+			if (k >= 0)
+				ptr = skb_header_pointer(skb, k, 4, &tmp);
+			else if (k < SKF_AD_OFF)
+				ptr = load_pointer(skb, k);
+			else
+				break;
+
 			if (ptr != NULL) {
 				A = ntohl(*(u32 *)ptr);
 				continue;
@@ -186,7 +180,13 @@ int sk_run_filter(struct sk_buff *skb, s
 		case BPF_LD|BPF_H|BPF_ABS:
 			k = fentry->k;
  load_h:
-			ptr = load_pointer(skb, k, 2, &tmp);
+			if (k >= 0)
+				ptr = skb_header_pointer(skb, k, 2, &tmp);
+			else if (k < SKF_AD_OFF)
+				ptr = load_pointer(skb, k);
+			else
+				break;
+
 			if (ptr != NULL) {
 				A = ntohs(*(u16 *)ptr);
 				continue;
@@ -195,7 +195,13 @@ int sk_run_filter(struct sk_buff *skb, s
 		case BPF_LD|BPF_B|BPF_ABS:
 			k = fentry->k;
 load_b:
-			ptr = load_pointer(skb, k, 1, &tmp);
+			if (k >= 0)
+				ptr = skb_header_pointer(skb, k, 1, &tmp);
+			else if (k < SKF_AD_OFF)
+				ptr = load_pointer(skb, k);
+			else
+				break;
+
 			if (ptr != NULL) {
 				A = *(u8 *)ptr;
 				continue;
@@ -217,7 +223,9 @@ load_b:
 			k = X + fentry->k;
 			goto load_b;
 		case BPF_LDX|BPF_B|BPF_MSH:
-			ptr = load_pointer(skb, fentry->k, 1, &tmp);
+			if (fentry->k < 0)
+				return 0;
+			ptr = skb_header_pointer(skb, fentry->k, 1, &tmp);
 			if (ptr != NULL) {
 				X = (*(u8 *)ptr & 0xf) << 2;
 				continue;

Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

[Patrick McHardy]

Herbert Xu wrote:
> We aren't handling the reading of specific fields like the IP protocol
> field correctly.  This patch should make it work again.

I can't spot the problem, could you give me a hint?

> I tried to move this logic into the new load_pointer function but it
> all came out messy so I simply rolled it back.

  		case BPF_LD|BPF_W|BPF_ABS:
  			k = fentry->k;
   load_w:
-			ptr = load_pointer(skb, k, 4, &tmp);
+			if (k >= 0)
+				ptr = skb_header_pointer(skb, k, 4, &tmp);
+			else if (k < SKF_AD_OFF)
+				ptr = load_pointer(skb, k);
+			else
+				break;

The old value of ptr will be used in this case, it should
be explicitly set to NULL to abort.

Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

[Patrick McHardy]

Patrick McHardy wrote:
> Herbert Xu wrote:
> 
>> We aren't handling the reading of specific fields like the IP protocol
>> field correctly.  This patch should make it work again.
> 
> 
> I can't spot the problem, could you give me a hint?

Never mind, I got it, we never fall through to the second switch
statement anymore. I think we could simply break when load_pointer
returns NULL. The switch statement will fall through to the default
case and return 0 for all cases but 0 > k >= SKF_AD_OFF.

Re: Kernel 2.6.13 breaks libpcap (and tcpdump).

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
> 
> Never mind, I got it, we never fall through to the second switch
> statement anymore. I think we could simply break when load_pointer
> returns NULL. The switch statement will fall through to the default
> case and return 0 for all cases but 0 > k >= SKF_AD_OFF.

Thanks Patrick, that's a much better idea.  Here's a patch to do just
that.

I left BPF_MSH alone because it's really a hack to calculate the IP
header length, which makes no sense when applied to the special data.

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

BTW, you should be able to send me mail now.  Sorry about that.

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
--
diff --git a/net/core/filter.c b/net/core/filter.c
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -182,7 +182,7 @@ int sk_run_filter(struct sk_buff *skb, s
 				A = ntohl(*(u32 *)ptr);
 				continue;
 			}
-			return 0;
+			break;
 		case BPF_LD|BPF_H|BPF_ABS:
 			k = fentry->k;
  load_h:
@@ -191,7 +191,7 @@ int sk_run_filter(struct sk_buff *skb, s
 				A = ntohs(*(u16 *)ptr);
 				continue;
 			}
-			return 0;
+			break;
 		case BPF_LD|BPF_B|BPF_ABS:
 			k = fentry->k;
 load_b:
@@ -200,7 +200,7 @@ load_b:
 				A = *(u8 *)ptr;
 				continue;
 			}
-			return 0;
+			break;
 		case BPF_LD|BPF_W|BPF_LEN:
 			A = skb->len;
 			continue;