[PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address

[PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address

[Su Yanjun]

When we send mpls packets and the interface only has a
manual global ipv6 address, then the two hosts cant communicate.
I find that in ndisc_send_ns it only tries to get a ll address.
In my case, the executive path is as below.
ip6_output
 ->ip6_finish_output
  ->lwtunnel_xmit
   ->mpls_xmit
    ->neigh_resolve_output
     ->neigh_probe
      ->ndisc_solicit
       ->ndisc_send_ns

In RFC4861, 7.2.2 says
"If the source address of the packet prompting the solicitation is the
same as one of the addresses assigned to the outgoing interface, that
address SHOULD be placed in the IP Source Address of the outgoing
solicitation.  Otherwise, any one of the addresses assigned to the
interface should be used."

In this patch we try get a global address if we get ll address failed.

Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
---
 include/net/addrconf.h |  4 ++++
 net/ipv6/addrconf.c    | 34 ++++++++++++++++++++++++++++++++++
 net/ipv6/ndisc.c       |  8 ++++++--
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index becdad5..006db8e 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -107,6 +107,10 @@ int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
 		      u32 banned_flags);
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
 		    u32 banned_flags);
+int __ipv6_get_addr(struct inet6_dev *idev, struct in6_addr *addr,
+		      u32 banned_flags);
+int ipv6_get_addr(struct net_device *dev, struct in6_addr *addr,
+		    u32 banned_flags);
 bool inet_rcv_saddr_equal(const struct sock *sk, const struct sock *sk2,
 			  bool match_wildcard);
 bool inet_rcv_saddr_any(const struct sock *sk);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 521e320..4c0a43f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1870,6 +1870,40 @@ int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
 	return err;
 }
 
+int __ipv6_get_addr(struct inet6_dev *idev, struct in6_addr *addr,
+		    u32 banned_flags)
+{
+	struct inet6_ifaddr *ifp;
+	int err = -EADDRNOTAVAIL;
+
+	list_for_each_entry_reverse(ifp, &idev->addr_list, if_list) {
+		if (ifp->scope == 0 &&
+		    !(ifp->flags & banned_flags)) {
+			*addr = ifp->addr;
+			err = 0;
+			break;
+		}
+	}
+	return err;
+}
+
+int ipv6_get_addr(struct net_device *dev, struct in6_addr *addr,
+		  u32 banned_flags)
+{
+	struct inet6_dev *idev;
+	int err = -EADDRNOTAVAIL;
+
+	rcu_read_lock();
+	idev = __in6_dev_get(dev);
+	if (idev) {
+		read_lock_bh(&idev->lock);
+		err = __ipv6_get_addr(idev, addr, banned_flags);
+		read_unlock_bh(&idev->lock);
+	}
+	rcu_read_unlock();
+	return err;
+}
+
 static int ipv6_count_addresses(const struct inet6_dev *idev)
 {
 	const struct inet6_ifaddr *ifp;
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 083cc1c..18ac2fb 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -606,8 +606,12 @@ void ndisc_send_ns(struct net_device *dev, const struct in6_addr *solicit,
 
 	if (!saddr) {
 		if (ipv6_get_lladdr(dev, &addr_buf,
-				   (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)))
-			return;
+				   (IFA_F_TENTATIVE | IFA_F_OPTIMISTIC))) {
+			/* try global address */
+			if (ipv6_get_addr(dev, &addr_buf,
+					  (IFA_F_TENTATIVE | IFA_F_OPTIMISTIC)))
+				return;
+		}
 		saddr = &addr_buf;
 	}
 
-- 
2.7.4

Re: [PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address

[David Miller]

From: Su Yanjun <suyj.fnst@cn.fujitsu.com>
Date: Mon, 29 Jul 2019 10:49:51 +0800

> When we send mpls packets and the interface only has a
> manual global ipv6 address, then the two hosts cant communicate.
> I find that in ndisc_send_ns it only tries to get a ll address.
> In my case, the executive path is as below.
> ip6_output
>  ->ip6_finish_output
>   ->lwtunnel_xmit
>    ->mpls_xmit
>     ->neigh_resolve_output
>      ->neigh_probe
>       ->ndisc_solicit
>        ->ndisc_send_ns
> 
> In RFC4861, 7.2.2 says
> "If the source address of the packet prompting the solicitation is the
> same as one of the addresses assigned to the outgoing interface, that
> address SHOULD be placed in the IP Source Address of the outgoing
> solicitation.  Otherwise, any one of the addresses assigned to the
> interface should be used."
> 
> In this patch we try get a global address if we get ll address failed.
> 
> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>

David, can you take a quick look at this?

Thank you.

Re: [PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address

[David Ahern]

On 7/29/19 3:17 PM, David Miller wrote:
> David, can you take a quick look at this?

will do. I'll get back to you by tomorrow.

Re: [PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address

[David Ahern]

On 7/28/19 8:49 PM, Su Yanjun wrote:
> When we send mpls packets and the interface only has a
> manual global ipv6 address, then the two hosts cant communicate.
> I find that in ndisc_send_ns it only tries to get a ll address.
> In my case, the executive path is as below.
> ip6_output
>  ->ip6_finish_output
>   ->lwtunnel_xmit
>    ->mpls_xmit
>     ->neigh_resolve_output
>      ->neigh_probe
>       ->ndisc_solicit
>        ->ndisc_send_ns

for the archives, this is not an MPLS problem but a general IPv6
forwarding problem when the egress interface does not have a link local
address.

> 
> In RFC4861, 7.2.2 says
> "If the source address of the packet prompting the solicitation is the
> same as one of the addresses assigned to the outgoing interface, that
> address SHOULD be placed in the IP Source Address of the outgoing
> solicitation.  Otherwise, any one of the addresses assigned to the
> interface should be used."
> 
> In this patch we try get a global address if we get ll address failed.
> 
> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> ---
>  include/net/addrconf.h |  4 ++++
>  net/ipv6/addrconf.c    | 34 ++++++++++++++++++++++++++++++++++
>  net/ipv6/ndisc.c       |  8 ++++++--
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index becdad5..006db8e 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -107,6 +107,10 @@ int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
>  		      u32 banned_flags);
>  int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
>  		    u32 banned_flags);
> +int __ipv6_get_addr(struct inet6_dev *idev, struct in6_addr *addr,
> +		      u32 banned_flags);

no reason to export __ipv6_get_addr. I suspect you copied
__ipv6_get_lladdr but it has an external (to addrconf.c) user. In this
case only ipv6_get_addr needs to be exported.


> +int ipv6_get_addr(struct net_device *dev, struct in6_addr *addr,
> +		    u32 banned_flags);
>  bool inet_rcv_saddr_equal(const struct sock *sk, const struct sock *sk2,
>  			  bool match_wildcard);
>  bool inet_rcv_saddr_any(const struct sock *sk);
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 521e320..4c0a43f 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1870,6 +1870,40 @@ int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
>  	return err;
>  }
>  
> +int __ipv6_get_addr(struct inet6_dev *idev, struct in6_addr *addr,
> +		    u32 banned_flags)
> +{
> +	struct inet6_ifaddr *ifp;
> +	int err = -EADDRNOTAVAIL;
> +
> +	list_for_each_entry_reverse(ifp, &idev->addr_list, if_list) {

Addresses are ordered by scope. __ipv6_get_lladdr uses
list_for_each_entry_reverse because the LLA's are after the globals.
Since this is falling back to 'give an address' from this interface, I
think you can just use list_for_each_entry.


> +		if (ifp->scope == 0 &&
> +		    !(ifp->flags & banned_flags)) {
> +			*addr = ifp->addr;
> +			err = 0;
> +			break;
> +		}
> +	}
> +	return err;
> +}
> +
> +int ipv6_get_addr(struct net_device *dev, struct in6_addr *addr,
> +		  u32 banned_flags)
> +{
> +	struct inet6_dev *idev;
> +	int err = -EADDRNOTAVAIL;
> +
> +	rcu_read_lock();
> +	idev = __in6_dev_get(dev);
> +	if (idev) {
> +		read_lock_bh(&idev->lock);
> +		err = __ipv6_get_addr(idev, addr, banned_flags);
> +		read_unlock_bh(&idev->lock);
> +	}
> +	rcu_read_unlock();
> +	return err;
> +}
> +
>  static int ipv6_count_addresses(const struct inet6_dev *idev)
>  {
>  	const struct inet6_ifaddr *ifp;
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 083cc1c..18ac2fb 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -606,8 +606,12 @@ void ndisc_send_ns(struct net_device *dev, const struct in6_addr *solicit,
>  
>  	if (!saddr) {

And since you are going to do a v2, another nit - define a local banned
flags and use it for both lookups just to make it clear.

		u32 banned_flags = IFA_F_TENTATIVE | IFA_F_OPTIMISTIC;

>  		if (ipv6_get_lladdr(dev, &addr_buf,
> -				   (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)))
> -			return;
> +				   (IFA_F_TENTATIVE | IFA_F_OPTIMISTIC))) {
> +			/* try global address */
> +			if (ipv6_get_addr(dev, &addr_buf,
> +					  (IFA_F_TENTATIVE | IFA_F_OPTIMISTIC)))
> +				return;
> +		}
>  		saddr = &addr_buf;
>  	}
>  
>