On Mon, Aug 07, 2017 at 06:29:25PM +0200, Alexander Bluhm wrote:
> On Mon, Aug 07, 2017 at 02:30:19PM +0000, Florian Obser wrote:
> > On Mon, Aug 07, 2017 at 03:51:00PM +0200, Alexander Bluhm wrote:
> > > On Mon, Aug 07, 2017 at 08:07:33AM +0000, Florian Obser wrote:
> > > > index cafdd9fe36f..7796af6191c 100644
> > > > --- sys/netinet6/in6.c
> > > > +++ sys/netinet6/in6.c
> > > > @@ -686,6 +686,10 @@ in6_update_ifa(struct ifnet *ifp, struct
> > > > in6_aliasreq *ifra,
> > > > */
> > > > ia6->ia6_flags = ifra->ifra_flags;
> > > >
> > > > + KERNEL_LOCK();
> > > > + nd6_expire_timer_update(ia6);
> > > > + KERNEL_UNLOCK();
> > > > +
> > >
> > > Can we ever reach this code without holding the kernel lock?
> >
> > sppp_update_ip6_addr() seems to hold NET_LOCK but not the kernel lock
>
> sppp_update_ip6_addr() is run from the systq.
> task_set(&sp->ipv6cp.set_addr_task, sppp_update_ip6_addr, sp);
> task_add(systq, &sp->ipv6cp.set_addr_task);
>
> The kernel provides two system taskqs: systq, which executes while
> holding the kernel lock, and systqmp, which does not hold the kernel lock
> during execution.
>
> So we don't need KERNEL_LOCK() here.
>
Thanks, removed. One of these days someone probably should document
the various locks and how they fit together. I'm just making this up
as I go along...
> > > > + if (!timeout_pending(&nd6_expire_timeout) || nd6_expire_time >
> > > > + expire_time) {
> > > > + expire_time++; /* fire one second after expiry */
> > >
> > > Why do we need this and why do we do it after the
> > > "nd6_expire_time > expire_time" check?
> >
> > expire_time contains the uptime seconds when pltime or vltime reach 0.
> > IFA6_IS_INVALID and IFA6_IS_DEPRECATED check for '>', not for '>='.
> > That means without the ++ the timer would fire exactly at the second
> > when pltime or vltime reach zero. But the address is not yet invalid
> > or deprecated. We would then schedule a timeout in 0 seconds. Which is
> > according to the man page actually 1 second and then the address is
> > invalid or deprecated.
>
> timeout_add() ... If the
> value is `0' it will, in the current implementation, be treated as `1',
>
> The man page is about timeout_add(), so 0 is interpreted as 1 tick,
> not 1 second. But still, if the nd6 timer means it is invalid
> the second after it is reached, the ++ is correct.
Ah yes.
>
> But should it be before the "nd6_expire_time > expire_time" check?
> If nd6_expire_time is expire_time + 1 we enter the block, increase
> expire_time and set nd6_expire_time to the same value. This means
> needless timeout_add_sec() calls.
I moved it up and added a comment why we are doing the ++. I will
probably forget why it's there in a few months.
I don't think it matters in practice where we increment. Addresses
usually don't expire one second after the other. But this is easier to
understand.
diff --git lib/libc/gen/sysctl.3 lib/libc/gen/sysctl.3
index bb0317d7187..0f6ea4233fe 100644
--- lib/libc/gen/sysctl.3
+++ lib/libc/gen/sysctl.3
@@ -1639,7 +1639,6 @@ The currently defined protocols and names are:
.It icmp6 Ta nd6_delay Ta integer Ta yes
.It icmp6 Ta nd6_maxnudhint Ta integer Ta yes
.It icmp6 Ta nd6_mmaxtries Ta integer Ta yes
-.It icmp6 Ta nd6_prune Ta integer Ta yes
.It icmp6 Ta nd6_umaxtries Ta integer Ta yes
.It icmp6 Ta redirtimeout Ta integer Ta yes
.It ip6 Ta auto_flowlabel Ta integer Ta yes
@@ -1718,10 +1717,6 @@ This variable specifies the
constant in IPv6 neighbor discovery specification
.Pq RFC 4861 .
.Pp
-.It Li icmp6.nd6_prune Pq Va net.inet6.icmp6.nd6_prune
-This variable specifies the interval between IPv6 neighbor cache babysitting
-in seconds.
-.Pp
.It Li icmp6.nd6_umaxtries Pq Va net.inet6.icmp6.nd6_umaxtries
This variable specifies the
.Dv MAX_UNICAST_SOLICIT
diff --git sys/netinet/icmp6.h sys/netinet/icmp6.h
index 1713f6c2125..628ffe36dd4 100644
--- sys/netinet/icmp6.h
+++ sys/netinet/icmp6.h
@@ -502,7 +502,6 @@ struct icmp6stat {
#define ICMPV6CTL_STATS 1
#define ICMPV6CTL_REDIRACCEPT 2 /* accept/process redirects */
#define ICMPV6CTL_REDIRTIMEOUT 3 /* redirect cache time */
-#define ICMPV6CTL_ND6_PRUNE 6
#define ICMPV6CTL_ND6_DELAY 8
#define ICMPV6CTL_ND6_UMAXTRIES 9
#define ICMPV6CTL_ND6_MMAXTRIES 10
@@ -521,7 +520,7 @@ struct icmp6stat {
{ "redirtimeout", CTLTYPE_INT }, \
{ 0, 0 }, \
{ 0, 0 }, \
- { "nd6_prune", CTLTYPE_INT }, \
+ { 0, 0 }, \
{ 0, 0 }, \
{ "nd6_delay", CTLTYPE_INT }, \
{ "nd6_umaxtries", CTLTYPE_INT }, \
@@ -543,7 +542,7 @@ struct icmp6stat {
&icmp6_redirtimeout, \
NULL, \
NULL, \
- &nd6_prune, \
+ NULL, \
NULL, \
&nd6_delay, \
&nd6_umaxtries, \
diff --git sys/netinet6/in6.c sys/netinet6/in6.c
index cafdd9fe36f..a1ad798cdf3 100644
--- sys/netinet6/in6.c
+++ sys/netinet6/in6.c
@@ -686,6 +686,8 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra,
*/
ia6->ia6_flags = ifra->ifra_flags;
+ nd6_expire_timer_update(ia6);
+
/*
* We are done if we have simply modified an existing address.
*/
diff --git sys/netinet6/nd6.c sys/netinet6/nd6.c
index 245b64c6155..c04ef7fdcf6 100644
--- sys/netinet6/nd6.c
+++ sys/netinet6/nd6.c
@@ -45,6 +45,7 @@
#include <sys/ioctl.h>
#include <sys/syslog.h>
#include <sys/queue.h>
+#include <sys/stdint.h>
#include <sys/task.h>
#include <net/if.h>
@@ -66,7 +67,7 @@
#define ND6_RECALC_REACHTM_INTERVAL (60 * 120) /* 2 hours */
/* timer values */
-int nd6_prune = 1; /* walk list every 1 seconds */
+time_t nd6_expire_time = -1; /* at which time_uptime nd6_expire runs */
int nd6_delay = 5; /* delay first probe time 5 second */
int nd6_umaxtries = 3; /* maximum unicast query */
int nd6_mmaxtries = 3; /* maximum multicast query */
@@ -122,8 +123,6 @@ nd6_init(void)
timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
timeout_set(&nd6_expire_timeout, nd6_expire_timer, NULL);
- timeout_add_sec(&nd6_expire_timeout, nd6_prune);
-
}
struct nd_ifinfo *
@@ -417,6 +416,44 @@ nd6_llinfo_timer(void *arg)
NET_UNLOCK(s);
}
+void
+nd6_expire_timer_update(struct in6_ifaddr *ia6)
+{
+ time_t expire_time = INT64_MAX;
+ int secs;
+
+ KERNEL_ASSERT_LOCKED();
+
+ if (ia6->ia6_lifetime.ia6t_vltime != ND6_INFINITE_LIFETIME)
+ expire_time = ia6->ia6_lifetime.ia6t_expire;
+
+ if (!(ia6->ia6_flags & IN6_IFF_DEPRECATED) &&
+ ia6->ia6_lifetime.ia6t_pltime != ND6_INFINITE_LIFETIME &&
+ expire_time > ia6->ia6_lifetime.ia6t_preferred)
+ expire_time = ia6->ia6_lifetime.ia6t_preferred;
+
+ if (expire_time == INT64_MAX)
+ return;
+
+ /*
+ * IFA6_IS_INVALID() and IFA6_IS_DEPRECATED() check for uptime
+ * greater than ia6t_expire or ia6t_preferred, not greater or equal.
+ * Schedule timeout one second later so that either IFA6_IS_INVALID()
+ * or IFA6_IS_DEPRECATED() is true.
+ */
+ expire_time++;
+
+ if (!timeout_pending(&nd6_expire_timeout) || nd6_expire_time >
+ expire_time) {
+ secs = expire_time - time_uptime;
+ if ( secs < 0)
+ secs = 0;
+
+ timeout_add_sec(&nd6_expire_timeout, secs);
+ nd6_expire_time = expire_time;
+ }
+}
+
/*
* Expire interface addresses.
*/
@@ -429,8 +466,6 @@ nd6_expire(void *unused)
KERNEL_LOCK();
NET_LOCK(s);
- timeout_add_sec(&nd6_expire_timeout, nd6_prune);
-
TAILQ_FOREACH(ifp, &ifnet, if_list) {
struct ifaddr *ifa, *nifa;
struct in6_ifaddr *ia6;
@@ -442,14 +477,10 @@ nd6_expire(void *unused)
/* check address lifetime */
if (IFA6_IS_INVALID(ia6)) {
in6_purgeaddr(&ia6->ia_ifa);
- } else if (IFA6_IS_DEPRECATED(ia6)) {
- ia6->ia6_flags |= IN6_IFF_DEPRECATED;
} else {
- /*
- * A new RA might have made a deprecated address
- * preferred.
- */
- ia6->ia6_flags &= ~IN6_IFF_DEPRECATED;
+ if (IFA6_IS_DEPRECATED(ia6))
+ ia6->ia6_flags |= IN6_IFF_DEPRECATED;
+ nd6_expire_timer_update(ia6);
}
}
}
diff --git sys/netinet6/nd6.h sys/netinet6/nd6.h
index 3b08e2c5dfe..5db1684895e 100644
--- sys/netinet6/nd6.h
+++ sys/netinet6/nd6.h
@@ -138,7 +138,6 @@ struct llinfo_nd6 {
(((MIN_RANDOM_FACTOR * (x >> 10)) + (arc4random() & \
((MAX_RANDOM_FACTOR - MIN_RANDOM_FACTOR) * (x >> 10)))) /1000)
-extern int nd6_prune;
extern int nd6_delay;
extern int nd6_umaxtries;
extern int nd6_mmaxtries;
@@ -208,6 +207,7 @@ void nd6_rs_input(struct mbuf *, int, int);
int in6_ifdel(struct ifnet *, struct in6_addr *);
void rt6_flush(struct in6_addr *, struct ifnet *);
+void nd6_expire_timer_update(struct in6_ifaddr *);
#endif /* _KERNEL */
#endif /* _NETINET6_ND6_H_ */
--
I'm not entirely sure you are real.
