Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca]

2018-02-27, 10:47:08 -0500, David Miller wrote:
> From: Sabrina Dubroca 
> Date: Tue, 27 Feb 2018 15:13:28 +0100
> 
> > 2018-02-26, 12:11:27 -0500, David Miller wrote:
> >> From: Sabrina Dubroca 
> >> Date: Mon, 26 Feb 2018 17:56:19 +0100
> >> 
> >> That's completely different to this case, which is a bonfide explicit
> >> allowance for userspace to take over these fundamental protocol tasks
> >> from the kernel.
> > 
> > This is not letting userspace take over. On the contrary, it allows
> > userspace to take advantage of the kernel's DAD, without suffering the
> > delay. The alternative, without optimistic DAD, would be to completely
> > disable DAD done by the kernel.
> > 
> > This follows RFC 4429, which explicitly allows optimistic DAD for
> > addresses generated by (among other mechanisms) DHCPv6.
> 
> Fair enough.
> 
> We can resume this conversation if and when problems pop up in the
> future :-)

Hehe :)

Thanks, I'll submit a v2 with the changes (the other) David asked for.

-- 
Sabrina

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[David Miller]

From: Sabrina Dubroca 
Date: Tue, 27 Feb 2018 15:13:28 +0100

> 2018-02-26, 12:11:27 -0500, David Miller wrote:
>> From: Sabrina Dubroca 
>> Date: Mon, 26 Feb 2018 17:56:19 +0100
>> 
>> That's completely different to this case, which is a bonfide explicit
>> allowance for userspace to take over these fundamental protocol tasks
>> from the kernel.
> 
> This is not letting userspace take over. On the contrary, it allows
> userspace to take advantage of the kernel's DAD, without suffering the
> delay. The alternative, without optimistic DAD, would be to completely
> disable DAD done by the kernel.
> 
> This follows RFC 4429, which explicitly allows optimistic DAD for
> addresses generated by (among other mechanisms) DHCPv6.

Fair enough.

We can resume this conversation if and when problems pop up in the
future :-)

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca]

2018-02-26, 12:11:27 -0500, David Miller wrote:
> From: Sabrina Dubroca 
> Date: Mon, 26 Feb 2018 17:56:19 +0100
> 
> > 2018-02-26, 10:57:11 -0500, David Miller wrote:
> >> Userland is now repsonsible for implementing correct behavior when it
> >> takes over this task, and therefore the kernel has no say in the
> >> matter of proper ipv6 neighbor discovery and addrconf behavior.
> > 
> > As an aside, that's also the case whenever userland uses packet
> > sockets.
> 
> When you use packet sockets, all bets are off and it is clearly the
> case that the user gets to keep the broken pieces when things go
> wrong.

Ok, I see your point.

> That's completely different to this case, which is a bonfide explicit
> allowance for userspace to take over these fundamental protocol tasks
> from the kernel.

This is not letting userspace take over. On the contrary, it allows
userspace to take advantage of the kernel's DAD, without suffering the
delay. The alternative, without optimistic DAD, would be to completely
disable DAD done by the kernel.

This follows RFC 4429, which explicitly allows optimistic DAD for
addresses generated by (among other mechanisms) DHCPv6.

-- 
Sabrina

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[David Miller]

From: Sabrina Dubroca 
Date: Mon, 26 Feb 2018 17:56:19 +0100

> 2018-02-26, 10:57:11 -0500, David Miller wrote:
>> Userland is now repsonsible for implementing correct behavior when it
>> takes over this task, and therefore the kernel has no say in the
>> matter of proper ipv6 neighbor discovery and addrconf behavior.
> 
> As an aside, that's also the case whenever userland uses packet
> sockets.

When you use packet sockets, all bets are off and it is clearly the
case that the user gets to keep the broken pieces when things go
wrong.

That's completely different to this case, which is a bonfide explicit
allowance for userspace to take over these fundamental protocol tasks
from the kernel.

So please do not use packet sockets as an example of a similar
situation.

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca]

2018-02-26, 10:57:11 -0500, David Miller wrote:
> From: Sabrina Dubroca 
> Date: Mon, 26 Feb 2018 16:41:32 +0100
> 
> > What are you concerned about, if we let userspace set this flag?
> 
> I am concerned that the kernel is no longer in charge of making sure
> that all of the RFC rules are met in this area.

This can already happen with IFA_F_NODAD or net.ipv6.conf.*.accept_dad.
We'll send packets using non-unique addresses.

> Userland is now repsonsible for implementing correct behavior when it
> takes over this task, and therefore the kernel has no say in the
> matter of proper ipv6 neighbor discovery and addrconf behavior.

As an aside, that's also the case whenever userland uses packet
sockets.

> Unlike with things like DHCP, addrconf et al. in ipv6 are
> fundamentally defined aspects of the protocol suite.
> 
> This division of responsibility means that we will also run into
> situations where who (kernel or user) must take care of X or Y might
> be ambiguous or hard to pin down in certain circumstances.

I don't think it's ambiguous here, but I can add documentation.

> I really don't like this situation where a fundamental protocol is
> conditionally the responsibility of the kernel, it's really bad design
> decision overall.

I understand. But I think with this patch, userspace could rely on the
kernel's DAD, instead of having to perform DAD itself in order to
avoid the delay that non-optimistic DAD introduces.

-- 
Sabrina

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[David Miller]

From: Sabrina Dubroca 
Date: Mon, 26 Feb 2018 16:41:32 +0100

> What are you concerned about, if we let userspace set this flag?

I am concerned that the kernel is no longer in charge of making sure
that all of the RFC rules are met in this area.

Userland is now repsonsible for implementing correct behavior when it
takes over this task, and therefore the kernel has no say in the
matter of proper ipv6 neighbor discovery and addrconf behavior.

Unlike with things like DHCP, addrconf et al. in ipv6 are
fundamentally defined aspects of the protocol suite.

This division of responsibility means that we will also run into
situations where who (kernel or user) must take care of X or Y might
be ambiguous or hard to pin down in certain circumstances.

I really don't like this situation where a fundamental protocol is
conditionally the responsibility of the kernel, it's really bad design
decision overall.

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca]

2018-02-21, 15:34:21 -0500, David Miller wrote:
> From: Sabrina Dubroca 
> Date: Tue, 20 Feb 2018 19:17:17 +0100
> 
> > 2018-02-20, 10:25:41 -0700, David Ahern wrote:
> >> On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
> >> > According to RFC 4429 (section 3.1), adding new IPv6 addresses as
> >> > optimistic addresses is acceptable, as long as the implementation
> >> > follows some rules:
> >> > 
> >> >* Optimistic DAD SHOULD only be used when the implementation is aware
> >> > that the address is based on a most likely unique interface
> >> > identifier (such as in [RFC2464]), generated randomly [RFC3041],
> >> > or by a well-distributed hash function [RFC3972] or assigned by
> >> > Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
> >> > Optimistic DAD SHOULD NOT be used for manually entered
> >> > addresses.
> >> 
> >> That last line suggests this patch should not be allowed.
> > 
> > I think it should. Some tools perform autoconfiguration in userspace,
> > why should the kernel prevent them from requesting optimistic DAD?
> > 
> > If the administrator decides to enable optimistic DAD on
> > poorly-choosen addresses, or to disable DAD entirely, that's their
> > problem.
> 
> See, this is the slippery slope we go down once we have allowed
> userspace to engage in the ipv6 autoconfiguration process.
> 
> Whether the kernel is in control or not, or enforcing the rules
> properly, is always going to be ambiguous and hard to determine.
> 
> I somewhat regret allowing us to go down this path...

It's not just autoconf (slaac), but DHCPv6 as well. This would happen
in userspace and produce addresses expected to be unique as well.

What are you concerned about, if we let userspace set this flag?

-- 
Sabrina

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[David Miller]

From: Sabrina Dubroca 
Date: Tue, 20 Feb 2018 19:17:17 +0100

> 2018-02-20, 10:25:41 -0700, David Ahern wrote:
>> On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
>> > According to RFC 4429 (section 3.1), adding new IPv6 addresses as
>> > optimistic addresses is acceptable, as long as the implementation
>> > follows some rules:
>> > 
>> >* Optimistic DAD SHOULD only be used when the implementation is aware
>> > that the address is based on a most likely unique interface
>> > identifier (such as in [RFC2464]), generated randomly [RFC3041],
>> > or by a well-distributed hash function [RFC3972] or assigned by
>> > Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
>> > Optimistic DAD SHOULD NOT be used for manually entered
>> > addresses.
>> 
>> That last line suggests this patch should not be allowed.
> 
> I think it should. Some tools perform autoconfiguration in userspace,
> why should the kernel prevent them from requesting optimistic DAD?
> 
> If the administrator decides to enable optimistic DAD on
> poorly-choosen addresses, or to disable DAD entirely, that's their
> problem.

See, this is the slippery slope we go down once we have allowed
userspace to engage in the ipv6 autoconfiguration process.

Whether the kernel is in control or not, or enforcing the rules
properly, is always going to be ambiguous and hard to determine.

I somewhat regret allowing us to go down this path...

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca]

2018-02-20, 10:25:41 -0700, David Ahern wrote:
> On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
> > According to RFC 4429 (section 3.1), adding new IPv6 addresses as
> > optimistic addresses is acceptable, as long as the implementation
> > follows some rules:
> > 
> >* Optimistic DAD SHOULD only be used when the implementation is aware
> > that the address is based on a most likely unique interface
> > identifier (such as in [RFC2464]), generated randomly [RFC3041],
> > or by a well-distributed hash function [RFC3972] or assigned by
> > Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
> > Optimistic DAD SHOULD NOT be used for manually entered
> > addresses.
> 
> That last line suggests this patch should not be allowed.

I think it should. Some tools perform autoconfiguration in userspace,
why should the kernel prevent them from requesting optimistic DAD?

If the administrator decides to enable optimistic DAD on
poorly-choosen addresses, or to disable DAD entirely, that's their
problem.


> But if it is ...
> 
> > 
> > Thus, it seems reasonable to allow userspace to set the optimistic flag
> > when adding new addresses.
> > 
> > We must not let userspace set NODAD + OPTIMISTIC, since if the kernel is
> > not performing DAD we would never clear the optimistic flag. We must
> > also ignore userspace's request to add OPTIMISTIC flag to addresses that
> > have already completed DAD.
> > 
> > Then we also need to clear the OPTIMISTIC flag on permanent addresses
> > when DAD fails. Otherwise, IFA_F_OPTIMISTIC addresses added by userspace
> > can still be used after DAD has failed, because in
> > ipv6_chk_addr_and_flags(), IFA_F_OPTIMISTIC overrides IFA_F_TENTATIVE.
> > 
> > Signed-off-by: Sabrina Dubroca 
> > ---
> >  net/ipv6/addrconf.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 4facfe0b1888..652285bae801 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -1968,6 +1968,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr 
> > *ifp, int dad_failed)
> > spin_lock_bh(>lock);
> > addrconf_del_dad_work(ifp);
> > ifp->flags |= IFA_F_TENTATIVE;
> > +   ifp->flags &= ~IFA_F_OPTIMISTIC;
> > spin_unlock_bh(>lock);
> > if (dad_failed)
> > ipv6_ifa_notify(0, ifp);
> > @@ -4501,6 +4502,9 @@ static int inet6_addr_modify(struct inet6_ifaddr 
> > *ifp, u32 ifa_flags,
> > (ifp->flags & IFA_F_TEMPORARY || ifp->prefix_len != 64))
> > return -EINVAL;
> >  
> > +   if (!(ifp->flags & (IFA_F_TENTATIVE | IFA_F_DADFAILED)))
> > +   ifa_flags &= ~IFA_F_OPTIMISTIC;
> > +
> > timeout = addrconf_timeout_fixup(valid_lft, HZ);
> > if (addrconf_finite_timeout(timeout)) {
> > expires = jiffies_to_clock_t(timeout * HZ);
> > @@ -4607,7 +4611,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct 
> > nlmsghdr *nlh,
> >  
> > /* We ignore other flags so far. */
> > ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> > -IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN;
> > +IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN | IFA_F_OPTIMISTIC;
> > +
> > +   if (ifa_flags & IFA_F_NODAD && ifa_flags & IFA_F_OPTIMISTIC)
> > +   return -EINVAL;
> 
> ... add an extack message telling users nodad and optimistic are
> mutually exclusive.

Ok, I'll do that in v2.


> Also, it seems like this feature needs to be wrapped in
> CONFIG_IPV6_OPTIMISTIC_DAD and optimistic checks for linklocal and
> autoconf are wrapped in sysctl checks. Why shouldn't manual addresses
> follow suit?

Good catch, they should, will fix in v2.

And while we're talking about CONFIG_IPV6_OPTIMISTIC_DAD: it has been
labeled "experimental" for almost 11 years, maybe it's time to drop
the label?  Most distributions seem to enable it despite the
"If unsure, say N." advice in Kconfig.

Thanks for the comments.

-- 
Sabrina

Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[David Ahern]

On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
> According to RFC 4429 (section 3.1), adding new IPv6 addresses as
> optimistic addresses is acceptable, as long as the implementation
> follows some rules:
> 
>* Optimistic DAD SHOULD only be used when the implementation is aware
> that the address is based on a most likely unique interface
> identifier (such as in [RFC2464]), generated randomly [RFC3041],
> or by a well-distributed hash function [RFC3972] or assigned by
> Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
> Optimistic DAD SHOULD NOT be used for manually entered
> addresses.

That last line suggests this patch should not be allowed.

But if it is ...

> 
> Thus, it seems reasonable to allow userspace to set the optimistic flag
> when adding new addresses.
> 
> We must not let userspace set NODAD + OPTIMISTIC, since if the kernel is
> not performing DAD we would never clear the optimistic flag. We must
> also ignore userspace's request to add OPTIMISTIC flag to addresses that
> have already completed DAD.
> 
> Then we also need to clear the OPTIMISTIC flag on permanent addresses
> when DAD fails. Otherwise, IFA_F_OPTIMISTIC addresses added by userspace
> can still be used after DAD has failed, because in
> ipv6_chk_addr_and_flags(), IFA_F_OPTIMISTIC overrides IFA_F_TENTATIVE.
> 
> Signed-off-by: Sabrina Dubroca 
> ---
>  net/ipv6/addrconf.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4facfe0b1888..652285bae801 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1968,6 +1968,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp, 
> int dad_failed)
>   spin_lock_bh(>lock);
>   addrconf_del_dad_work(ifp);
>   ifp->flags |= IFA_F_TENTATIVE;
> + ifp->flags &= ~IFA_F_OPTIMISTIC;
>   spin_unlock_bh(>lock);
>   if (dad_failed)
>   ipv6_ifa_notify(0, ifp);
> @@ -4501,6 +4502,9 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, 
> u32 ifa_flags,
>   (ifp->flags & IFA_F_TEMPORARY || ifp->prefix_len != 64))
>   return -EINVAL;
>  
> + if (!(ifp->flags & (IFA_F_TENTATIVE | IFA_F_DADFAILED)))
> + ifa_flags &= ~IFA_F_OPTIMISTIC;
> +
>   timeout = addrconf_timeout_fixup(valid_lft, HZ);
>   if (addrconf_finite_timeout(timeout)) {
>   expires = jiffies_to_clock_t(timeout * HZ);
> @@ -4607,7 +4611,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr 
> *nlh,
>  
>   /* We ignore other flags so far. */
>   ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> -  IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN;
> +  IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN | IFA_F_OPTIMISTIC;
> +
> + if (ifa_flags & IFA_F_NODAD && ifa_flags & IFA_F_OPTIMISTIC)
> + return -EINVAL;

... add an extack message telling users nodad and optimistic are
mutually exclusive.


Also, it seems like this feature needs to be wrapped in
CONFIG_IPV6_OPTIMISTIC_DAD and optimistic checks for linklocal and
autoconf are wrapped in sysctl checks. Why shouldn't manual addresses
follow suit?

>  
>   ifa = ipv6_get_ifaddr(net, pfx, dev, 1);
>   if (!ifa) {
>