[PATCH v2 0/6] ipv6: Do route updating for redirect in ndisc layer

[PATCH v2 0/6] ipv6: Do route updating for redirect in ndisc layer

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

the ip6_redirect() could be replaced with
ip6_redirect_no_header(), we could always use ip6_redirect()
for route updating in ndisc layer and use the data of the
redirected header option just for finding the socket to be
notified and then notify user in protocols' err_handler.

---
 Changes for v2:
 1.handle the update of the NDISC_REDIRECT error code directly in
   icmpv6_err_convert.
 2.squash some patchs into one patch.
 3.modify the subject of those patchs.

Duan Jiong (6):
  ipv6: del the statements for updating route in (dccp|tcp|sctp)_v6_err
  ipv6: just match on ICMPV6_PKT_TOOBIG in those err_handle
  ipv6: del statements for dealing with NDISC_REDIRECT
  ip6tnl: move route updating for redirect to ndisc layer
  ipv6: modify the err to 0 when dealing with NDISC_REDIRECT
  ipv6: Do route updating for redirect in ndisc layer

 include/net/ip6_route.h |  3 ---
 net/dccp/ipv6.c         | 10 +++-------
 net/ipv6/ah6.c          |  9 ++-------
 net/ipv6/esp6.c         |  9 ++-------
 net/ipv6/icmp.c         |  5 +++--
 net/ipv6/ip6_tunnel.c   |  5 -----
 net/ipv6/ipcomp6.c      |  9 ++-------
 net/ipv6/ndisc.c        |  6 ++----
 net/ipv6/raw.c          |  3 +--
 net/ipv6/route.c        | 29 ++---------------------------
 net/ipv6/tcp_ipv6.c     | 12 ++++--------
 net/ipv6/udp.c          |  2 --
 net/sctp/input.c        | 12 ------------
 net/sctp/ipv6.c         |  6 +++---
 14 files changed, 24 insertions(+), 96 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/6] ipv6: del the statements for updating route in (dccp|tcp|sctp)_v6_err

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

Because we will do route updating for redirect in nidsc layer. And
when dealing with redirect message, the dccp and sctp should like
tcp return directly.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 net/dccp/ipv6.c     | 10 +++-------
 net/ipv6/tcp_ipv6.c | 12 ++++--------
 net/sctp/input.c    | 12 ------------
 net/sctp/ipv6.c     |  6 +++---
 4 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 9c61f9c..300840c 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -98,6 +98,9 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		return;
 	}
 
+	if (type == NDISC_REDIRECT)
+		return;
+
 	sk = inet6_lookup(net, &dccp_hashinfo,
 			&hdr->daddr, dh->dccph_dport,
 			&hdr->saddr, dh->dccph_sport, inet6_iif(skb));
@@ -130,13 +133,6 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 	np = inet6_sk(sk);
 
-	if (type == NDISC_REDIRECT) {
-		struct dst_entry *dst = __sk_dst_check(sk, np->dst_cookie);
-
-		if (dst)
-			dst->ops->redirect(dst, sk, skb);
-	}
-
 	if (type == ICMPV6_PKT_TOOBIG) {
 		struct dst_entry *dst = NULL;
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 5c71501..d3ca8a4 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -346,6 +346,10 @@ static void tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	__u32 seq;
 	struct net *net = dev_net(skb->dev);
 
+
+	if (type == NDISC_REDIRECT)
+		return;
+
 	sk = inet6_lookup(net, &tcp_hashinfo, &hdr->daddr,
 			th->dest, &hdr->saddr, th->source, skb->dev->ifindex);
 
@@ -382,14 +386,6 @@ static void tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 	np = inet6_sk(sk);
 
-	if (type == NDISC_REDIRECT) {
-		struct dst_entry *dst = __sk_dst_check(sk, np->dst_cookie);
-
-		if (dst)
-			dst->ops->redirect(dst, sk, skb);
-		goto out;
-	}
-
 	if (type == ICMPV6_PKT_TOOBIG) {
 		/* We are not interested in TCP_LISTEN and open_requests
 		 * (SYN-ACKs send out by Linux are always <576bytes so
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 5f20686..0d2d4b7 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -413,18 +413,6 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
 	sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
 }
 
-void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
-			struct sk_buff *skb)
-{
-	struct dst_entry *dst;
-
-	if (!t)
-		return;
-	dst = sctp_transport_dst_check(t);
-	if (dst)
-		dst->ops->redirect(dst, sk, skb);
-}
-
 /*
  * SCTP Implementer's Guide, 2.37 ICMP handling procedures
  *
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index da613ce..ee12d87 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -151,6 +151,9 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	int err;
 	struct net *net = dev_net(skb->dev);
 
+	if (type == NDISC_REDIRECT)
+		return;
+
 	idev = in6_dev_get(skb->dev);
 
 	/* Fix up skb to look at the embedded net header. */
@@ -181,9 +184,6 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 			goto out_unlock;
 		}
 		break;
-	case NDISC_REDIRECT:
-		sctp_icmp_redirect(sk, transport, skb);
-		break;
 	default:
 		break;
 	}
-- 
1.8.3.1

Re: [PATCH v2 1/6] ipv6: del the statements for updating route in (dccp|tcp|sctp)_v6_err

[Hannes Frederic Sowa]

[added linux-sctp, Daniel and Vlad + full quote]

On Fri, Sep 13, 2013 at 10:58:55AM +0800, Duan Jiong wrote:
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> 
> Because we will do route updating for redirect in nidsc layer. And
> when dealing with redirect message, the dccp and sctp should like
> tcp return directly.
> 
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> ---
>  net/dccp/ipv6.c     | 10 +++-------
>  net/ipv6/tcp_ipv6.c | 12 ++++--------
>  net/sctp/input.c    | 12 ------------
>  net/sctp/ipv6.c     |  6 +++---
>  4 files changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> index 9c61f9c..300840c 100644
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -98,6 +98,9 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  		return;
>  	}
>  
> +	if (type == NDISC_REDIRECT)
> +		return;
> +

Minor: Could you move this above the if (...) ICMP6_INC_STATS_BH. We don't
want to count errors here under no circumstance. These are in fact no errors.
(This is also the scheme in the other *_err functions).

Please Cc linux-sctp@vger.kernel.org on the sctp bits in the next round,
too.

Otherwise I looked at all patches and they seemed fine to me.

Duan, I would suggest the following:

I cannot judge if the patch Daniel proposed regarding EPROTO in sk->sk_err
on redirects should go to stable. If it should, perhaps this patch should
go in first and you could later on rebase this series as soon as Daniel's
patch has landed in the net repo? Only some minor edits should be needed
then. This way there is a clean patch for stable and David can consider
taking this in for net or net-next (this fixes a glitch where we do not
apply redirects generated by packets of ipv6 tunnels, otherwise a bit of
code removal).

Thanks,

  Hannes

>  	sk = inet6_lookup(net, &dccp_hashinfo,
>  			&hdr->daddr, dh->dccph_dport,
>  			&hdr->saddr, dh->dccph_sport, inet6_iif(skb));
> @@ -130,13 +133,6 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  
>  	np = inet6_sk(sk);
>  
> -	if (type == NDISC_REDIRECT) {
> -		struct dst_entry *dst = __sk_dst_check(sk, np->dst_cookie);
> -
> -		if (dst)
> -			dst->ops->redirect(dst, sk, skb);
> -	}
> -
>  	if (type == ICMPV6_PKT_TOOBIG) {
>  		struct dst_entry *dst = NULL;
>  
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 5c71501..d3ca8a4 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -346,6 +346,10 @@ static void tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  	__u32 seq;
>  	struct net *net = dev_net(skb->dev);
>  
> +
> +	if (type == NDISC_REDIRECT)
> +		return;
> +
>  	sk = inet6_lookup(net, &tcp_hashinfo, &hdr->daddr,
>  			th->dest, &hdr->saddr, th->source, skb->dev->ifindex);
>  
> @@ -382,14 +386,6 @@ static void tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  
>  	np = inet6_sk(sk);
>  
> -	if (type == NDISC_REDIRECT) {
> -		struct dst_entry *dst = __sk_dst_check(sk, np->dst_cookie);
> -
> -		if (dst)
> -			dst->ops->redirect(dst, sk, skb);
> -		goto out;
> -	}
> -
>  	if (type == ICMPV6_PKT_TOOBIG) {
>  		/* We are not interested in TCP_LISTEN and open_requests
>  		 * (SYN-ACKs send out by Linux are always <576bytes so
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 5f20686..0d2d4b7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -413,18 +413,6 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
>  	sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
>  }
>  
> -void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
> -			struct sk_buff *skb)
> -{
> -	struct dst_entry *dst;
> -
> -	if (!t)
> -		return;
> -	dst = sctp_transport_dst_check(t);
> -	if (dst)
> -		dst->ops->redirect(dst, sk, skb);
> -}
> -
>  /*
>   * SCTP Implementer's Guide, 2.37 ICMP handling procedures
>   *
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index da613ce..ee12d87 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -151,6 +151,9 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  	int err;
>  	struct net *net = dev_net(skb->dev);
>  
> +	if (type == NDISC_REDIRECT)
> +		return;
> +
>  	idev = in6_dev_get(skb->dev);
>  
>  	/* Fix up skb to look at the embedded net header. */
> @@ -181,9 +184,6 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>  			goto out_unlock;
>  		}
>  		break;
> -	case NDISC_REDIRECT:
> -		sctp_icmp_redirect(sk, transport, skb);
> -		break;
>  	default:
>  		break;
>  	}
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
gruss,

  Hannes

Re: [PATCH v2 1/6] ipv6: del the statements for updating route in (dccp|tcp|sctp)_v6_err

[Daniel Borkmann]

> [added linux-sctp, Daniel and Vlad + full quote]
> 
> On Fri, Sep 13, 2013 at 10:58:55AM +0800, Duan Jiong wrote:
> > From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> > 
> > Because we will do route updating for redirect in nidsc layer. And
> > when dealing with redirect message, the dccp and sctp should like
> > tcp return directly.
> > 
> > Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
[...]
> Please Cc linux-sctp@vger.kernel.org on the sctp bits in the next round,
> too.
> 
> Otherwise I looked at all patches and they seemed fine to me.
> 
> Duan, I would suggest the following:
> 
> I cannot judge if the patch Daniel proposed regarding EPROTO in sk->sk_err
> on redirects should go to stable. If it should, perhaps this patch should
> go in first and you could later on rebase this series as soon as Daniel's
> patch has landed in the net repo? Only some minor edits should be needed
> then. This way there is a clean patch for stable and David can consider
> taking this in for net or net-next (this fixes a glitch where we do not
> apply redirects generated by packets of ipv6 tunnels, otherwise a bit of
> code removal).

Thanks Hannes !

I'm on travel over the weekend and mostly offline, so I'll look into this
on right on Monday morning.

Cheers and thanks,

Daniel

[PATCH v2 2/6] ipv6: just match on ICMPV6_PKT_TOOBIG in those err_handle

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

we don't handle NDISC_REDIRECTs here any more, and the match
on ICMPV6_DEST_UNREACH is meaningless.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 net/ipv6/ah6.c     | 9 ++-------
 net/ipv6/esp6.c    | 9 ++-------
 net/ipv6/ipcomp6.c | 9 ++-------
 3 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 73784c3..79fb40f 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -618,19 +618,14 @@ static void ah6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct ip_auth_hdr *ah = (struct ip_auth_hdr*)(skb->data+offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
-	    type != NDISC_REDIRECT)
+	if (type != ICMPV6_PKT_TOOBIG)
 		return;
 
 	x = xfrm_state_lookup(net, skb->mark, (xfrm_address_t *)&iph->daddr, ah->spi, IPPROTO_AH, AF_INET6);
 	if (!x)
 		return;
 
-	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, skb->dev->ifindex, 0);
-	else
-		ip6_update_pmtu(skb, net, info, 0, 0);
+	ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
 }
 
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index d3618a7..6aa64e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -436,9 +436,7 @@ static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct ip_esp_hdr *esph = (struct ip_esp_hdr *)(skb->data + offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
-	    type != NDISC_REDIRECT)
+	if (type != ICMPV6_PKT_TOOBIG)
 		return;
 
 	x = xfrm_state_lookup(net, skb->mark, (const xfrm_address_t *)&iph->daddr,
@@ -446,10 +444,7 @@ static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	if (!x)
 		return;
 
-	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, skb->dev->ifindex, 0);
-	else
-		ip6_update_pmtu(skb, net, info, 0, 0);
+	ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
 }
 
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 5636a91..e943158 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -64,9 +64,7 @@ static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		(struct ip_comp_hdr *)(skb->data + offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
-	    type != NDISC_REDIRECT)
+	if (type != ICMPV6_PKT_TOOBIG)
 		return;
 
 	spi = htonl(ntohs(ipcomph->cpi));
@@ -75,10 +73,7 @@ static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	if (!x)
 		return;
 
-	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, skb->dev->ifindex, 0);
-	else
-		ip6_update_pmtu(skb, net, info, 0, 0);
+	ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
 }
 
-- 
1.8.3.1

[PATCH v2 3/6] ipv6: del statements for dealing with NDISC_REDIRECT

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

Now the route updating for redirect is done in ndisc
layer.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 net/ipv6/icmp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index eef8d94..4bde43c 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -91,8 +91,6 @@ static void icmpv6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 	if (type == ICMPV6_PKT_TOOBIG)
 		ip6_update_pmtu(skb, net, info, 0, 0);
-	else if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, skb->dev->ifindex, 0);
 
 	if (!(type & ICMPV6_INFOMSG_MASK))
 		if (icmp6->icmp6_type == ICMPV6_ECHO_REQUEST)
-- 
1.8.3.1

[PATCH v2 4/6] ip6tnl: move route updating for redirect to ndisc layer

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

In rfc2473, we can know that the tunnel ICMP redirect
message should not be reported to the source of the
original packet, so after calling ip6_tnl_err(), the
rel_msg is set to 0 in function ip4ip6_err(), and the
redirect will never be handled.

In order to deal with this, we move route updating for
redirect to ndisc layer.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.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 61355f7..3ea834b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -576,9 +576,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;
 	}
@@ -637,8 +634,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));
 
-- 
1.8.3.1

[PATCH v2 5/6] ipv6: modify the err to 0 when dealing with NDISC_REDIRECT

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

when dealing with redirect message, the err shoud
be assigned to 0, not EPROTO. And del the statements
for updating route.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 net/ipv6/icmp.c | 3 +++
 net/ipv6/raw.c  | 3 +--
 net/ipv6/udp.c  | 2 --
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 4bde43c..6bcedcc 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -975,6 +975,9 @@ int icmpv6_err_convert(u8 type, u8 code, int *err)
 	case ICMPV6_TIME_EXCEED:
 		*err = EHOSTUNREACH;
 		break;
+	case NDISC_REDIRECT:
+		*err = 0;
+		break;
 	}
 
 	return fatal;
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 58916bb..baf86b8 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -335,8 +335,7 @@ static void rawv6_err(struct sock *sk, struct sk_buff *skb,
 		ip6_sk_update_pmtu(skb, sk, info);
 		harderr = (np->pmtudisc == IPV6_PMTUDISC_DO);
 	}
-	if (type == NDISC_REDIRECT)
-		ip6_sk_redirect(skb, sk);
+
 	if (np->recverr) {
 		u8 *payload = skb->data;
 		if (!inet->hdrincl)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index f405815..a40b392 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -525,8 +525,6 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 	if (type == ICMPV6_PKT_TOOBIG)
 		ip6_sk_update_pmtu(skb, sk, info);
-	if (type == NDISC_REDIRECT)
-		ip6_sk_redirect(skb, sk);
 
 	np = inet6_sk(sk);
 
-- 
1.8.3.1

[PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Duan Jiong]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

Do the whole verification and route updating in ndisc
lay and then just call into icmpv6_notify() to notify
the upper protocols.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 include/net/ip6_route.h |  3 ---
 net/ipv6/ndisc.c        |  6 ++----
 net/ipv6/route.c        | 29 ++---------------------------
 3 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index f525e70..5db259e 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -133,9 +133,6 @@ extern void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
 extern void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk,
 			       __be32 mtu);
 extern void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark);
-extern void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
-				   u32 mark);
-extern void ip6_sk_redirect(struct sk_buff *skb, struct sock *sk);
 
 struct netlink_callback;
 
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index f8a55ff..6bd1b41 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1368,11 +1368,9 @@ static void ndisc_redirect_rcv(struct sk_buff *skb)
 	if (!ndisc_parse_options(msg->opt, ndoptlen, &ndopts))
 		return;
 
-	if (!ndopts.nd_opts_rh) {
-		ip6_redirect_no_header(skb, dev_net(skb->dev),
-					skb->dev->ifindex, 0);
+	ip6_redirect(skb, dev_net(skb->dev), skb->dev->ifindex, 0);
+	if (!ndopts.nd_opts_rh)
 		return;
-	}
 
 	hdr = (u8 *)ndopts.nd_opts_rh;
 	hdr += 8;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c979dd9..151bd6c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1227,27 +1227,7 @@ static struct dst_entry *ip6_route_redirect(struct net *net,
 				flags, __ip6_route_redirect);
 }
 
-void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
-{
-	const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
-	struct dst_entry *dst;
-	struct flowi6 fl6;
-
-	memset(&fl6, 0, sizeof(fl6));
-	fl6.flowi6_oif = oif;
-	fl6.flowi6_mark = mark;
-	fl6.flowi6_flags = 0;
-	fl6.daddr = iph->daddr;
-	fl6.saddr = iph->saddr;
-	fl6.flowlabel = ip6_flowinfo(iph);
-
-	dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
-	rt6_do_redirect(dst, NULL, skb);
-	dst_release(dst);
-}
-EXPORT_SYMBOL_GPL(ip6_redirect);
-
-void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
+void ip6_redirect(struct sk_buff *skb, struct net *net, int oif,
 			    u32 mark)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
@@ -1266,12 +1246,7 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
 	rt6_do_redirect(dst, NULL, skb);
 	dst_release(dst);
 }
-
-void ip6_sk_redirect(struct sk_buff *skb, struct sock *sk)
-{
-	ip6_redirect(skb, sock_net(sk), sk->sk_bound_dev_if, sk->sk_mark);
-}
-EXPORT_SYMBOL_GPL(ip6_sk_redirect);
+EXPORT_SYMBOL_GPL(ip6_redirect);
 
 static unsigned int ip6_default_advmss(const struct dst_entry *dst)
 {
-- 
1.8.3.1

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[David Miller]

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Fri, 13 Sep 2013 11:03:07 +0800

> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> 
> Do the whole verification and route updating in ndisc
> lay and then just call into icmpv6_notify() to notify
> the upper protocols.
> 
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>

This is completely broken, and I believe your patch set fundamentally
is too.

We absolutely _must_ handle the redirect at the socket level when
we are able to, otherwise we cannot specify the mark properly and
the mark is an essential part of the key used to find the correct
route to work with.

I am not applying this patch series until you deal with this
deficiency.  I am not willing to consider changes which stop using the
more precise keying information available from a socket.

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Hannes Frederic Sowa]

On Tue, Sep 17, 2013 at 08:29:36PM -0400, David Miller wrote:
> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> Date: Fri, 13 Sep 2013 11:03:07 +0800
> 
> > From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> > 
> > Do the whole verification and route updating in ndisc
> > lay and then just call into icmpv6_notify() to notify
> > the upper protocols.
> > 
> > Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> 
> This is completely broken, and I believe your patch set fundamentally
> is too.
> 
> We absolutely _must_ handle the redirect at the socket level when
> we are able to, otherwise we cannot specify the mark properly and
> the mark is an essential part of the key used to find the correct
> route to work with.
> 
> I am not applying this patch series until you deal with this
> deficiency.  I am not willing to consider changes which stop using the
> more precise keying information available from a socket.

Oh, Duan, I am very sorry for not catching this earlier. We use the
sk->mark to select the proper routing table where we clone the rt6_info into.
And we only get that value out of the sockets. I missed that. We should leave
the redirect logic in the socket layer where it is possible.

But parts of this series are still valid. We need to fix redirects for tunnels
and I do think we can still simplify some code in the error handlers.

Thanks for pointing this out,

  Hannes

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Duan Jiong]

于 2013年09月18日 09:39, Hannes Frederic Sowa 写道:
> On Tue, Sep 17, 2013 at 08:29:36PM -0400, David Miller wrote:
>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>> Date: Fri, 13 Sep 2013 11:03:07 +0800
>>
>>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>
>>> Do the whole verification and route updating in ndisc
>>> lay and then just call into icmpv6_notify() to notify
>>> the upper protocols.
>>>
>>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>
>> This is completely broken, and I believe your patch set fundamentally
>> is too.
>>
>> We absolutely _must_ handle the redirect at the socket level when
>> we are able to, otherwise we cannot specify the mark properly and
>> the mark is an essential part of the key used to find the correct
>> route to work with.
>>
>> I am not applying this patch series until you deal with this
>> deficiency.  I am not willing to consider changes which stop using the
>> more precise keying information available from a socket.
> 
> Oh, Duan, I am very sorry for not catching this earlier. We use the
> sk->mark to select the proper routing table where we clone the rt6_info into.
> And we only get that value out of the sockets. I missed that. We should leave
> the redirect logic in the socket layer where it is possible.
> 
> But parts of this series are still valid. We need to fix redirects for tunnels
> and I do think we can still simplify some code in the error handlers.
> 

I got it.

Thanks,
  Duan

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Hannes Frederic Sowa]

On Wed, Sep 18, 2013 at 09:52:42AM +0800, Duan Jiong wrote:
> 于 2013年09月18日 09:39, Hannes Frederic Sowa 写道:
> > On Tue, Sep 17, 2013 at 08:29:36PM -0400, David Miller wrote:
> >> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> >> Date: Fri, 13 Sep 2013 11:03:07 +0800
> >>
> >>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> >>>
> >>> Do the whole verification and route updating in ndisc
> >>> lay and then just call into icmpv6_notify() to notify
> >>> the upper protocols.
> >>>
> >>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> >>
> >> This is completely broken, and I believe your patch set fundamentally
> >> is too.
> >>
> >> We absolutely _must_ handle the redirect at the socket level when
> >> we are able to, otherwise we cannot specify the mark properly and
> >> the mark is an essential part of the key used to find the correct
> >> route to work with.
> >>
> >> I am not applying this patch series until you deal with this
> >> deficiency.  I am not willing to consider changes which stop using the
> >> more precise keying information available from a socket.
> > 
> > Oh, Duan, I am very sorry for not catching this earlier. We use the
> > sk->mark to select the proper routing table where we clone the rt6_info into.
> > And we only get that value out of the sockets. I missed that. We should leave
> > the redirect logic in the socket layer where it is possible.
> > 
> > But parts of this series are still valid. We need to fix redirects for tunnels
> > and I do think we can still simplify some code in the error handlers.
> > 
> 
> I got it.

I gave it a bit more thought:

RFC 4861 8.3:
"
   Redirect messages apply to all flows that are being sent to a given
   destination.  That is, upon receipt of a Redirect for a Destination
   Address, all Destination Cache entries to that address should be
   updated to use the specified next-hop, regardless of the contents of
   the Flow Label field that appears in the Redirected Header option.
"

Especially because redirects also help in the on-link determination (same
RFC, section 8), I changed my mind and am still in favour of updating it
in the ndisc layer. In my opinion we just have to consider all routing
tables and apply the update to every one which carries a valid next hop
to the source of the redirect (under consideration of the destination).

This will be important if we actually try to get linux to correctly
implement the ipv6 subnet model (RFC 5942, Section 4 Rule 1). In that
case we are not allowed to assume nodes on-link even if they would match
the same prefix as a locally configured address.

Greetings,

  Hannes

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Duan Jiong]

于 2013年09月18日 12:13, Hannes Frederic Sowa 写道:
> On Wed, Sep 18, 2013 at 09:52:42AM +0800, Duan Jiong wrote:
>> 于 2013年09月18日 09:39, Hannes Frederic Sowa 写道:
>>> On Tue, Sep 17, 2013 at 08:29:36PM -0400, David Miller wrote:
>>>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>> Date: Fri, 13 Sep 2013 11:03:07 +0800
>>>>
>>>>> From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>>>
>>>>> Do the whole verification and route updating in ndisc
>>>>> lay and then just call into icmpv6_notify() to notify
>>>>> the upper protocols.
>>>>>
>>>>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>>
>>>> This is completely broken, and I believe your patch set fundamentally
>>>> is too.
>>>>
>>>> We absolutely _must_ handle the redirect at the socket level when
>>>> we are able to, otherwise we cannot specify the mark properly and
>>>> the mark is an essential part of the key used to find the correct
>>>> route to work with.
>>>>
>>>> I am not applying this patch series until you deal with this
>>>> deficiency.  I am not willing to consider changes which stop using the
>>>> more precise keying information available from a socket.
>>>
>>> Oh, Duan, I am very sorry for not catching this earlier. We use the
>>> sk->mark to select the proper routing table where we clone the rt6_info into.
>>> And we only get that value out of the sockets. I missed that. We should leave
>>> the redirect logic in the socket layer where it is possible.
>>>
>>> But parts of this series are still valid. We need to fix redirects for tunnels
>>> and I do think we can still simplify some code in the error handlers.
>>>
>>
>> I got it.
> 
> I gave it a bit more thought:
> 
> RFC 4861 8.3:
> "
>    Redirect messages apply to all flows that are being sent to a given
>    destination.  That is, upon receipt of a Redirect for a Destination
>    Address, all Destination Cache entries to that address should be
>    updated to use the specified next-hop, regardless of the contents of
>    the Flow Label field that appears in the Redirected Header option.
> "
> 
> Especially because redirects also help in the on-link determination (same
> RFC, section 8), I changed my mind and am still in favour of updating it
> in the ndisc layer. In my opinion we just have to consider all routing
> tables and apply the update to every one which carries a valid next hop
> to the source of the redirect (under consideration of the destination).
> 
> This will be important if we actually try to get linux to correctly
> implement the ipv6 subnet model (RFC 5942, Section 4 Rule 1). In that
> case we are not allowed to assume nodes on-link even if they would match
> the same prefix as a locally configured address.
> 

I think this need a little time to discuss with David Miller, so i will send
the other patchs to fix redirects for tunnels and to fix the sk->sk_err
problems in udpv6_err()/rawv6_err().

Thanks,
  Duan

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Hannes Frederic Sowa]

Hi Duan!

On Wed, Sep 18, 2013 at 06:13:37AM +0200, Hannes Frederic Sowa wrote:
> Especially because redirects also help in the on-link determination (same
> RFC, section 8), I changed my mind and am still in favour of updating it
> in the ndisc layer. In my opinion we just have to consider all routing
> tables and apply the update to every one which carries a valid next hop
> to the source of the redirect (under consideration of the destination).
> 
> This will be important if we actually try to get linux to correctly
> implement the ipv6 subnet model (RFC 5942, Section 4 Rule 1). In that
> case we are not allowed to assume nodes on-link even if they would match
> the same prefix as a locally configured address.

I am playing around with a simple patch which does suppress adding routing
information for the on-link assumption we currently do in linux.

Are you intereseted in following up on this? I still do think we should update
not only the routing table the socket uses but all routing tables which have a
valid route towards the router which emitted the redirect.

I try to check if we actually handle redirect messages when ECMP routes are in
use correctly.

Greetings,

  Hannes

Re: [PATCH v2 6/6] ipv6: Do route updating for redirect in ndisc layer

[Duan Jiong]

于 2013年10月09日 09:43, Hannes Frederic Sowa 写道:
> Hi Duan!
> 
> On Wed, Sep 18, 2013 at 06:13:37AM +0200, Hannes Frederic Sowa wrote:
>> Especially because redirects also help in the on-link determination (same
>> RFC, section 8), I changed my mind and am still in favour of updating it
>> in the ndisc layer. In my opinion we just have to consider all routing
>> tables and apply the update to every one which carries a valid next hop
>> to the source of the redirect (under consideration of the destination).
>>
>> This will be important if we actually try to get linux to correctly
>> implement the ipv6 subnet model (RFC 5942, Section 4 Rule 1). In that
>> case we are not allowed to assume nodes on-link even if they would match
>> the same prefix as a locally configured address.
> 
> I am playing around with a simple patch which does suppress adding routing
> information for the on-link assumption we currently do in linux.
> 
> Are you intereseted in following up on this? I still do think we should update
> not only the routing table the socket uses but all routing tables which have a
> valid route towards the router which emitted the redirect.
> 

No, thanks, i have got other things on my mind, more important, but i will
pay attention to it.

Thanks,
  Duan.

> I try to check if we actually handle redirect messages when ECMP routes are in
> use correctly.
>