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.
> > > + 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.
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.
> > And where is the IN6_IFF_DEPRECATED flag cleared now?
>
> In in6_update_ifa() which is called from the ioctl, the only path where
> we are setting new pltimes / vltimes.
Fine, so the removed code was not necessary anyway.
bluhm
