Re: [PATCH] snap: needs hardware checksum fix

2006-03-10 Thread David S. Miller
From: Herbert Xu [EMAIL PROTECTED]
Date: Tue, 7 Feb 2006 22:21:37 +1100

 [NET]: Replace skb_pull/skb_postpull_rcsum with skb_pull_rcsum
 
 We're now starting to have quite a number of places that do skb_pull
 followed immediately by an skb_postpull_rcsum.  We can merge these
 two operations into one function with skb_pull_rcsum.  This makes
 sense since most pull operations on receive skb's need to update
 the checksum.
 
 I've decided to make this out-of-line since it is fairly big and the
 fast path where hardware checksums are enabled need to call csum_partial
 anyway.
 
 Since this is a brand new function we get to add an extra check on the
 len argument.  As it is most callers of skb_pull ignore its return value
 which essentially means that there is no check on the len argument.
 
 Signed-off-by: Herbert Xu [EMAIL PROTECTED]

Applied, thanks Herbert.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] snap: needs hardware checksum fix

2006-02-07 Thread Herbert Xu
On Fri, Feb 03, 2006 at 10:01:17AM -0800, Stephen Hemminger wrote:
 
 static unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len)
 {
   if (unlikely(len  skb-len))
   return NULL;
   if (skb-ip_summed == CHECKSUM_HW)
   skb-csum = csum_sub(skb-csum, csum_partial(skb-data, len, 
 0));
   return __skb_pull(skb, len);
 }

Thanks Stephen.  I've changed most of the places that call
skb_postpull_rcsum over to use this instead.  The only places
left are IPv6 where it makes sense to separate the checksum
update since it wants to pull in bits and pieces and update
at the very end.  The other place is GRE which is in fact
buggy with respect to the pulling (the bug was introduced
with the WCCP patch).  I'll send a separate patch for that.

[NET]: Replace skb_pull/skb_postpull_rcsum with skb_pull_rcsum

We're now starting to have quite a number of places that do skb_pull
followed immediately by an skb_postpull_rcsum.  We can merge these
two operations into one function with skb_pull_rcsum.  This makes
sense since most pull operations on receive skb's need to update
the checksum.

I've decided to make this out-of-line since it is fairly big and the
fast path where hardware checksums are enabled need to call csum_partial
anyway.

Since this is a brand new function we get to add an extra check on the
len argument.  As it is most callers of skb_pull ignore its return value
which essentially means that there is no check on the len argument.

Signed-off-by: Herbert Xu [EMAIL PROTECTED]

Cheers
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -1691,8 +1691,8 @@ ppp_receive_nonmp_frame(struct ppp *ppp,
|| ppp-npmode[npi] != NPMODE_PASS) {
kfree_skb(skb);
} else {
-   skb_pull(skb, 2);   /* chop off protocol */
-   skb_postpull_rcsum(skb, skb-data - 2, 2);
+   /* chop off protocol */
+   skb_pull_rcsum(skb, 2);
skb-dev = ppp-dev;
skb-protocol = htons(npindex_to_ethertype[npi]);
skb-mac.raw = skb-data;
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -337,8 +337,7 @@ static int pppoe_rcv_core(struct sock *s
if (sk-sk_state  PPPOX_BOUND) {
struct pppoe_hdr *ph = (struct pppoe_hdr *) skb-nh.raw;
int len = ntohs(ph-length);
-   skb_pull(skb, sizeof(struct pppoe_hdr));
-   skb_postpull_rcsum(skb, ph, sizeof(*ph));
+   skb_pull_rcsum(skb, sizeof(struct pppoe_hdr));
if (pskb_trim_rcsum(skb, len))
goto abort_kfree;
 
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1174,12 +1174,14 @@ static inline int skb_linearize(struct s
  */
 
 static inline void skb_postpull_rcsum(struct sk_buff *skb,
-const void *start, int len)
+ const void *start, unsigned int len)
 {
if (skb-ip_summed == CHECKSUM_HW)
skb-csum = csum_sub(skb-csum, csum_partial(start, len, 0));
 }
 
+unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
+
 /**
  * pskb_trim_rcsum - trim received skb and update checksum
  * @skb: buffer to trim
diff --git a/net/802/psnap.c b/net/802/psnap.c
--- a/net/802/psnap.c
+++ b/net/802/psnap.c
@@ -61,8 +61,7 @@ static int snap_rcv(struct sk_buff *skb,
/* Pass the frame on. */
u8 *hdr = skb-data;
skb-h.raw  += 5;
-   skb_pull(skb, 5);
-   skb_postpull_rcsum(skb, hdr, 5);
+   skb_pull_rcsum(skb, 5);
rc = proto-rcvfunc(skb, dev, snap_packet_type, orig_dev);
} else {
skb-sk = NULL;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -163,10 +163,8 @@ int vlan_skb_recv(struct sk_buff *skb, s
stats-rx_packets++;
stats-rx_bytes += skb-len;
 
-   skb_pull(skb, VLAN_HLEN); /* take off the VLAN header (4 bytes 
currently) */
-
-   /* Need to correct hardware checksum */
-   skb_postpull_rcsum(skb, vhdr, VLAN_HLEN);
+   /* Take off the VLAN header (4 bytes currently) */
+   skb_pull_rcsum(skb, VLAN_HLEN);
 
/* Ok, lets check to make sure the device (dev) we
 * came in on is what this VLAN is attached to.
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
--- a/net/bridge/br_netfilter.c
+++ 

Re: [PATCH] snap: needs hardware checksum fix

2006-02-03 Thread Stephen Hemminger
On Thu, 02 Feb 2006 22:24:27 -0800 (PST)
David S. Miller [EMAIL PROTECTED] wrote:

 From: Herbert Xu [EMAIL PROTECTED]
 Date: Fri, 03 Feb 2006 12:26:32 +1100
 
  David S. Miller [EMAIL PROTECTED] wrote:
   
   This patch made me notice that the length is sort of implicit
   or can be calculated given start and the current skb-data
   value.
   
   Someone might want to look into making that simplification
   at some point.
  
  Or we could simply merge skb_pull and skb_postpull_rcsum into one
  function that does both.
 
 True.  There aren't that many skb_postpull_rcsum() call sites
 at the moment so the changes should be quite easy.

static unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len)
{
if (unlikely(len  skb-len))
return NULL;
if (skb-ip_summed == CHECKSUM_HW)
skb-csum = csum_sub(skb-csum, csum_partial(skb-data, len, 
0));
return __skb_pull(skb, len);
}
-- 
Stephen Hemminger [EMAIL PROTECTED]
OSDL http://developer.osdl.org/~shemminger
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] snap: needs hardware checksum fix

2006-02-03 Thread Stephen Hemminger
On Thu, 02 Feb 2006 22:24:27 -0800 (PST)
David S. Miller [EMAIL PROTECTED] wrote:

 From: Herbert Xu [EMAIL PROTECTED]
 Date: Fri, 03 Feb 2006 12:26:32 +1100
 
  David S. Miller [EMAIL PROTECTED] wrote:
   
   This patch made me notice that the length is sort of implicit
   or can be calculated given start and the current skb-data
   value.
   
   Someone might want to look into making that simplification
   at some point.
  
  Or we could simply merge skb_pull and skb_postpull_rcsum into one
  function that does both.
 
 True.  There aren't that many skb_postpull_rcsum() call sites
 at the moment so the changes should be quite easy.

IMHO overloading CHECKSUM_HW with different semantics for transmit
and receive was a bad idea. When packets are going through bridging,
filtering, NAT, etc it can be really confusing.

It really should be separate CHECKSUM_HWTX and CHECKSUM_HWRX,

-- 
Stephen Hemminger [EMAIL PROTECTED]
OSDL http://developer.osdl.org/~shemminger
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html