| Message ID | 1494241863-32549-1-git-send-email-liuhangbin@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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...
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.
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
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));
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(-)