[PATCH net 0/2] net: fix a mcast issue for tipc udp media

[PATCH net 0/2] net: fix a mcast issue for tipc udp media

[Xin Long]

Patch 1 is to add a function to get the dev by source address,
whcih will be used by Patch 2.

Xin Long (2):
  ipv6: add ipv6_dev_find()
  tipc: set ub->ifindex for local ipv6 address

 include/net/addrconf.h |  2 ++
 net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
 net/tipc/udp_media.c   |  8 ++++++++
 3 files changed, 49 insertions(+)

-- 
2.1.0

[PATCH net 1/2] ipv6: add ipv6_dev_find()

[Xin Long]

This is to add an ip_dev_find like function for ipv6, used to find
the dev by saddr.

It will be used by TIPC protocol. So also export it.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/addrconf.h |  2 ++
 net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 8418b7d..ba3f6c15 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
 
 int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
 
+struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
+
 struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
 				     const struct in6_addr *addr,
 				     struct net_device *dev, int strict);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 840bfdb..857d6f9 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev)
 }
 EXPORT_SYMBOL(ipv6_chk_prefix);
 
+/**
+ * ipv6_dev_find - find the first device with a given source address.
+ * @net: the net namespace
+ * @addr: the source address
+ *
+ * The caller should be protected by RCU, or RTNL.
+ */
+struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr)
+{
+	unsigned int hash = inet6_addr_hash(net, addr);
+	struct inet6_ifaddr *ifp, *result = NULL;
+	struct net_device *dev = NULL;
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
+		if (net_eq(dev_net(ifp->idev->dev), net) &&
+		    ipv6_addr_equal(&ifp->addr, addr)) {
+			result = ifp;
+			break;
+		}
+	}
+
+	if (!result) {
+		struct rt6_info *rt;
+
+		rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
+		if (rt) {
+			dev = rt->dst.dev;
+			ip6_rt_put(rt);
+		}
+	} else {
+		dev = result->idev->dev;
+	}
+	rcu_read_unlock();
+
+	return dev;
+}
+EXPORT_SYMBOL(ipv6_dev_find);
+
 struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
 				     struct net_device *dev, int strict)
 {
-- 
2.1.0

[PATCH net 2/2] tipc: set ub->ifindex for local ipv6 address

[Xin Long]

Without ub->ifindex set for ipv6 address in tipc_udp_enable(),
ipv6_sock_mc_join() may make the wrong dev join the multicast
address in enable_mcast(). This causes that tipc links would
never be created.

So fix it by getting the right netdev and setting ub->ifindex,
as it does for ipv4 address.

Reported-by: Shuang Li <shuali@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/tipc/udp_media.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 28a283f..9dec596 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -738,6 +738,13 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 		b->mtu = b->media->mtu;
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (local.proto == htons(ETH_P_IPV6)) {
+		struct net_device *dev;
+
+		dev = ipv6_dev_find(net, &local.ipv6);
+		if (!dev) {
+			err = -ENODEV;
+			goto err;
+		}
 		udp_conf.family = AF_INET6;
 		udp_conf.use_udp6_tx_checksums = true;
 		udp_conf.use_udp6_rx_checksums = true;
@@ -745,6 +752,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 			udp_conf.local_ip6 = in6addr_any;
 		else
 			udp_conf.local_ip6 = local.ipv6;
+		ub->ifindex = dev->ifindex;
 		b->mtu = 1280;
 #endif
 	} else {
-- 
2.1.0

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Ying Xue]

On 8/3/20 11:34 PM, Xin Long wrote:
> This is to add an ip_dev_find like function for ipv6, used to find
> the dev by saddr.
> 
> It will be used by TIPC protocol. So also export it.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Ying Xue <ying.xue@windriver.com>

> ---
>  include/net/addrconf.h |  2 ++
>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index 8418b7d..ba3f6c15 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
>  
>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
>  
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> +
>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
>  				     const struct in6_addr *addr,
>  				     struct net_device *dev, int strict);
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 840bfdb..857d6f9 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ipv6_chk_prefix);
>  
> +/**
> + * ipv6_dev_find - find the first device with a given source address.
> + * @net: the net namespace
> + * @addr: the source address
> + *
> + * The caller should be protected by RCU, or RTNL.
> + */
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr)
> +{
> +	unsigned int hash = inet6_addr_hash(net, addr);
> +	struct inet6_ifaddr *ifp, *result = NULL;
> +	struct net_device *dev = NULL;
> +
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
> +		if (net_eq(dev_net(ifp->idev->dev), net) &&
> +		    ipv6_addr_equal(&ifp->addr, addr)) {
> +			result = ifp;
> +			break;
> +		}
> +	}
> +
> +	if (!result) {
> +		struct rt6_info *rt;
> +
> +		rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> +		if (rt) {
> +			dev = rt->dst.dev;
> +			ip6_rt_put(rt);
> +		}
> +	} else {
> +		dev = result->idev->dev;
> +	}
> +	rcu_read_unlock();
> +
> +	return dev;
> +}
> +EXPORT_SYMBOL(ipv6_dev_find);
> +
>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
>  				     struct net_device *dev, int strict)
>  {
> 

Re: [PATCH net 2/2] tipc: set ub->ifindex for local ipv6 address

[Ying Xue]

On 8/3/20 11:34 PM, Xin Long wrote:
> Without ub->ifindex set for ipv6 address in tipc_udp_enable(),
> ipv6_sock_mc_join() may make the wrong dev join the multicast
> address in enable_mcast(). This causes that tipc links would
> never be created.
> 
> So fix it by getting the right netdev and setting ub->ifindex,
> as it does for ipv4 address.
> 
> Reported-by: Shuang Li <shuali@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Ying Xue <ying.xue@windriver.com>

> ---
>  net/tipc/udp_media.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
> index 28a283f..9dec596 100644
> --- a/net/tipc/udp_media.c
> +++ b/net/tipc/udp_media.c
> @@ -738,6 +738,13 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
>  		b->mtu = b->media->mtu;
>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else if (local.proto == htons(ETH_P_IPV6)) {
> +		struct net_device *dev;
> +
> +		dev = ipv6_dev_find(net, &local.ipv6);
> +		if (!dev) {
> +			err = -ENODEV;
> +			goto err;
> +		}
>  		udp_conf.family = AF_INET6;
>  		udp_conf.use_udp6_tx_checksums = true;
>  		udp_conf.use_udp6_rx_checksums = true;
> @@ -745,6 +752,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
>  			udp_conf.local_ip6 = in6addr_any;
>  		else
>  			udp_conf.local_ip6 = local.ipv6;
> +		ub->ifindex = dev->ifindex;
>  		b->mtu = 1280;
>  #endif
>  	} else {
> 

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[David Ahern]

On 8/5/20 5:03 AM, Ying Xue wrote:
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 840bfdb..857d6f9 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev)
>>  }
>>  EXPORT_SYMBOL(ipv6_chk_prefix);
>>  
>> +/**
>> + * ipv6_dev_find - find the first device with a given source address.
>> + * @net: the net namespace
>> + * @addr: the source address
>> + *
>> + * The caller should be protected by RCU, or RTNL.
>> + */
>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr)
>> +{
>> +	unsigned int hash = inet6_addr_hash(net, addr);
>> +	struct inet6_ifaddr *ifp, *result = NULL;
>> +	struct net_device *dev = NULL;
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
>> +		if (net_eq(dev_net(ifp->idev->dev), net) &&
>> +		    ipv6_addr_equal(&ifp->addr, addr)) {
>> +			result = ifp;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!result) {
>> +		struct rt6_info *rt;
>> +
>> +		rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);

why is this needed? you have already walked the interfaces in the
namespace and not found the address. Doing a route lookup is beyond the
stated scope of this function.

>> +		if (rt) {
>> +			dev = rt->dst.dev;
>> +			ip6_rt_put(rt);
>> +		}
>> +	} else {
>> +		dev = result->idev->dev;
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	return dev;
>> +}
>> +EXPORT_SYMBOL(ipv6_dev_find);
>> +
>>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
>>  				     struct net_device *dev, int strict)
>>  {
>>

Re: [PATCH net 0/2] net: fix a mcast issue for tipc udp media

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon,  3 Aug 2020 23:34:45 +0800

> Patch 1 is to add a function to get the dev by source address,
> whcih will be used by Patch 2.

Series applied, thank you.

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Hideaki Yoshifuji]

Hi,

2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
>
> This is to add an ip_dev_find like function for ipv6, used to find
> the dev by saddr.
>
> It will be used by TIPC protocol. So also export it.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/addrconf.h |  2 ++
>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index 8418b7d..ba3f6c15 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
>
>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
>
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> +

How do we handle link-local addresses?

--yoshfuji

>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
>                                      const struct in6_addr *addr,
>                                      struct net_device *dev, int strict);
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 840bfdb..857d6f9 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ipv6_chk_prefix);
>
> +/**
> + * ipv6_dev_find - find the first device with a given source address.
> + * @net: the net namespace
> + * @addr: the source address
> + *
> + * The caller should be protected by RCU, or RTNL.
> + */
> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr)
> +{
> +       unsigned int hash = inet6_addr_hash(net, addr);
> +       struct inet6_ifaddr *ifp, *result = NULL;
> +       struct net_device *dev = NULL;
> +
> +       rcu_read_lock();
> +       hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
> +               if (net_eq(dev_net(ifp->idev->dev), net) &&
> +                   ipv6_addr_equal(&ifp->addr, addr)) {
> +                       result = ifp;
> +                       break;
> +               }
> +       }
> +
> +       if (!result) {
> +               struct rt6_info *rt;
> +
> +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> +               if (rt) {
> +                       dev = rt->dst.dev;
> +                       ip6_rt_put(rt);
> +               }
> +       } else {
> +               dev = result->idev->dev;
> +       }
> +       rcu_read_unlock();
> +
> +       return dev;
> +}
> +EXPORT_SYMBOL(ipv6_dev_find);
> +
>  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
>                                      struct net_device *dev, int strict)
>  {
> --
> 2.1.0
>

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Xin Long]

On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
<hideaki.yoshifuji@miraclelinux.com> wrote:
>
> Hi,
>
> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> >
> > This is to add an ip_dev_find like function for ipv6, used to find
> > the dev by saddr.
> >
> > It will be used by TIPC protocol. So also export it.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/addrconf.h |  2 ++
> >  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > index 8418b7d..ba3f6c15 100644
> > --- a/include/net/addrconf.h
> > +++ b/include/net/addrconf.h
> > @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> >
> >  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> >
> > +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> > +
>
> How do we handle link-local addresses?
This is what "if (!result)" branch meant to do:

+       if (!result) {
+               struct rt6_info *rt;
+
+               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
+               if (rt) {
+                       dev = rt->dst.dev;
+                       ip6_rt_put(rt);
+               }
+       } else {
+               dev = result->idev->dev;
+       }

Thanks.

>
> --yoshfuji
>
> >  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
> >                                      const struct in6_addr *addr,
> >                                      struct net_device *dev, int strict);
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 840bfdb..857d6f9 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -1983,6 +1983,45 @@ int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev)
> >  }
> >  EXPORT_SYMBOL(ipv6_chk_prefix);
> >
> > +/**
> > + * ipv6_dev_find - find the first device with a given source address.
> > + * @net: the net namespace
> > + * @addr: the source address
> > + *
> > + * The caller should be protected by RCU, or RTNL.
> > + */
> > +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr)
> > +{
> > +       unsigned int hash = inet6_addr_hash(net, addr);
> > +       struct inet6_ifaddr *ifp, *result = NULL;
> > +       struct net_device *dev = NULL;
> > +
> > +       rcu_read_lock();
> > +       hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
> > +               if (net_eq(dev_net(ifp->idev->dev), net) &&
> > +                   ipv6_addr_equal(&ifp->addr, addr)) {
> > +                       result = ifp;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!result) {
> > +               struct rt6_info *rt;
> > +
> > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > +               if (rt) {
> > +                       dev = rt->dst.dev;
> > +                       ip6_rt_put(rt);
> > +               }
> > +       } else {
> > +               dev = result->idev->dev;
> > +       }
> > +       rcu_read_unlock();
> > +
> > +       return dev;
> > +}
> > +EXPORT_SYMBOL(ipv6_dev_find);
> > +
> >  struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
> >                                      struct net_device *dev, int strict)
> >  {
> > --
> > 2.1.0
> >

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[David Ahern]

On 8/6/20 2:55 AM, Xin Long wrote:
> On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> <hideaki.yoshifuji@miraclelinux.com> wrote:
>>
>> Hi,
>>
>> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
>>>
>>> This is to add an ip_dev_find like function for ipv6, used to find
>>> the dev by saddr.
>>>
>>> It will be used by TIPC protocol. So also export it.
>>>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>  include/net/addrconf.h |  2 ++
>>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 41 insertions(+)
>>>
>>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>>> index 8418b7d..ba3f6c15 100644
>>> --- a/include/net/addrconf.h
>>> +++ b/include/net/addrconf.h
>>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
>>>
>>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
>>>
>>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
>>> +
>>
>> How do we handle link-local addresses?
> This is what "if (!result)" branch meant to do:
> 
> +       if (!result) {
> +               struct rt6_info *rt;
> +
> +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> +               if (rt) {
> +                       dev = rt->dst.dev;
> +                       ip6_rt_put(rt);
> +               }
> +       } else {
> +               dev = result->idev->dev;
> +       }
> 

the stated purpose of this function is to find the netdevice to which an
address is attached. A route lookup should not be needed. Walking the
address hash list finds the address and hence the netdev or it does not.

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Xin Long]

On Thu, Aug 6, 2020 at 10:03 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 8/6/20 2:55 AM, Xin Long wrote:
> > On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> > <hideaki.yoshifuji@miraclelinux.com> wrote:
> >>
> >> Hi,
> >>
> >> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> >>>
> >>> This is to add an ip_dev_find like function for ipv6, used to find
> >>> the dev by saddr.
> >>>
> >>> It will be used by TIPC protocol. So also export it.
> >>>
> >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >>> ---
> >>>  include/net/addrconf.h |  2 ++
> >>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 41 insertions(+)
> >>>
> >>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> >>> index 8418b7d..ba3f6c15 100644
> >>> --- a/include/net/addrconf.h
> >>> +++ b/include/net/addrconf.h
> >>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> >>>
> >>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> >>>
> >>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> >>> +
> >>
> >> How do we handle link-local addresses?
> > This is what "if (!result)" branch meant to do:
> >
> > +       if (!result) {
> > +               struct rt6_info *rt;
> > +
> > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > +               if (rt) {
> > +                       dev = rt->dst.dev;
> > +                       ip6_rt_put(rt);
> > +               }
> > +       } else {
> > +               dev = result->idev->dev;
> > +       }
> >
>
> the stated purpose of this function is to find the netdevice to which an
> address is attached. A route lookup should not be needed. Walking the
> address hash list finds the address and hence the netdev or it does not.
Hi, David,
Sorry. it does. I misunderstood the code in __ip_dev_find().
I will delete the rt6_lookup() part from ipv6_dev_find().

Also for the compatibility, tipc part should change to:
@@ -741,10 +741,8 @@ static int tipc_udp_enable(struct net *net,
struct tipc_bearer *b,
                struct net_device *dev;

               dev = ipv6_dev_find(net, &local.ipv6);
               if (!dev)
                      ub->ifindex = dev->ifindex;

as when dev is not found from the hash list, it should fall back to
the old tipc code.

Ying, what do you think?

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Hideaki Yoshifuji]

Hi,

2020年8月6日(木) 23:03 David Ahern <dsahern@gmail.com>:
>
> On 8/6/20 2:55 AM, Xin Long wrote:
> > On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> > <hideaki.yoshifuji@miraclelinux.com> wrote:
> >>
> >> Hi,
> >>
> >> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> >>>
> >>> This is to add an ip_dev_find like function for ipv6, used to find
> >>> the dev by saddr.
> >>>
> >>> It will be used by TIPC protocol. So also export it.
> >>>
> >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >>> ---
> >>>  include/net/addrconf.h |  2 ++
> >>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 41 insertions(+)
> >>>
> >>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> >>> index 8418b7d..ba3f6c15 100644
> >>> --- a/include/net/addrconf.h
> >>> +++ b/include/net/addrconf.h
> >>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> >>>
> >>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> >>>
> >>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> >>> +
> >>
> >> How do we handle link-local addresses?
> > This is what "if (!result)" branch meant to do:
> >
> > +       if (!result) {
> > +               struct rt6_info *rt;
> > +
> > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > +               if (rt) {
> > +                       dev = rt->dst.dev;
> > +                       ip6_rt_put(rt);
> > +               }
> > +       } else {
> > +               dev = result->idev->dev;
> > +       }
> >
>
> the stated purpose of this function is to find the netdevice to which an
> address is attached. A route lookup should not be needed. Walking the
> address hash list finds the address and hence the netdev or it does not.
>
>

User supplied scope id which should be set for link-local addresses
in TIPC_NLA_UDP_LOCAL attribute must be honored when we
check the address.

ipv6_chk_addr() can check if the address and supplied ifindex is a valid
local address.  Or introduce an extra ifindex argument to ipv6_dev_find().

--yoshfuji

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Xin Long]

On Fri, Aug 7, 2020 at 5:26 PM Hideaki Yoshifuji
<hideaki.yoshifuji@miraclelinux.com> wrote:
>
> Hi,
>
> 2020年8月6日(木) 23:03 David Ahern <dsahern@gmail.com>:
> >
> > On 8/6/20 2:55 AM, Xin Long wrote:
> > > On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> > > <hideaki.yoshifuji@miraclelinux.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> > >>>
> > >>> This is to add an ip_dev_find like function for ipv6, used to find
> > >>> the dev by saddr.
> > >>>
> > >>> It will be used by TIPC protocol. So also export it.
> > >>>
> > >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > >>> ---
> > >>>  include/net/addrconf.h |  2 ++
> > >>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> > >>>  2 files changed, 41 insertions(+)
> > >>>
> > >>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > >>> index 8418b7d..ba3f6c15 100644
> > >>> --- a/include/net/addrconf.h
> > >>> +++ b/include/net/addrconf.h
> > >>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> > >>>
> > >>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> > >>>
> > >>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> > >>> +
> > >>
> > >> How do we handle link-local addresses?
> > > This is what "if (!result)" branch meant to do:
> > >
> > > +       if (!result) {
> > > +               struct rt6_info *rt;
> > > +
> > > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > > +               if (rt) {
> > > +                       dev = rt->dst.dev;
> > > +                       ip6_rt_put(rt);
> > > +               }
> > > +       } else {
> > > +               dev = result->idev->dev;
> > > +       }
> > >
> >
> > the stated purpose of this function is to find the netdevice to which an
> > address is attached. A route lookup should not be needed. Walking the
> > address hash list finds the address and hence the netdev or it does not.
> >
> >
>
> User supplied scope id which should be set for link-local addresses
> in TIPC_NLA_UDP_LOCAL attribute must be honored when we
> check the address.
Hi, Hideaki san,

Sorry for not understanding your comment earlier.

The bad thing is tipc in iproute2 doesn't seem able to set scope_id.
I saw many places in kernel doing this check:

                         if (__ipv6_addr_needs_scope_id(atype) &&
                             !ip6->sin6_scope_id) { return -EINVAL; }

Can I ask why scope id is needed for link-local addresses?
and is that for link-local addresses only?

>
> ipv6_chk_addr() can check if the address and supplied ifindex is a valid
> local address.  Or introduce an extra ifindex argument to ipv6_dev_find().
Yeah, but if scope id means ifindex for  link-local addresses, ipv6_dev_find()
would be more like a function to validate the address with right scope id.

Thanks for your reviewing.

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Hideaki Yoshifuji]

Hi,

2020年8月9日(日) 19:52 Xin Long <lucien.xin@gmail.com>:
>
> On Fri, Aug 7, 2020 at 5:26 PM Hideaki Yoshifuji
> <hideaki.yoshifuji@miraclelinux.com> wrote:
> >
> > Hi,
> >
> > 2020年8月6日(木) 23:03 David Ahern <dsahern@gmail.com>:
> > >
> > > On 8/6/20 2:55 AM, Xin Long wrote:
> > > > On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> > > > <hideaki.yoshifuji@miraclelinux.com> wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> > > >>>
> > > >>> This is to add an ip_dev_find like function for ipv6, used to find
> > > >>> the dev by saddr.
> > > >>>
> > > >>> It will be used by TIPC protocol. So also export it.
> > > >>>
> > > >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > >>> ---
> > > >>>  include/net/addrconf.h |  2 ++
> > > >>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> > > >>>  2 files changed, 41 insertions(+)
> > > >>>
> > > >>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > > >>> index 8418b7d..ba3f6c15 100644
> > > >>> --- a/include/net/addrconf.h
> > > >>> +++ b/include/net/addrconf.h
> > > >>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> > > >>>
> > > >>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> > > >>>
> > > >>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> > > >>> +
> > > >>
> > > >> How do we handle link-local addresses?
> > > > This is what "if (!result)" branch meant to do:
> > > >
> > > > +       if (!result) {
> > > > +               struct rt6_info *rt;
> > > > +
> > > > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > > > +               if (rt) {
> > > > +                       dev = rt->dst.dev;
> > > > +                       ip6_rt_put(rt);
> > > > +               }
> > > > +       } else {
> > > > +               dev = result->idev->dev;
> > > > +       }
> > > >
> > >
> > > the stated purpose of this function is to find the netdevice to which an
> > > address is attached. A route lookup should not be needed. Walking the
> > > address hash list finds the address and hence the netdev or it does not.
> > >
> > >
> >
> > User supplied scope id which should be set for link-local addresses
> > in TIPC_NLA_UDP_LOCAL attribute must be honored when we
> > check the address.
> Hi, Hideaki san,
>
> Sorry for not understanding your comment earlier.
>
> The bad thing is tipc in iproute2 doesn't seem able to set scope_id.

I looked into the iproute2 code quickly and I think it should; it uses
getaddrinfo(3) and it will fill if you say "fe80::1%eth0" or something
like that.... OR, fix the bug.

> I saw many places in kernel doing this check:
>
>                          if (__ipv6_addr_needs_scope_id(atype) &&
>                              !ip6->sin6_scope_id) { return -EINVAL; }
>
> Can I ask why scope id is needed for link-local addresses?
> and is that for link-local addresses only?

Because we distinguish link-local scope addresses on different interfaces.
On the other hand, we do not distinguish global scope addresses on
different interfaces.

>
> >
> > ipv6_chk_addr() can check if the address and supplied ifindex is a valid
> > local address.  Or introduce an extra ifindex argument to ipv6_dev_find().
> Yeah, but if scope id means ifindex for  link-local addresses, ipv6_dev_find()
> would be more like a function to validate the address with right scope id.
>

I think we should find a net_device with a specific "valid" (non-tentative)
address here, and your initial implementation is not enough because it does
not reject tentative addresses.  I'd recommend using generic ipv6_chk_addr()
inside.

> Thanks for your reviewing.

Re: [PATCH net 1/2] ipv6: add ipv6_dev_find()

[Xin Long]

On Tue, Aug 11, 2020 at 10:26 AM Hideaki Yoshifuji
<hideaki.yoshifuji@miraclelinux.com> wrote:
>
> Hi,
>
> 2020年8月9日(日) 19:52 Xin Long <lucien.xin@gmail.com>:
> >
> > On Fri, Aug 7, 2020 at 5:26 PM Hideaki Yoshifuji
> > <hideaki.yoshifuji@miraclelinux.com> wrote:
> > >
> > > Hi,
> > >
> > > 2020年8月6日(木) 23:03 David Ahern <dsahern@gmail.com>:
> > > >
> > > > On 8/6/20 2:55 AM, Xin Long wrote:
> > > > > On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
> > > > > <hideaki.yoshifuji@miraclelinux.com> wrote:
> > > > >>
> > > > >> Hi,
> > > > >>
> > > > >> 2020年8月4日(火) 0:35 Xin Long <lucien.xin@gmail.com>:
> > > > >>>
> > > > >>> This is to add an ip_dev_find like function for ipv6, used to find
> > > > >>> the dev by saddr.
> > > > >>>
> > > > >>> It will be used by TIPC protocol. So also export it.
> > > > >>>
> > > > >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > >>> ---
> > > > >>>  include/net/addrconf.h |  2 ++
> > > > >>>  net/ipv6/addrconf.c    | 39 +++++++++++++++++++++++++++++++++++++++
> > > > >>>  2 files changed, 41 insertions(+)
> > > > >>>
> > > > >>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> > > > >>> index 8418b7d..ba3f6c15 100644
> > > > >>> --- a/include/net/addrconf.h
> > > > >>> +++ b/include/net/addrconf.h
> > > > >>> @@ -97,6 +97,8 @@ bool ipv6_chk_custom_prefix(const struct in6_addr *addr,
> > > > >>>
> > > > >>>  int ipv6_chk_prefix(const struct in6_addr *addr, struct net_device *dev);
> > > > >>>
> > > > >>> +struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr);
> > > > >>> +
> > > > >>
> > > > >> How do we handle link-local addresses?
> > > > > This is what "if (!result)" branch meant to do:
> > > > >
> > > > > +       if (!result) {
> > > > > +               struct rt6_info *rt;
> > > > > +
> > > > > +               rt = rt6_lookup(net, addr, NULL, 0, NULL, 0);
> > > > > +               if (rt) {
> > > > > +                       dev = rt->dst.dev;
> > > > > +                       ip6_rt_put(rt);
> > > > > +               }
> > > > > +       } else {
> > > > > +               dev = result->idev->dev;
> > > > > +       }
> > > > >
> > > >
> > > > the stated purpose of this function is to find the netdevice to which an
> > > > address is attached. A route lookup should not be needed. Walking the
> > > > address hash list finds the address and hence the netdev or it does not.
> > > >
> > > >
> > >
> > > User supplied scope id which should be set for link-local addresses
> > > in TIPC_NLA_UDP_LOCAL attribute must be honored when we
> > > check the address.
> > Hi, Hideaki san,
> >
> > Sorry for not understanding your comment earlier.
> >
> > The bad thing is tipc in iproute2 doesn't seem able to set scope_id.
>
> I looked into the iproute2 code quickly and I think it should; it uses
> getaddrinfo(3) and it will fill if you say "fe80::1%eth0" or something
> like that.... OR, fix the bug.
right, thanks.

>
> > I saw many places in kernel doing this check:
> >
> >                          if (__ipv6_addr_needs_scope_id(atype) &&
> >                              !ip6->sin6_scope_id) { return -EINVAL; }
> >
> > Can I ask why scope id is needed for link-local addresses?
> > and is that for link-local addresses only?
>
> Because we distinguish link-local scope addresses on different interfaces.
> On the other hand, we do not distinguish global scope addresses on
> different interfaces.
okay.

>
> >
> > >
> > > ipv6_chk_addr() can check if the address and supplied ifindex is a valid
> > > local address.  Or introduce an extra ifindex argument to ipv6_dev_find().
> > Yeah, but if scope id means ifindex for  link-local addresses, ipv6_dev_find()
> > would be more like a function to validate the address with right scope id.
> >
>
> I think we should find a net_device with a specific "valid" (non-tentative)
> address here, and your initial implementation is not enough because it does
> not reject tentative addresses.  I'd recommend using generic ipv6_chk_addr()
> inside.
ipv6_chk_addr() is calling ipv6_chk_addr_and_flags(), which traverses
the addr hash list again. So I'm thinking to reuse the code of
ipv6_chk_addr_and_flags(), and do:

+static struct net_device *
+__ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
+                         const struct net_device *dev, bool skip_dev_check,
+                         int strict, u32 banned_flags)
 {
        unsigned int hash = inet6_addr_hash(net, addr);
        const struct net_device *l3mdev;
@@ -1926,12 +1918,29 @@ int ipv6_chk_addr_and_flags(struct net *net,
const struct in6_addr *addr,
                    (!dev || ifp->idev->dev == dev ||
                     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
                        rcu_read_unlock();
-                       return 1;
+                       return ifp->idev->dev;
                }
        }

        rcu_read_unlock();
-       return 0;
+       return NULL;
+}

and change these functions to :

int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
                            const struct net_device *dev, bool skip_dev_check,
                            int strict, u32 banned_flags)
{
        return __ipv6_chk_addr_and_flags(net, addr, dev, skip_dev_check,
                                         strict, banned_flags) ? 1 : 0;
}
EXPORT_SYMBOL(ipv6_chk_addr_and_flags);

struct net_device *ipv6_dev_find(struct net *net, const struct in6_addr *addr,
                                 struct net_device *dev)
{
        return __ipv6_chk_addr_and_flags(net, addr, NULL, 0, 1,
                                         IFA_F_TENTATIVE);
}
EXPORT_SYMBOL(ipv6_dev_find);

what do you think?