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

On Tue, Aug 11, 2020 at 10:26 AM Hideaki Yoshifuji
[off-list ref] wrote:

Hi,

2020年8月9日(日) 19:52 Xin Long [off-list ref]:

quoted

On Fri, Aug 7, 2020 at 5:26 PM Hideaki Yoshifuji
[off-list ref] wrote:

quoted

Hi,

2020年8月6日(木) 23:03 David Ahern [off-list ref]:

quoted

On 8/6/20 2:55 AM, Xin Long wrote:

quoted

On Thu, Aug 6, 2020 at 10:50 AM Hideaki Yoshifuji
[off-list ref] wrote:

quoted

Hi,

2020年8月4日(火) 0:35 Xin Long [off-list ref]:

quoted

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.

quoted

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.

quoted

quoted

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?