Re: [PATCH] clear skb cb on IP input
From: Guillaume Chazarain <[EMAIL PROTECTED]> Date: Wed, 19 Jul 2006 14:35:15 +0200 > Herbert Xu wrote : > > Probably. Patches are welcome :) > Here are they, in both case I checked that the stuff to clear > was not already cleared, but I could not produce any misbehavior > by writing random junk instead of clearing the data. All my tests > were on the loopback using UML. > > For IPv4, the added safety seems worth, but for IPv6 it's less clear. Both patches applied, thanks Guillaume. - 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] clear skb cb on IP input
Herbert Xu wrote : Probably. Patches are welcome :) Here are they, in both case I checked that the stuff to clear was not already cleared, but I could not produce any misbehavior by writing random junk instead of clearing the data. All my tests were on the loopback using UML. For IPv4, the added safety seems worth, but for IPv6 it's less clear. Thanks. -- Guillaume Clear the accumulated junk in IP6CB when starting to handle an IPV6 packet. Signed-off-by: Guillaume Chazarain <[EMAIL PROTECTED]> --- ip6_input.c |2 ++ 1 file changed, 2 insertions(+) --- a/net/ipv6/ip6_input.c +++ b/net/ipv6/ip6_input.c @@ -70,6 +70,8 @@ int ipv6_rcv(struct sk_buff *skb, struct IP6_INC_STATS_BH(IPSTATS_MIB_INDISCARDS); goto out; } + + memset(IP6CB(skb), 0, sizeof(struct inet6_skb_parm)); /* * Store incoming device index. When the packet will Clear the whole IPCB, this clears also IPCB(skb)->flags. Signed-off-by: Guillaume Chazarain <[EMAIL PROTECTED]> --- ip_input.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -429,7 +429,7 @@ int ip_rcv(struct sk_buff *skb, struct n } /* Remove any debris in the socket control block */ - memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options)); + memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); return NF_HOOK(PF_INET, NF_IP_PRE_ROUTING, skb, dev, NULL, ip_rcv_finish);
Re: [PATCH] clear skb cb on IP input
On Tue, Jul 18, 2006 at 08:19:34PM +0200, Guillaume Chazarain wrote:
>
> Why not clearing the whole IPCB(skb) instead of
> just IPCB(skb)->opts? that would also clear
> IPCB(skb)->flags.
I agree, we should clear the whole IPCB.
> And, does not ipv6 need the same treatment with
> IP6CB?
Probably. Patches are welcome :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
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] clear skb cb on IP input
chas williams - CONTRACTOR wrote: why does the input side of the ip layer believe the cb contains valid data? it should zero the contents of the cb, or just fill in the cb correctly when the packet arrives at its doorstop. Why not clearing the whole IPCB(skb) instead of just IPCB(skb)->opts? that would also clear IPCB(skb)->flags. And, does not ipv6 need the same treatment with IP6CB? Regards. -- Guillaume - 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] clear skb cb on IP input
From: "chas williams - CONTRACTOR" <[EMAIL PROTECTED]> Date: Sun, 16 Jul 2006 07:20:35 -0400 > why does the input side of the ip layer believe the cb contains valid > data? it should zero the contents of the cb, or just fill in the cb > correctly when the packet arrives at its doorstop. Yes, that's right, after hearing your and Herbert's comments, this point of view is clearly right :) - 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] clear skb cb on IP input
From: Herbert Xu <[EMAIL PROTECTED]> Date: Mon, 17 Jul 2006 08:03:50 +1000 > The thing is qdiscs using cb means that this method of clearing cb > before netif_rx doesn't work anymore. > > In particular, even if loopback clears cb before calling netif_rx, > some qdisc could come along between netif_rx and ip_rcv and put > stuff in the cb. > > The same thing can happen to any NIC in fact, as long as we allow > qdiscs to use the cb area without clearing it, ip_rcv needs to > clear it itself. Ok, I'm convinced that IPv4 has to clear this out and shouldn't assume it's contents are anything but garbage. > With a little bit of effort we should be able to get away with > clearing just optlen. Whether this effort is worthwhile I don't > know :) It all sits in the same cacheline, so probably not. - 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] clear skb cb on IP input
On Sat, Jul 15, 2006 at 06:12:22PM -0700, David Miller wrote:
>
> But I'm beginning to think that the onus of this may in fact fall upon
> the devices, in fact. Loopback is one of the few devices where the
> control block might not be cleared out, due to uses in the output
> path. Devices predominantly provide a zero'd out control block in the
> skb on packet receive.
The thing is qdiscs using cb means that this method of clearing cb
before netif_rx doesn't work anymore.
In particular, even if loopback clears cb before calling netif_rx,
some qdisc could come along between netif_rx and ip_rcv and put
stuff in the cb.
The same thing can happen to any NIC in fact, as long as we allow
qdiscs to use the cb area without clearing it, ip_rcv needs to
clear it itself.
With a little bit of effort we should be able to get away with
clearing just optlen. Whether this effort is worthwhile I don't
know :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
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] clear skb cb on IP input
In message <[EMAIL PROTECTED]>,David Miller writes: >From: Herbert Xu <[EMAIL PROTECTED]> >> At least this lets us get rid of a few other memsets :) >> >Applied, good spotting :-) I remember when we added those >things. > >But I'm beginning to think that the onus of this may in fact fall upon >the devices, in fact. Loopback is one of the few devices where the >control block might not be cleared out, due to uses in the output >path. Devices predominantly provide a zero'd out control block in the >skb on packet receive. the atm layer has the same problem. some atm device drivers use the skb cb field, so it needs to be zero'ed by the next upper layer (clip, lane) before being passed to the ip layer. its also possible that the atm layer should clone the skb before passing to the next layer which would also zeroize the cb. >Other opinions welcome... why does the input side of the ip layer believe the cb contains valid data? it should zero the contents of the cb, or just fill in the cb correctly when the packet arrives at its doorstop. - 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] clear skb cb on IP input
From: Stephen Hemminger <[EMAIL PROTECTED]> Date: Sat, 15 Jul 2006 08:50:58 -0700 > Since skb->cb is aligned, we could optimize the initialization > slightly by just using: > *(unsigned long *)skb->cb = 0; Well, that depends upon two things. 1) How much of the ip_options really needs to be zero'd out initially. From what I can tell, all 3 words really need it. 2) How big is an unsigned long on your computer? It can be 1 or 2 words, but not three :) - 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] clear skb cb on IP input
From: Herbert Xu <[EMAIL PROTECTED]> Date: Sat, 15 Jul 2006 23:28:34 +1000 > At least this lets us get rid of a few other memsets :) > > [IPV4]: Get rid of redundant IPCB->opts initialisation > > Now that we always zero the IPCB->opts in ip_rcv, it is no longer > necessary to do so before calling netif_rx for tunneled packets. > > Signed-off-by: Herbert Xu <[EMAIL PROTECTED]> Applied, good spotting :-) I remember when we added those things. But I'm beginning to think that the onus of this may in fact fall upon the devices, in fact. Loopback is one of the few devices where the control block might not be cleared out, due to uses in the output path. Devices predominantly provide a zero'd out control block in the skb on packet receive. And the memset() cases we're removing here with Herbert's change are the other exceptional cases, such as tunnels that decapsulate the IPV4 protocol. Other opinions welcome... - 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] clear skb cb on IP input
Herbert Xu wrote: David Miller <[EMAIL PROTECTED]> wrote: Thank goodness this thing is only 3-words in size, this is going to run on every single IPv4 packet received by the system. :-/ At least this lets us get rid of a few other memsets :) [IPV4]: Get rid of redundant IPCB->opts initialisation Now that we always zero the IPCB->opts in ip_rcv, it is no longer necessary to do so before calling netif_rx for tunneled packets. Signed-off-by: Herbert Xu <[EMAIL PROTECTED]> Cheers Since skb->cb is aligned, we could optimize the initialization slightly by just using: *(unsigned long *)skb->cb = 0; - 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] clear skb cb on IP input
David Miller <[EMAIL PROTECTED]> wrote:
>
> Thank goodness this thing is only 3-words in size, this is going to
> run on every single IPv4 packet received by the system. :-/
At least this lets us get rid of a few other memsets :)
[IPV4]: Get rid of redundant IPCB->opts initialisation
Now that we always zero the IPCB->opts in ip_rcv, it is no longer
necessary to do so before calling netif_rx for tunneled packets.
Signed-off-by: Herbert Xu <[EMAIL PROTECTED]>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6ff9b10..0f9b3a3 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -617,7 +617,6 @@ static int ipgre_rcv(struct sk_buff *skb
skb->mac.raw = skb->nh.raw;
skb->nh.raw = __pskb_pull(skb, offset);
skb_postpull_rcsum(skb, skb->h.raw, offset);
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
skb->pkt_type = PACKET_HOST;
#ifdef CONFIG_NET_IPGRE_BROADCAST
if (MULTICAST(iph->daddr)) {
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index cbcae65..406056e 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -256,7 +256,6 @@ int ip_options_compile(struct ip_options
if (!opt) {
opt = &(IPCB(skb)->opt);
- memset(opt, 0, sizeof(struct ip_options));
iph = skb->nh.raw;
opt->optlen = ((struct iphdr *)iph)->ihl*4 - sizeof(struct
iphdr);
optptr = iph + sizeof(struct iphdr);
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 3291d51..76ab50b 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -487,7 +487,6 @@ static int ipip_rcv(struct sk_buff *skb)
skb->mac.raw = skb->nh.raw;
skb->nh.raw = skb->data;
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
skb->protocol = htons(ETH_P_IP);
skb->pkt_type = PACKET_HOST;
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index ba33f86..9ccacf5 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1461,7 +1461,6 @@ int pim_rcv_v1(struct sk_buff * skb)
skb_pull(skb, (u8*)encap - skb->data);
skb->nh.iph = (struct iphdr *)skb->data;
skb->dev = reg_dev;
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
skb->protocol = htons(ETH_P_IP);
skb->ip_summed = 0;
skb->pkt_type = PACKET_HOST;
@@ -1517,7 +1516,6 @@ static int pim_rcv(struct sk_buff * skb)
skb_pull(skb, (u8*)encap - skb->data);
skb->nh.iph = (struct iphdr *)skb->data;
skb->dev = reg_dev;
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
skb->protocol = htons(ETH_P_IP);
skb->ip_summed = 0;
skb->pkt_type = PACKET_HOST;
diff --git a/net/ipv4/xfrm4_mode_tunnel.c b/net/ipv4/xfrm4_mode_tunnel.c
index f8d880b..13cafbe 100644
--- a/net/ipv4/xfrm4_mode_tunnel.c
+++ b/net/ipv4/xfrm4_mode_tunnel.c
@@ -92,7 +92,6 @@ static int xfrm4_tunnel_input(struct xfr
skb->mac.raw = memmove(skb->data - skb->mac_len,
skb->mac.raw, skb->mac_len);
skb->nh.raw = skb->data;
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
err = 0;
out:
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index c56aeec..836eecd 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -380,7 +380,6 @@ static int ipip6_rcv(struct sk_buff *skb
secpath_reset(skb);
skb->mac.raw = skb->nh.raw;
skb->nh.raw = skb->data;
- memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
IPCB(skb)->flags = 0;
skb->protocol = htons(ETH_P_IPV6);
skb->pkt_type = PACKET_HOST;
-
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] clear skb cb on IP input
From: Stephen Hemminger <[EMAIL PROTECTED]> Date: Fri, 14 Jul 2006 11:56:21 -0700 > when data arrives at IP through loopback (and possibly other devices). > So the field needs to be cleared before it confuses the route code. > This was seen when running netem over loopback, but there are probably > other device cases. Maybe this should go into stable? > > Signed-off-by: Stephen Hemminger <[EMAIL PROTECTED]> Thank goodness this thing is only 3-words in size, this is going to run on every single IPv4 packet received by the system. :-/ I'll apply this, thanks a lot Stephen. - 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
[PATCH] clear skb cb on IP input
when data arrives at IP through loopback (and possibly other devices). So the field needs to be cleared before it confuses the route code. This was seen when running netem over loopback, but there are probably other device cases. Maybe this should go into stable? Signed-off-by: Stephen Hemminger <[EMAIL PROTECTED]> --- net/ipv4/ip_input.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c index e1a7dba..184c78c 100644 --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -428,6 +428,9 @@ int ip_rcv(struct sk_buff *skb, struct n goto drop; } + /* Remove any debris in the socket control block */ + memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options)); + return NF_HOOK(PF_INET, NF_IP_PRE_ROUTING, skb, dev, NULL, ip_rcv_finish); -- 1.4.0 - 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
