On 3/7/18 4:53 AM, Kirill Tkhai wrote:
>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>> index c4185a7b0e90..132e5b95167a 100644
>> --- a/include/net/addrconf.h
>> +++ b/include/net/addrconf.h
>> @@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user
>> *arg);
>> int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>> const struct net_device *dev, int strict);
>> int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> - const struct net_device *dev, int strict,
>> - u32 banned_flags);
>> + const struct net_device *dev, bool skip_dev_check,
>> + int strict, u32 banned_flags);
>
> This function already has 5 arguments, while this patch adds one more.
> Can't we use new flags argument for both of them?
>
> Also, the name of function and input parameters are already so big, that they
> don't fit a single line already, while your patch adds more parameters.
> Can't we make it more slim? Something like ipv6_chk_addr_fl() instead of
> current
> name.
I think I can combine strict and the new skip_dev_check. I am going to
leave the function name as is.
>
>> #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
>> int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index b5fd116c046a..17d5d3f42d21 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1851,22 +1851,40 @@ static int ipv6_count_addresses(const struct
>> inet6_dev *idev)
>> int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>> const struct net_device *dev, int strict)
>> {
>> - return ipv6_chk_addr_and_flags(net, addr, dev, strict, IFA_F_TENTATIVE);
>> + return ipv6_chk_addr_and_flags(net, addr, dev, !dev,
>> + strict, IFA_F_TENTATIVE);
>> }
>> EXPORT_SYMBOL(ipv6_chk_addr);
>
> This function was not introduced by this commit, but since the commit
> modifies it,
> and the function is pretty simple, we could declare it as static inline in
> header
> in separate patch.
That function is needed by netfilter code. Any consolidation is outside
the scope of this patch set.
>
>>
>> +/* device argument is used to find the L3 domain of interest. If
>> + * skip_dev_check is set, then the ifp device is not checked against
>> + * the passed in dev argument. So the 2 cases for addresses checks are:
>> + * 1. does the address exist in the L3 domain that dev is part of
>> + * (skip_dev_check = true), or
>> + *
>> + * 2. does the address exist on the specific device
>> + * (skip_dev_check = false)
>> + */
>> int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> - const struct net_device *dev, int strict,
>> - u32 banned_flags)
>> + 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;
>> struct inet6_ifaddr *ifp;
>> u32 ifp_flags;
>>
>> rcu_read_lock();
>> +
>> + l3mdev = l3mdev_master_dev_rcu(dev);
>> +
>> hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
>> if (!net_eq(dev_net(ifp->idev->dev), net))
>> continue;
>> +
>> + if (l3mdev_master_dev_rcu(ifp->idev->dev) != l3mdev)
>> + continue;
>> +
>> /* Decouple optimistic from tentative for evaluation here.
>> * Ban optimistic addresses explicitly, when required.
>> */
>> @@ -1875,7 +1893,7 @@ int ipv6_chk_addr_and_flags(struct net *net, const
>> struct in6_addr *addr,
>> : ifp->flags;
>> if (ipv6_addr_equal(&ifp->addr, addr) &&
>> !(ifp_flags&banned_flags) &&
>> - (!dev || ifp->idev->dev == dev ||
>> + (skip_dev_check || ifp->idev->dev == dev ||
>> !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>> rcu_read_unlock();
>> return 1;
>
> There are two logical pieces in changes of this function:
>
> 1)You become always pass not NULL dev and add skip_dev_check argument.
> 2)l3mdev_master_dev_rcu() check is introduced.
dev can still be null; l3mdev lookup handles a null argument.
>
> They should go in separate patches.
The change needs to go in together since I am altering how this function
works.
>
>> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
>> index c61718dba2e6..d580d4d456a5 100644
>> --- a/net/ipv6/anycast.c
>> +++ b/net/ipv6/anycast.c
>> @@ -66,7 +66,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const
>> struct in6_addr *addr)
>> return -EPERM;
>> if (ipv6_addr_is_multicast(addr))
>> return -EINVAL;
>> - if (ipv6_chk_addr(net, addr, NULL, 0))
>> +
>> + if (ifindex)
>> + dev = __dev_get_by_index(net, ifindex);
>> +
>> + if (ipv6_chk_addr_and_flags(net, addr, dev, true, 0, IFA_F_TENTATIVE))
>> return -EINVAL;
>>
>> pac = sock_kmalloc(sk, sizeof(struct ipv6_ac_socklist), GFP_KERNEL);
>> @@ -90,8 +94,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const
>> struct in6_addr *addr)
>> dev = __dev_get_by_flags(net, IFF_UP,
>> IFF_UP | IFF_LOOPBACK);
>> }
>> - } else
>> - dev = __dev_get_by_index(net, ifindex);
>> + }
>
> The hunk moving __dev_get_by_index() dereference may go as separate change,
> as it's a refactoring.
> This will make the review easier.
Moving 1 line of code up a few lines should not need a standalone patch;
I think the above is understandable.
>
>>
>> if (!dev) {
>> err = -ENODEV;
>> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
>> index fbf08ce3f5ab..b27333d7b099 100644
>> --- a/net/ipv6/datagram.c
>> +++ b/net/ipv6/datagram.c
>> @@ -801,8 +801,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock
>> *sk,
>> if (addr_type != IPV6_ADDR_ANY) {
>> int strict = __ipv6_addr_src_scope(addr_type)
>> <= IPV6_ADDR_SCOPE_LINKLOCAL;
>> if (!(inet_sk(sk)->freebind ||
>> inet_sk(sk)->transparent) &&
>> - !ipv6_chk_addr(net, &src_info->ipi6_addr,
>> - strict ? dev : NULL, 0) &&
>> + !ipv6_chk_addr_and_flags(net,
>> &src_info->ipi6_addr,
>> + dev, !strict, 0,
>> + IFA_F_TENTATIVE) &&
>> !ipv6_chk_acast_addr_src(net, dev,
>>
>> &src_info->ipi6_addr))
>> err = -EINVAL;
>> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
>> index 56c4967f1868..1ce8244e8aee 100644
>> --- a/net/ipv6/ip6_tunnel.c
>> +++ b/net/ipv6/ip6_tunnel.c
>> @@ -758,9 +758,11 @@ int ip6_tnl_rcv_ctl(struct ip6_tnl *t,
>> ldev = dev_get_by_index_rcu(net, p->link);
>>
>> if ((ipv6_addr_is_multicast(laddr) ||
>> - likely(ipv6_chk_addr(net, laddr, ldev, 0))) &&
>> + likely(ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> + 0, IFA_F_TENTATIVE))) &&
>> ((p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) ||
>> - likely(!ipv6_chk_addr(net, raddr, NULL, 0))))
>> + likely(!ipv6_chk_addr_and_flags(net, raddr, ldev, true,
>> + 0, IFA_F_TENTATIVE))))
>> ret = 1;
>> }
>> return ret;
>> @@ -990,12 +992,14 @@ int ip6_tnl_xmit_ctl(struct ip6_tnl *t,
>> if (p->link)
>> ldev = dev_get_by_index_rcu(net, p->link);
>>
>> - if (unlikely(!ipv6_chk_addr(net, laddr, ldev, 0)))
>> + if (unlikely(!ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> + 0, IFA_F_TENTATIVE)))
>> pr_warn("%s xmit: Local address not yet configured!\n",
>> p->name);
>> else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
>> !ipv6_addr_is_multicast(raddr) &&
>> - unlikely(ipv6_chk_addr(net, raddr, NULL, 0)))
>> + unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
>> + true, 0,
>> IFA_F_TENTATIVE)))
>> pr_warn("%s xmit: Routing loop! Remote address found on
>> this node!\n",
>> p->name);
>> else
>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>> index 0a19ce3a6f7f..13bf775c7f1a 100644
>> --- a/net/ipv6/ndisc.c
>> +++ b/net/ipv6/ndisc.c
>> @@ -707,7 +707,7 @@ static void ndisc_solicit(struct neighbour *neigh,
>> struct sk_buff *skb)
>> int probes = atomic_read(&neigh->probes);
>>
>> if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
>> - dev, 1,
>> + dev, false, 1,
>> IFA_F_TENTATIVE|IFA_F_OPTIMISTIC))
>> saddr = &ipv6_hdr(skb)->saddr;
>> probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 3851c3ccfd7a..bbc62799eb3b 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2633,18 +2633,25 @@ static int ip6_validate_gw(struct net *net, struct
>> fib6_config *cfg,
>> const struct in6_addr *gw_addr = &cfg->fc_gateway;
>> int gwa_type = ipv6_addr_type(gw_addr);
>> const struct net_device *dev = *_dev;
>> + bool need_local_addr_check = !dev;
>> int err = -EINVAL;
>>
>> - /* if gw_addr is local we will fail to detect this in case
>> - * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> - * will return already-added prefix route via interface that
>> - * prefix route was assigned to, which might be non-loopback.
>> + /* if route spec contains the device, check if gateway address
>> + * is a local address in the same L3 domain
>> */
>> - if (ipv6_chk_addr_and_flags(net, gw_addr,
>> - gwa_type & IPV6_ADDR_LINKLOCAL ?
>> - dev : NULL, 0, 0)) {
>> - NL_SET_ERR_MSG(extack, "Invalid gateway address");
>> - goto out;
>> + if (dev) {
>> + /* if gw_addr is local we will fail to detect this in case
>> + * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> + * will return already-added prefix route via interface that
>> + * prefix route was assigned to, which might be non-loopback.
>> + */
>> + if (ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> + gwa_type & IPV6_ADDR_LINKLOCAL ?
>> + false : true, 0, 0)) {
>> + NL_SET_ERR_MSG(extack,
>> + "Gateway can not be a local address");
>> + goto out;
>> + }
>
> Why do these two "if" go as tree, not as single "if (a && b)"?
yes, I should combine that.
>
>> }
>>
>> if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
>> @@ -2683,6 +2690,18 @@ static int ip6_validate_gw(struct net *net, struct
>> fib6_config *cfg,
>> "Egress device can not be loopback device for
>> this route");
>> goto out;
>> }
>> +
>> + /* if we did not check gw_addr above, do so now that the
>> + * egress device has been resolved.
>> + */
>> + if (need_local_addr_check &&
>
> Do we really need a variable with so long name? Can't we use "local_check" or
> something like this?
given the limited use of the flag, I believe the longer name is more
readable. Making it shorter does not allow the 2 checks to consolidate
into 1 line, so nothing is gained by an overly short variable name.
>
>> + ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> + gwa_type & IPV6_ADDR_LINKLOCAL ?
>> + false : true, 0, 0)) {
>
> Second time there is "gwa_type & IPV6_ADDR_LINKLOCAL ? false : true".
> gwa_type is constant,
> it doesn't change. Repeating constant expressions have to be be cached into
> local variable
> for improving readability.
they don't have to be but I will make a bool for it.
>
>> + NL_SET_ERR_MSG(extack, "Gateway can not be a local address");
>> + goto out;
>> + }
>> +
>> err = 0;
>> out:
>> return err;
>>
>
> Thanks,
> Kirill
>
