Fix a race between DAD start and stop on an address.

[markj]

Summary

nd6_dad_start() contains an (admittedly narrow) race:

1. T1 calls nd6_dad_start() on an address and adds the corresponding dadq to the DAD list. dp now has a refcount of 1, and we haven't yet set up the callout.
2. T2 removes the DAD list entry as part of address teardown. This causes the dadq to be removed from the list and its refcount decremented, so it is freed.
3. T1 calls nd6_dad_starttimer() on a freed dadq.

This change fixes the race by holding the DAD write lock across both the
list insertion and the nd6_dad_starttimer() call.

[ae]

sys/netinet6/nd6_nbr.c

1356

Now you hold the lock and invoke nd6_dad_ns_output(). Was it intended?

[markj]

sys/netinet6/nd6_nbr.c

1356

Not specifically, but I didn't think it would be problematic. Am I missing something? An alternative approach would be to drop the lock, transmit the NS, and then re-lookup the dadq before calling nd6_dad_starttimer().

[ae]

It looks like there is no problem. I just remember a negative experience in the ND6 code. We use internally the patch similar to this one: https://reviews.freebsd.org/D903

[markj]

In D2256#6, @ae wrote:

It looks like there is no problem. I just remember a negative experience in the ND6 code. We use internally the patch similar to this one: https://reviews.freebsd.org/D903

I see. I'd prefer to avoid holding the lock over calls to nd6_ns_output(), but we only ever need to take the DAD lock when adding or removing an ifaddr, or in the NS input path. Note that D2255 also does the same thing, since with that change nd6_dad_timer() executes with the DAD lock held instead of Giant. I've also been testing with INVARIANTS and haven't observed any new LOR warnings.