[PATCH 1/3] sit: ipip6_err: ICMP_PORT_UNREACH is a possible event

[PATCH 1/3] sit: ipip6_err: ICMP_PORT_UNREACH is a possible event

[Sascha Hlusiak]

Linux sends ICMP_DEST_UNREACH + ICMP_PORT_UNREACH itself in ipip6_rcv,
if no matching tunnel is found.

Signed-off-by: Sascha Hlusiak <contact@saschahlusiak.de>
---
 net/ipv6/sit.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index b3a59bd..ba74094 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -494,7 +494,6 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 	case ICMP_DEST_UNREACH:
 		switch (code) {
 		case ICMP_SR_FAILED:
-		case ICMP_PORT_UNREACH:
 			/* Impossible event. */
 			return 0;
 		case ICMP_FRAG_NEEDED:
-- 
1.6.3.1

[PATCH 2/3] sit: handle ICMPv4 for tunnels with no remote

[Sascha Hlusiak]

Don't drop ICMPv4 packages, if tunnel has no remote endpoint, like
6to4 or isatap tunnels.

Also don't drop it if tunnel inherits ttl and icmp_time_exceeded.
Don't really see the usecase here.

Signed-off-by: Sascha Hlusiak <contact@saschahlusiak.de>
---
 net/ipv6/sit.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index ba74094..89d8369 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -520,12 +520,10 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 				skb->dev,
 				iph->daddr,
 				iph->saddr);
-	if (t == NULL || t->parms.iph.daddr == 0)
+	if (t == NULL)
 		goto out;
 
 	err = 0;
-	if (t->parms.iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
-		goto out;
 
 	if (time_before(jiffies, t->err_time + IPTUNNEL_ERR_TIMEO))
 		t->err_count++;
-- 
1.6.3.1

[PATCH 3/3] sit: Translate ICMPv4 errors to ICMPv6, if possible

[Sascha Hlusiak]

This partly reimplements, what was removed in 071f92d05.
If we receive enough ICMPv4 payload for an IPv6 header,
send the payload into the tunnel encapsulated in ICMPv6.

If we receive less payload and tunnel has a remote endpoint,
increase tunnel error count, resulting in a dst_link_failure.

If tunnel has no remote (6to4, isatap), we can't do anything
useful with the data, so let's drop it.

Signed-off-by: Sascha Hlusiak <contact@saschahlusiak.de>
---
 net/ipv6/sit.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 89d8369..c604b59 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -16,6 +16,7 @@
  * Nate Thompson <nate@thebog.net>:		6to4 support
  * Fred Templin <fred.l.templin@boeing.com>:	isatap support
  * Sascha Hlusiak <mail@saschahlusiak.de>:	stateless autoconf for isatap
+ *                                              better ICMPv4 handling
  */
 
 #include <linux/module.h>
@@ -475,16 +476,15 @@ static void ipip6_tunnel_uninit(struct net_device *dev)
 
 static int ipip6_err(struct sk_buff *skb, u32 info)
 {
-
-/* All the routers (except for Linux) return only
-   8 bytes of packet payload. It means, that precise relaying of
-   ICMP in the real Internet is absolutely infeasible.
- */
 	struct iphdr *iph = (struct iphdr*)skb->data;
 	const int type = icmp_hdr(skb)->type;
 	const int code = icmp_hdr(skb)->code;
 	struct ip_tunnel *t;
 	int err;
+	const int hlen = iph->ihl << 2;
+	int rel_type = 0;
+	int rel_code = 0;
+	int rel_info = 0;
 
 	switch (type) {
 	default:
@@ -504,12 +504,16 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 			   rfc2003 contains "deep thoughts" about NET_UNREACH,
 			   I believe they are just ether pollution. --ANK
 			 */
+			rel_type = ICMPV6_DEST_UNREACH;
+			rel_code = ICMPV6_ADDR_UNREACH;
 			break;
 		}
 		break;
 	case ICMP_TIME_EXCEEDED:
 		if (code != ICMP_EXC_TTL)
 			return 0;
+		rel_type = ICMPV6_TIME_EXCEED;
+		rel_code = ICMPV6_EXC_HOPLIMIT;
 		break;
 	}
 
@@ -524,12 +528,35 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 		goto out;
 
 	err = 0;
-
-	if (time_before(jiffies, t->err_time + IPTUNNEL_ERR_TIMEO))
-		t->err_count++;
-	else
-		t->err_count = 1;
-	t->err_time = jiffies;
+	/* Most routers return only 8 bytes of packet payload.
+	   It means, that precise relaying of ICMP in the real
+	   Internet is absolutely infeasible
+
+	   If we get rfc1812 compliant ICMP packages, handle them properly,
+	   otherwise set tunnel error state, _if_ it has a remote endpoint.
+	   If it does not (6to4, isatap), possibly unrelated ICMPv6 messages
+	   might be sent in ipip6_tunnel_xmit, so better ignore it.
+	*/
+	if (skb->len - hlen >= sizeof(struct ipv6hdr)) {
+		struct ipv6hdr *iph6 = (struct ipv6hdr *)((u8*)iph + hlen);
+		struct sk_buff *skb2;
+
+		skb2 = skb_clone(skb, GFP_ATOMIC);
+		if (skb2 == NULL)
+			goto out;
+		skb_pull(skb2, (u8 *)iph6 - skb->data);
+		skb_reset_network_header(skb2);
+		skb2->dev = t->dev;
+		icmpv6_send(skb2, rel_type, rel_code, rel_info, skb2->dev);
+
+		kfree_skb(skb2);
+	} else if (t->parms.iph.daddr) {
+		if (time_before(jiffies, t->err_time + IPTUNNEL_ERR_TIMEO))
+			t->err_count++;
+		else
+			t->err_count = 1;
+		t->err_time = jiffies;
+	}
 out:
 	read_unlock(&ipip6_lock);
 	return err;
-- 
1.6.3.1

Re: [PATCH 2/3] sit: handle ICMPv4 for tunnels with no remote

[David Miller]

From: Sascha Hlusiak <contact@saschahlusiak.de>
Date: Sat,  6 Jun 2009 16:02:51 +0200

> Don't drop ICMPv4 packages, if tunnel has no remote endpoint, like
> 6to4 or isatap tunnels.
> 
> Also don't drop it if tunnel inherits ttl and icmp_time_exceeded.
> Don't really see the usecase here.
> 
> Signed-off-by: Sascha Hlusiak <contact@saschahlusiak.de>

You're going to have to describe this change a bit more.

What in the world is wrong with dropping an ICMP time exceeded if the
TTL in the template header is zero?  Someone wrote that code and had
some reason to put it there.   Until you know the reason or can prove
without a shadow of doubt that it's pointless, you can't just remove
it.

You other two changes need more verbose commit messages as well.
I can't tell what the point or benefit are of your changes.

So I'm dropping your patches until all of this is resolved, and
the commit messages are more verbose and explain why you're doing
all of these things.

Re: [PATCH 2/3] sit: handle ICMPv4 for tunnels with no remote

[mail]

Zitat von David Miller <davem@davemloft.net>:

> From: Sascha Hlusiak <contact@saschahlusiak.de>
> Date: Sat,  6 Jun 2009 16:02:51 +0200
>
>> Don't drop ICMPv4 packages, if tunnel has no remote endpoint, like
>> 6to4 or isatap tunnels.
>>
>> Also don't drop it if tunnel inherits ttl and icmp_time_exceeded.
>> Don't really see the usecase here.
>>
>> Signed-off-by: Sascha Hlusiak <contact@saschahlusiak.de>
>
> You're going to have to describe this change a bit more.
My primacy concern with the patchset is in the usecase of ISATAP or
6to4, but I know this might heavily affect static tunnels as well.

In case an ISATAP neighbour is unreachable, an ICMPv4 error will be
sent to the tunnel and without my patches it will be discarded. The
3rd patch will re-add the code to treat ICMPv4 errors as link errors
and send ICMPv6 errors back to the sending application so it can be
handled. Without it, the application won't get any ICMPv6 messages
and it will timeout in case of link(=IPv4) errors. An ICMPv4
unreachable should result in an ICMPv6 unreachable in the case of ISATAP.

> What in the world is wrong with dropping an ICMP time exceeded if the
> TTL in the template header is zero?  Someone wrote that code and had
> some reason to put it there.   Until you know the reason or can prove
> without a shadow of doubt that it's pointless, you can't just remove
> it.
If iph.ttl == 0, it means that the ttl of the ipv4 package is inherited
from the ipv6 package to be sent. In case the ipv4 ttl of the path
expires, the ipv6 ttl would expire too. While the link does send a
time_exceeded, I believe that the application should receive an ICMPv6
time_exceeded as well, even if the package died right on the tunnel
endpoint.

In case we cannot find the application, we can discard it because it's
not a link failure. That case should still be handled, you are right.

> You other two changes need more verbose commit messages as well.
> I can't tell what the point or benefit are of your changes.
If we receive enough payload in the ICMPv4 error, I think
it's a good idea to pass the error on as an ICMPv6 to the sender. The
current implementation is to mark the whole link faulty for 30 seconds,
if it's a directed tunnel (iph.daddr != 0).

> So I'm dropping your patches until all of this is resolved, and
> the commit messages are more verbose and explain why you're doing
> all of these things.
I basically want to partly revert 071f92d05967a0c8422f1c8587ce0b4d90a8b447
which removed the desired behaviour that was activated with
I_WISH_WORLD_WERE_PERFECT. In case you agree that it's a good idea, I'll
prepare a new patchset.

Thanks,
Sascha

Re: [PATCH 2/3] sit: handle ICMPv4 for tunnels with no remote

[Patrick McHardy]

mail@saschahlusiak.de wrote:
> If iph.ttl == 0, it means that the ttl of the ipv4 package is inherited
> from the ipv6 package to be sent. In case the ipv4 ttl of the path
> expires, the ipv6 ttl would expire too. While the link does send a
> time_exceeded, I believe that the application should receive an ICMPv6
> time_exceeded as well, even if the package died right on the tunnel
> endpoint.

I have some patches that I wanted to post tommorrow, which will allow
to propagate errno values and queue congestion state of the underlying
device upwards from the hard_start_xmit() functions of virtual network
devices. With these patches, you can simply return -EHOSTUNREACH or
whatever is appropriate and it will be delivered to the application.

Would that help?