Netem does not work with loopback

Netem does not work with loopback

[Guillaume Chazarain]

Hello,

Is it a known problem that the Netem qdisc is very unreliable on the 
loopback (unlike on a true NIC)?

This seems to come from its usage of skb->cb which conflicts with IPCB. 
For instance, the field
IPCB(skb)->opt.optlen becomes non-null and memory corruption follows.

I ``worked around'' this conflict with the following ugly hack:

--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -79,6 +79,7 @@ struct netem_sched_data {
 
 /* Time stamp put into socket buffer control block */
 struct netem_skb_cb {
+    char padding[32]; /* to avoid stepping over IPCB when used on the 
loopback */
     psched_time_t    time_to_send;
 };


Hopefully someone has a better solution :-)

Thanks.

-- 
Guillaume

Re: Netem does not work with loopback

[Stephen Hemminger]

On Fri, 14 Jul 2006 16:34:30 +0200
Guillaume Chazarain <guichaz@yahoo.fr> wrote:

> Hello,
> 
> Is it a known problem that the Netem qdisc is very unreliable on the 
> loopback (unlike on a true NIC)?
> 
> This seems to come from its usage of skb->cb which conflicts with IPCB. 
> For instance, the field
> IPCB(skb)->opt.optlen becomes non-null and memory corruption follows.
> 
> I ``worked around'' this conflict with the following ugly hack:

Isn't the skb owned by netem at that point...

-- 
If one would give me six lines written by the hand of the most honest
man, I would find something in them to have him hanged. -- Cardinal Richlieu

Re: Netem does not work with loopback

[Guillaume Chazarain]

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

Stephen Hemminger wrote :
> Isn't the skb owned by netem at that point...
>   
Thank you, that makes sense to me.
But then the loopback device reinjects the skb without cleaning skb->cb.

The attached patch solves the problem for me (netem on lo). Maybe the
correct solution would be to make a skb->destructor function for netem,
but I am not sure this is the intended use of a destructor function.

Thanks.

-- 
Guillaume


[-- Attachment #2: loopback_zero_cb.diff --]
[-- Type: text/x-patch, Size: 344 bytes --]

--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -156,6 +156,9 @@
 	lb_stats->tx_packets = lb_stats->rx_packets;
 	put_cpu();
 
+	/* Get rid of all the private data accumulated in the sending part */
+	memset(skb->cb, 0, sizeof(skb->cb));
+
 	netif_rx(skb);
 
 	return(0);

-- 
Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>

Re: Netem does not work with loopback

[Stephen Hemminger]

On Fri, 14 Jul 2006 19:04:01 +0200
Guillaume Chazarain <guichaz@yahoo.fr> wrote:

> Stephen Hemminger wrote :
> > Isn't the skb owned by netem at that point...
> >   
> Thank you, that makes sense to me.
> But then the loopback device reinjects the skb without cleaning skb->cb.
> 
> The attached patch solves the problem for me (netem on lo). Maybe the
> correct solution would be to make a skb->destructor function for netem,
> but I am not sure this is the intended use of a destructor function.
> 

No, the correct fix is to make IP input not accept any options from the
device.  Does this work?


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);
 

Re: Netem does not work with loopback

[Guillaume Chazarain]

Stephen Hemminger wrote:
> No, the correct fix is to make IP input not accept any options from the
> device.
OK.
>   Does this work?
>   
Yes it does. Thank you very much.

-- 
Guillaume

[PATCH] clear skb cb on IP input

[Stephen Hemminger]

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 <shemminger@osdl.org>
---
 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

Re: [PATCH] clear skb cb on IP input

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
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 <shemminger@osdl.org>

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.

Re: [PATCH] clear skb cb on IP input

[Herbert Xu]

David Miller <davem@davemloft.net> 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 <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/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;

Re: [PATCH] clear skb cb on IP input

[Stephen Hemminger]

Herbert Xu wrote:
> David Miller <davem@davemloft.net> 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 <herbert@gondor.apana.org.au>
>
> Cheers
>   

Since skb->cb is aligned, we could optimize the initialization
slightly by just using:
    *(unsigned long *)skb->cb = 0;

Re: [PATCH] clear skb cb on IP input

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
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 :)

Re: [PATCH] clear skb cb on IP input

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
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 <herbert@gondor.apana.org.au>

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...

Re: [PATCH] clear skb cb on IP input

[chas williams - CONTRACTOR]

In message <20060715.181222.74751444.davem@davemloft.net>,David Miller writes:
>From: Herbert Xu <herbert@gondor.apana.org.au>
>> 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.

Re: [PATCH] clear skb cb on IP input

[David Miller]

From: "chas williams - CONTRACTOR" <chas@cmf.nrl.navy.mil>
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 :)

Re: [PATCH] clear skb cb on IP input

[Guillaume Chazarain]

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

Re: [PATCH] clear skb cb on IP input

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] clear skb cb on IP input

[Guillaume Chazarain]

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

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


[-- Attachment #2: clear_ip6cb.diff --]
[-- Type: text/x-patch, Size: 489 bytes --]

Clear the accumulated junk in IP6CB when starting to handle an
IPV6 packet.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 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


[-- Attachment #3: clear_whole_ipcb.diff --]
[-- Type: text/x-patch, Size: 563 bytes --]

Clear the whole IPCB, this clears also IPCB(skb)->flags.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 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

[David Miller]

From: Guillaume Chazarain <guichaz@yahoo.fr>
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.

Re: [PATCH] clear skb cb on IP input

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] clear skb cb on IP input

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
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.