Netem does not work with loopback

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:

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

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

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

@@ -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 <[email protected]>

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

index e1a7dba..184c78c 100644
@@ -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);
 

OK.

Yes it does. Thank you very much.

Guillaume

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?

---
 net/ipv4/ip_input.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index e1a7dba..184c78c 100644
@@ -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

From: Stephen Hemminger <[email protected]>
Date: Fri, 14 Jul 2006 11:56:21 -0700

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.

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.

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

index 6ff9b10..0f9b3a3 100644
@@ -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)) {

index cbcae65..406056e 100644
@@ -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);

index 3291d51..76ab50b 100644
@@ -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;
 

index ba33f86..9ccacf5 100644
@@ -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;

index f8d880b..13cafbe 100644
@@ -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:

index c56aeec..836eecd 100644
@@ -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;

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

From: Stephen Hemminger <[email protected]>
Date: Sat, 15 Jul 2006 08:50:58 -0700

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

From: Herbert Xu <[email protected]>
Date: Sat, 15 Jul 2006 23:28:34 +1000

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

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.

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.

From: "chas williams - CONTRACTOR" <[email protected]>
Date: Sun, 16 Jul 2006 07:20:35 -0400

Yes, that's right, after hearing your and Herbert's comments, this point of view is clearly right :)

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

I agree, we should clear the whole IPCB.

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

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.

---

 ip6_input.c |    2 ++
 1 file changed, 2 insertions(+)

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

---

 ip_input.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

From: Guillaume Chazarain <[email protected]>
Date: Wed, 19 Jul 2006 14:35:15 +0200

Both patches applied, thanks Guillaume.

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

From: Herbert Xu <[email protected]>
Date: Mon, 17 Jul 2006 08:03:50 +1000

Ok, I'm convinced that IPv4 has to clear this out and shouldn't assume it's contents are anything but garbage.

It all sits in the same cacheline, so probably not.