send fewer router solicitations

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

send fewer router solicitations

Florian Obser-2
Our kernel based rtsol code is like this little child.  We bring up
the interface, send our first solicitation and get an advertisment
back with a pltime of a week or so.

We lean back, quite happy that we can do v6 now, but after 60 seconds
we wake up, oh shit, better check if that prefix is still valid. and
so on and on.

This is particularly annoying if you try to debug ICMPv6 with tcpdump
in a network with a lot of openbsd machines.

To stop naddy from pestering me about this at every hackathon (rightly
so!), let's base the timeout on the prefixes pltime. ;)

OK?

diff --git nd6_rtr.c nd6_rtr.c
index 6ca25f0..4ac978f 100644
--- nd6_rtr.c
+++ nd6_rtr.c
@@ -75,6 +75,7 @@ int rt6_deleteroute(struct rtentry *, void *, unsigned int);
 void nd6_addr_add(void *);
 
 void nd6_rs_output_timo(void *);
+u_int32_t nd6_rs_next_pltime_timo(struct ifnet *);
 void nd6_rs_output_set_timo(int);
 void nd6_rs_output(struct ifnet *, struct in6_ifaddr *);
 void nd6_rs_dev_state(void *);
@@ -283,30 +284,63 @@ nd6_rs_output_set_timo(int timeout)
  timeout_add_sec(&nd6_rs_output_timer, nd6_rs_output_timeout);
 }
 
+u_int32_t
+nd6_rs_next_pltime_timo(struct ifnet *ifp)
+{
+ struct ifaddr *ifa;
+ struct in6_ifaddr *ia6;
+ u_int32_t pltime_expires = ND6_INFINITE_LIFETIME;
+
+ TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+ if (ifa->ifa_addr->sa_family != AF_INET6)
+ continue;
+
+ ia6 = ifatoia6(ifa);
+ if (ia6->ia6_lifetime.ia6t_pltime == ND6_INFINITE_LIFETIME ||
+    IFA6_IS_DEPRECATED(ia6) || IFA6_IS_INVALID(ia6))
+ continue;
+
+ pltime_expires = MIN(pltime_expires,
+    ia6->ia6_lifetime.ia6t_pltime);
+ }
+
+ return pltime_expires;
+}
+
 void
 nd6_rs_output_timo(void *ignored_arg)
 {
  struct ifnet *ifp;
  struct in6_ifaddr *ia6;
+ u_int32_t pltime_expire = ND6_INFINITE_LIFETIME, t;
+ int timeout = ND6_RS_OUTPUT_INTERVAL;
 
  if (nd6_rs_timeout_count == 0)
  return;
 
  if (nd6_rs_output_timeout < ND6_RS_OUTPUT_INTERVAL)
  /* exponential backoff if running quick timeouts */
- nd6_rs_output_timeout *= 2;
- if (nd6_rs_output_timeout > ND6_RS_OUTPUT_INTERVAL)
- nd6_rs_output_timeout = ND6_RS_OUTPUT_INTERVAL;
+ timeout = nd6_rs_output_timeout * 2;
 
  TAILQ_FOREACH(ifp, &ifnet, if_list) {
  if (ISSET(ifp->if_flags, IFF_RUNNING) &&
     ISSET(ifp->if_xflags, IFXF_AUTOCONF6)) {
- ia6 = in6ifa_ifpforlinklocal(ifp, IN6_IFF_TENTATIVE);
- if (ia6 != NULL)
- nd6_rs_output(ifp, ia6);
+ t = nd6_rs_next_pltime_timo(ifp);
+ if (t == ND6_INFINITE_LIFETIME || t <
+    ND6_RS_OUTPUT_INTERVAL) {
+ ia6 = in6ifa_ifpforlinklocal(ifp,
+    IN6_IFF_TENTATIVE);
+ if (ia6 != NULL)
+ nd6_rs_output(ifp, ia6);
+ }
+
+ pltime_expire = MIN(pltime_expire, t);
  }
  }
- nd6_rs_output_set_timo(nd6_rs_output_timeout);
+ if (pltime_expire != ND6_INFINITE_LIFETIME)
+ timeout = MAX(timeout, pltime_expire / 2);
+
+ nd6_rs_output_set_timo(timeout);
 }
 
 void


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Stuart Henderson
On 2016/09/02 10:37, Florian Obser wrote:

> Our kernel based rtsol code is like this little child.  We bring up
> the interface, send our first solicitation and get an advertisment
> back with a pltime of a week or so.
>
> We lean back, quite happy that we can do v6 now, but after 60 seconds
> we wake up, oh shit, better check if that prefix is still valid. and
> so on and on.
>
> This is particularly annoying if you try to debug ICMPv6 with tcpdump
> in a network with a lot of openbsd machines.
>
> To stop naddy from pestering me about this at every hackathon (rightly
> so!), let's base the timeout on the prefixes pltime. ;)
>
> OK?

I agree with doing this, but I think this makes it more important
that we do something to expire old prefixes when changing networks (i.e.
change of link state).

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Stuart Henderson
In reply to this post by Florian Obser-2
On 2016/09/02 10:37, Florian Obser wrote:
> To stop naddy from pestering me about this at every hackathon (rightly
> so!), let's base the timeout on the prefixes pltime. ;)

Just a thought, are we going to need to cap this to the RDNSS time too when
we start caring about that?

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Florian Obser-2
On Fri, Sep 02, 2016 at 05:49:22PM +0100, Stuart Henderson wrote:
> On 2016/09/02 10:37, Florian Obser wrote:
> > To stop naddy from pestering me about this at every hackathon (rightly
> > so!), let's base the timeout on the prefixes pltime. ;)
>
> Just a thought, are we going to need to cap this to the RDNSS time too when
> we start caring about that?
>

I will not parse the DNS options in the kernel. See CVE-2014-3954 ;)

I imagine when the time comes that we care we have the router
advertisement parsing in userland and solicitations will again be send
from userland, too.

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Stuart Henderson
In reply to this post by Florian Obser-2
On 2016/09/02 10:37, Florian Obser wrote:

> Our kernel based rtsol code is like this little child.  We bring up
> the interface, send our first solicitation and get an advertisment
> back with a pltime of a week or so.
>
> We lean back, quite happy that we can do v6 now, but after 60 seconds
> we wake up, oh shit, better check if that prefix is still valid. and
> so on and on.
>
> This is particularly annoying if you try to debug ICMPv6 with tcpdump
> in a network with a lot of openbsd machines.
>
> To stop naddy from pestering me about this at every hackathon (rightly
> so!), let's base the timeout on the prefixes pltime. ;)

There's a problem with this: we lose the exponential backoff for the
quick timer. Say you have v6 at home and enable autoconf on your laptop
then move to a network without v6 - this results in you spamming the
network with a multicast frame every second, which does not make you
a good network citizen especially if that's on a wireless lan.

 310 void
 311 nd6_rs_output_timo(void *ignored_arg)
 312 {
 313         struct ifnet *ifp;
 314         struct in6_ifaddr *ia6;
 315         u_int32_t pltime_expire = ND6_INFINITE_LIFETIME, t;
 316         int timeout = ND6_RS_OUTPUT_INTERVAL;
 318         if (nd6_rs_timeout_count == 0)
 319                 return;
 320                
 321         if (nd6_rs_output_timeout < ND6_RS_OUTPUT_INTERVAL)
 322                 /* exponential backoff if running quick timeouts */
 323                 timeout = nd6_rs_output_timeout * 2;

I've tried a few things along the lines of these here instead

             if (nd6_rs_output_timeout < ND6_RS_OUTPUT_INTERVAL) {
                        nd6_rs_output_timeout *= 2;
                        timeout = nd6_rs_output_timeout;
             }

but haven't hit on a working combination yet.

I think the current state is quite a lot worse than the previous one
(even though it's better on networks that *do* have v6), so I'm wondering
if it might be better to revert if it's complicated to fix here (there
was a different plan for sending RS in the future anyway wasn't there?)

 324                
 325         TAILQ_FOREACH(ifp, &ifnet, if_list) {
 326                 if (ISSET(ifp->if_flags, IFF_RUNNING) &&
 327                     ISSET(ifp->if_xflags, IFXF_AUTOCONF6)) {
 328                         t = nd6_rs_next_pltime_timo(ifp);
 329                         if (t == ND6_INFINITE_LIFETIME || t <
 330                             ND6_RS_OUTPUT_INTERVAL) {
 331                                 timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
 332                                 ia6 = in6ifa_ifpforlinklocal(ifp,
 333                                     IN6_IFF_TENTATIVE);
 334                                 if (ia6 != NULL)
 335                                         nd6_rs_output(ifp, ia6);
 336                         }              
 337                        
 338                         pltime_expire = MIN(pltime_expire, t);
 339                 }      
 340         }      
 341         if (pltime_expire != ND6_INFINITE_LIFETIME)
 342                 timeout = MAX(timeout, pltime_expire / 2);
 343                
 344         nd6_rs_output_set_timo(timeout);
 345 }  

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Florian Obser-2
On Wed, Sep 21, 2016 at 01:23:25PM +0100, Stuart Henderson wrote:

> There's a problem with this: we lose the exponential backoff for the
> quick timer. Say you have v6 at home and enable autoconf on your laptop
> then move to a network without v6 - this results in you spamming the
> network with a multicast frame every second, which does not make you
> a good network citizen especially if that's on a wireless lan.
>
[...]
> I think the current state is quite a lot worse than the previous one
> (even though it's better on networks that *do* have v6), so I'm wondering
> if it might be better to revert if it's complicated to fix here (there
> was a different plan for sending RS in the future anyway wasn't there?)

does this help?
If not, I guess we should back it out

diff --git nd6_rtr.c nd6_rtr.c
index a7529c9..3c23365 100644
--- nd6_rtr.c
+++ nd6_rtr.c
@@ -328,7 +328,8 @@ nd6_rs_output_timo(void *ignored_arg)
  t = nd6_rs_next_pltime_timo(ifp);
  if (t == ND6_INFINITE_LIFETIME || t <
     ND6_RS_OUTPUT_INTERVAL) {
- timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
+ if (t == ND6_INFINITE_LIFETIME)
+ timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
  ia6 = in6ifa_ifpforlinklocal(ifp,
     IN6_IFF_TENTATIVE);
  if (ia6 != NULL)



--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Stuart Henderson
On 2016/09/26 20:14, Florian Obser wrote:

> On Wed, Sep 21, 2016 at 01:23:25PM +0100, Stuart Henderson wrote:
>
> > There's a problem with this: we lose the exponential backoff for the
> > quick timer. Say you have v6 at home and enable autoconf on your laptop
> > then move to a network without v6 - this results in you spamming the
> > network with a multicast frame every second, which does not make you
> > a good network citizen especially if that's on a wireless lan.
> >
> [...]
> > I think the current state is quite a lot worse than the previous one
> > (even though it's better on networks that *do* have v6), so I'm wondering
> > if it might be better to revert if it's complicated to fix here (there
> > was a different plan for sending RS in the future anyway wasn't there?)
>
> does this help?
> If not, I guess we should back it out
>
> diff --git nd6_rtr.c nd6_rtr.c
> index a7529c9..3c23365 100644
> --- nd6_rtr.c
> +++ nd6_rtr.c
> @@ -328,7 +328,8 @@ nd6_rs_output_timo(void *ignored_arg)
>   t = nd6_rs_next_pltime_timo(ifp);
>   if (t == ND6_INFINITE_LIFETIME || t <
>      ND6_RS_OUTPUT_INTERVAL) {
> - timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
> + if (t == ND6_INFINITE_LIFETIME)
> + timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
>   ia6 = in6ifa_ifpforlinklocal(ifp,
>      IN6_IFF_TENTATIVE);
>   if (ia6 != NULL)

Same behaviour with this. The problem is that it's just setting
to a fixed ND6_RS_OUTPUT_QUICK_INTERVAL which isn't getting backed
off anywhere. To fix it I think we'd at least need another global
(if not a per-interface variable) that can be used as the "current
quick_timeout" and back that off or reset it as necessary, but
my attempts to do that thus far haven't been successful.

Reply | Threaded
Open this post in threaded view
|

Re: send fewer router solicitations

Florian Obser
On Mon, Sep 26, 2016 at 10:16:04PM +0100, Stuart Henderson wrote:

> On 2016/09/26 20:14, Florian Obser wrote:
> > On Wed, Sep 21, 2016 at 01:23:25PM +0100, Stuart Henderson wrote:
> >
> > > There's a problem with this: we lose the exponential backoff for the
> > > quick timer. Say you have v6 at home and enable autoconf on your laptop
> > > then move to a network without v6 - this results in you spamming the
> > > network with a multicast frame every second, which does not make you
> > > a good network citizen especially if that's on a wireless lan.
> > >
> > [...]
> > > I think the current state is quite a lot worse than the previous one
> > > (even though it's better on networks that *do* have v6), so I'm wondering
> > > if it might be better to revert if it's complicated to fix here (there
> > > was a different plan for sending RS in the future anyway wasn't there?)
> >
> > does this help?
> > If not, I guess we should back it out
> >
> > diff --git nd6_rtr.c nd6_rtr.c
> > index a7529c9..3c23365 100644
> > --- nd6_rtr.c
> > +++ nd6_rtr.c
> > @@ -328,7 +328,8 @@ nd6_rs_output_timo(void *ignored_arg)
> >   t = nd6_rs_next_pltime_timo(ifp);
> >   if (t == ND6_INFINITE_LIFETIME || t <
> >      ND6_RS_OUTPUT_INTERVAL) {
> > - timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
> > + if (t == ND6_INFINITE_LIFETIME)
> > + timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
> >   ia6 = in6ifa_ifpforlinklocal(ifp,
> >      IN6_IFF_TENTATIVE);
> >   if (ia6 != NULL)
>
> Same behaviour with this. The problem is that it's just setting
> to a fixed ND6_RS_OUTPUT_QUICK_INTERVAL which isn't getting backed
> off anywhere. To fix it I think we'd at least need another global
> (if not a per-interface variable) that can be used as the "current
> quick_timeout" and back that off or reset it as necessary, but
> my attempts to do that thus far haven't been successful.
>

then let's revert.
OK?

diff --git nd6_rtr.c nd6_rtr.c
index 93e2f4c..a9fb3f5 100644
--- nd6_rtr.c
+++ nd6_rtr.c
@@ -75,7 +75,6 @@ int rt6_deleteroute(struct rtentry *, void *, unsigned int);
 void nd6_addr_add(void *);
 
 void nd6_rs_output_timo(void *);
-u_int32_t nd6_rs_next_pltime_timo(struct ifnet *);
 void nd6_rs_output_set_timo(int);
 void nd6_rs_output(struct ifnet *, struct in6_ifaddr *);
 void nd6_rs_dev_state(void *);
@@ -284,64 +283,30 @@ nd6_rs_output_set_timo(int timeout)
  timeout_add_sec(&nd6_rs_output_timer, nd6_rs_output_timeout);
 }
 
-u_int32_t
-nd6_rs_next_pltime_timo(struct ifnet *ifp)
-{
- struct ifaddr *ifa;
- struct in6_ifaddr *ia6;
- u_int32_t pltime_expires = ND6_INFINITE_LIFETIME;
-
- TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
- if (ifa->ifa_addr->sa_family != AF_INET6)
- continue;
-
- ia6 = ifatoia6(ifa);
- if (ia6->ia6_lifetime.ia6t_pltime == ND6_INFINITE_LIFETIME ||
-    IFA6_IS_DEPRECATED(ia6) || IFA6_IS_INVALID(ia6))
- continue;
-
- pltime_expires = MIN(pltime_expires,
-    ia6->ia6_lifetime.ia6t_pltime);
- }
-
- return pltime_expires;
-}
-
 void
 nd6_rs_output_timo(void *ignored_arg)
 {
  struct ifnet *ifp;
  struct in6_ifaddr *ia6;
- u_int32_t pltime_expire = ND6_INFINITE_LIFETIME, t;
- int timeout = ND6_RS_OUTPUT_INTERVAL;
 
  if (nd6_rs_timeout_count == 0)
  return;
 
  if (nd6_rs_output_timeout < ND6_RS_OUTPUT_INTERVAL)
  /* exponential backoff if running quick timeouts */
- timeout = nd6_rs_output_timeout * 2;
+ nd6_rs_output_timeout *= 2;
+ if (nd6_rs_output_timeout > ND6_RS_OUTPUT_INTERVAL)
+ nd6_rs_output_timeout = ND6_RS_OUTPUT_INTERVAL;
 
  TAILQ_FOREACH(ifp, &ifnet, if_list) {
  if (ISSET(ifp->if_flags, IFF_RUNNING) &&
     ISSET(ifp->if_xflags, IFXF_AUTOCONF6)) {
- t = nd6_rs_next_pltime_timo(ifp);
- if (t == ND6_INFINITE_LIFETIME || t <
-    ND6_RS_OUTPUT_INTERVAL) {
- timeout = ND6_RS_OUTPUT_QUICK_INTERVAL;
- ia6 = in6ifa_ifpforlinklocal(ifp,
-    IN6_IFF_TENTATIVE);
- if (ia6 != NULL)
- nd6_rs_output(ifp, ia6);
- }
-
- pltime_expire = MIN(pltime_expire, t);
+ ia6 = in6ifa_ifpforlinklocal(ifp, IN6_IFF_TENTATIVE);
+ if (ia6 != NULL)
+ nd6_rs_output(ifp, ia6);
  }
  }
- if (pltime_expire != ND6_INFINITE_LIFETIME)
- timeout = MAX(timeout, pltime_expire / 2);
-
- nd6_rs_output_set_timo(timeout);
+ nd6_rs_output_set_timo(nd6_rs_output_timeout);
 }
 
 void


--
I'm not entirely sure you are real.