netdev
[Top] [All Lists]

Re: [PATCH] Fix checksum bug for multicast/broadcast packets on postrout

To: Mika Penttilä <mika.penttila@xxxxxxxxxxx>
Subject: Re: [PATCH] Fix checksum bug for multicast/broadcast packets on postrouting hook
From: James Morris <jmorris@xxxxxxxxxx>
Date: Sun, 15 Feb 2004 01:09:23 -0500 (EST)
Cc: "David S. Miller" <davem@xxxxxxxxxx>, Harald Welte <laforge@xxxxxxxxxxxxx>, <netdev@xxxxxxxxxxx>, Stephen Smalley <sds@xxxxxxxxxxxxxx>
In-reply-to: <402E71E2.1040508@xxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
On Sat, 14 Feb 2004, Mika Penttilä wrote:

> James Morris wrote:
> 
> >The proposed solution below is to copy the skb rather than clone it, to 
> >ensure that the original and looped back packets are independent.
> >
>
> This is unneeded overhead for the common case. The right fix is to make 
> sure the modifier (netfilter etc) makes the copy if needed. Actually, 
> this is what skb_ip_make_writable() is doing.

The common case here will be only for locally generated multicast and 
broadcast packets.

If the netfilter core code is modified instead, we will end up adding
skb_ip_make_writable() to nf_hook_slow() which will be called for every 
packet with an output device which uses hardware checksums.

Not sure which is worse, but here's a proposed patch which does this.


- James
-- 
James Morris
<jmorris@xxxxxxxxxx>


diff -urN -X dontdiff linux-2.6.3-rc2-mm1.o/include/linux/netfilter.h 
linux-2.6.3-rc2-mm1.w2/include/linux/netfilter.h
--- linux-2.6.3-rc2-mm1.o/include/linux/netfilter.h     2004-02-03 
22:43:48.000000000 -0500
+++ linux-2.6.3-rc2-mm1.w2/include/linux/netfilter.h    2004-02-15 
00:16:30.000000000 -0500
@@ -162,6 +162,12 @@
 /* FIXME: Before cache is ever used, this must be implemented for real. */
 extern void nf_invalidate_cache(int pf);
 
+/* Call this before modifying an existing IP packet: ensures it is
+   modifiable and linear to the point you care about (writable_len).
+   Returns true or false. */
+extern int skb_ip_make_writable(struct sk_buff **pskb,
+                               unsigned int writable_len);
+
 #else /* !CONFIG_NETFILTER */
 #define NF_HOOK(pf, hook, skb, indev, outdev, okfn) (okfn)(skb)
 #endif /*CONFIG_NETFILTER*/
diff -urN -X dontdiff linux-2.6.3-rc2-mm1.o/include/linux/netfilter_ipv4.h 
linux-2.6.3-rc2-mm1.w2/include/linux/netfilter_ipv4.h
--- linux-2.6.3-rc2-mm1.o/include/linux/netfilter_ipv4.h        2004-02-03 
22:44:26.000000000 -0500
+++ linux-2.6.3-rc2-mm1.w2/include/linux/netfilter_ipv4.h       2004-02-15 
00:16:13.000000000 -0500
@@ -78,11 +78,6 @@
 
 extern int ip_route_me_harder(struct sk_buff **pskb);
 
-/* Call this before modifying an existing IP packet: ensures it is
-   modifiable and linear to the point you care about (writable_len).
-   Returns true or false. */
-extern int skb_ip_make_writable(struct sk_buff **pskb,
-                               unsigned int writable_len);
 #endif /*__KERNEL__*/
 
 #endif /*__LINUX_IP_NETFILTER_H*/
diff -urN -X dontdiff linux-2.6.3-rc2-mm1.o/net/core/netfilter.c 
linux-2.6.3-rc2-mm1.w2/net/core/netfilter.c
--- linux-2.6.3-rc2-mm1.o/net/core/netfilter.c  2004-02-03 22:43:46.000000000 
-0500
+++ linux-2.6.3-rc2-mm1.w2/net/core/netfilter.c 2004-02-15 00:57:05.000000000 
-0500
@@ -509,6 +509,13 @@
                if (outdev == NULL) {
                        skb->ip_summed = CHECKSUM_NONE;
                } else {
+                       int wlen = skb->h.raw - skb->data + skb->csum;
+                       
+                       /* packet is about to be modified */
+                       if (!skb_ip_make_writable(&skb, wlen)) {
+                               kfree(skb);
+                               return -ENOMEM;
+                       }
                        skb_checksum_help(skb);
                }
        }



<Prev in Thread] Current Thread [Next in Thread>