[PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[Hangbin Liu]

After call ip6_tnl_err(), the rel_type will be ether ICMPV6_DEST_UNREACH
or ICMPV6_PKT_TOOBIG. We will never reach ICMP_REDIRECT. So remove it.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/ip6_tunnel.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6eb2ae5..16f8d42 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -591,9 +591,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		rel_type = ICMP_DEST_UNREACH;
 		rel_code = ICMP_FRAG_NEEDED;
 		break;
-	case NDISC_REDIRECT:
-		rel_type = ICMP_REDIRECT;
-		rel_code = ICMP_REDIR_HOST;
 	default:
 		return 0;
 	}
@@ -652,8 +649,6 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 		skb_dst(skb2)->ops->update_pmtu(skb_dst(skb2), NULL, skb2, rel_info);
 	}
-	if (rel_type == ICMP_REDIRECT)
-		skb_dst(skb2)->ops->redirect(skb_dst(skb2), NULL, skb2);
 
 	icmp_send(skb2, rel_type, rel_code, htonl(rel_info));
 
-- 
2.5.5

Re: [PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Mon,  8 May 2017 19:11:03 +0800

> After call ip6_tnl_err(), the rel_type will be ether ICMPV6_DEST_UNREACH
> or ICMPV6_PKT_TOOBIG. We will never reach ICMP_REDIRECT. So remove it.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Your patches here, all seemingly due to "visual inspection", are starting
to really, truly, irritate me.

Half of them are unnecessary, some are completely buggy.

I have to review them and audit them very satrictly as a result, and
this takes up an unnecessarily large amount of my time.

Therefore, I'm basically going to stop looking at your changes _unless_
you can show that you specifically tested and exercised all of the code
paths you are changing.

I am sorry to have to do this, but the value to effort ratio of
reviewing and integrating your changes is quite poor.

Re: [PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[Cong Wang]

On Mon, May 8, 2017 at 4:11 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> After call ip6_tnl_err(), the rel_type will be ether ICMPV6_DEST_UNREACH
> or ICMPV6_PKT_TOOBIG. We will never reach ICMP_REDIRECT. So remove it.

Are you sure we really don't need to handle NDISC_REDIRECT here?

I can't find anything in RFC 2473 explictly, but I am feeling we should handle
it rather than ignoring it according to:

   To report a problem detected inside the tunnel to the source of an
   original packet, the tunnel entry point node must relay the ICMP
   message received from inside the tunnel to the source of that
   original IPv6 packet.

I am not sure...

Re: [PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[Hangbin Liu]

2017-05-09 3:59 GMT+08:00 David Miller <davem@davemloft.net>:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Mon,  8 May 2017 19:11:03 +0800
>
>> After call ip6_tnl_err(), the rel_type will be ether ICMPV6_DEST_UNREACH
>> or ICMPV6_PKT_TOOBIG. We will never reach ICMP_REDIRECT. So remove it.
>>
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>
> Your patches here, all seemingly due to "visual inspection", are starting
> to really, truly, irritate me.

Yes, this is based on the coverity scan result.  I'm really sorry to make
you feel uncomfortable and I apologize for this.  I trust the result too much
and did not do enough research. I will try my best to make sure the patch
really improve or fix something next time.

Sorry again.

Regards
Hangbin

>
> Half of them are unnecessary, some are completely buggy.
>
> I have to review them and audit them very satrictly as a result, and
> this takes up an unnecessarily large amount of my time.
>
> Therefore, I'm basically going to stop looking at your changes _unless_
> you can show that you specifically tested and exercised all of the code
> paths you are changing.
>
> I am sorry to have to do this, but the value to effort ratio of
> reviewing and integrating your changes is quite poor.

Re: [PATCH net] ip6_tunnel: remove unreachable ICMP_REDIRECT code

[Hangbin Liu]

On Mon, May 08, 2017 at 01:26:48PM -0700, Cong Wang wrote:
> On Mon, May 8, 2017 at 4:11 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> > After call ip6_tnl_err(), the rel_type will be ether ICMPV6_DEST_UNREACH
> > or ICMPV6_PKT_TOOBIG. We will never reach ICMP_REDIRECT. So remove it.
> 
> Are you sure we really don't need to handle NDISC_REDIRECT here?

Hi Cong,

I have no intend to remove any handler if we need it.

Just from the code path, after call ip6_tnl_err() without error, the rel_type
will be set to either ICMPV6_DEST_UNREACH or ICMPV6_PKT_TOOBIG. Which mean the
NDISC_REDIRECT check will never be reached. That's the reason I removed it.

So if we still want to handle it, I think we need a check in ip6_tnl_err().

Please correct me if I missed anything. You know I'm a fresher here.
> 
> I can't find anything in RFC 2473 explictly, but I am feeling we should handle
> it rather than ignoring it according to:
> 
>    To report a problem detected inside the tunnel to the source of an
>    original packet, the tunnel entry point node must relay the ICMP
>    message received from inside the tunnel to the source of that
>    original IPv6 packet.


As I understand, the problem is detected inside the tunnel and should
reply to the source of original packet.

In section 8.1 Tunnel ICMP Messages

The tunnel ICMP messages that are reported to the source of the
original packet are:
hop limit exceeded
unreachable node
parameter problem
packet too big

Also what I understand that a redirect msg may happen looks like

A: Original Packet Source Node
B: Tunnel Entry-Point Node
C: Tunnel Exit-Point Node
D: Original Packet Destination Node

A   --  B  -- Node 1 -- C -- D
           \- Node 2 -/

When B send msg to C, there may have a redirect from Node 1 to B, which
should be a ICMP error inside the tunnel. Not tunnel entry point to original
souce.


Or looks like

A: Original Packet Source Node
BE: Tunnel Entry-Point Node
CF: Tunnel Exit-Point Node
D: Original Packet Destination Node

A  --  B  --  C  --  D
   \-  E  --  F  -/

When A send pkt to D, and B reply a redirect msg to A. But I think this
problem is not detected _inside_ tunnel.

Thanks
Hangbin