There are a number of races in the DAD code which can cause double frees or use-after-frees of the DAD objects. These objects are kept in a global queue which is protected by a rwlock; however, they aren't reference-counted.

This change attempts to fix some of the races by adding a refcount to struct dadq and simplifying the way the DAD objects are managed.

Currently, the DAD queue can be manipulated in a number of contexts: the DAD callout, nd6_dad_start() and nd6_dad_stop() (called from ioctl handlers), and NS and NA packet handlers. This makes it pretty difficult to fix all of the races; see my comments here: https://lists.freebsd.org/pipermail/freebsd-current/2014-November/053437.html

The proposed change makes the following modifications:

1. Add a refcount field to struct dadq. This field is incremented/decremented whenever a dadq is inserted into/removed from the DAD queue. In nd6_dad_start(), the refcount is initialized to 1 for the callout's reference, and nd6_dad_timer() releases the reference if it decides not to reschedule itself. Otherwise nd6_dad_stoptimer() releases it. nd6_dad_find() now returns a referenced struct dadq.

2. Fix handling of the dad_ifa reference in each struct dadq. This reference needs to be declared when the struct dadq is initialized and released only when the dadq is about to be freed. The current handling is just wrong.

3. Stop calling nd6_dad_duplicated() in packet input paths. Instead, just increment counters in the corresponding struct dadq so that the DAD callout for that address can mark the address as a duplicate when it runs. This means that there will be a small delay before the address is actually marked as duplicate, but during that time it'll be marked as tentative, which means that it shouldn't be used anyway.

4. nd6_dad_duplicated() no longer needs to stop the callout or free the struct dadq, since these things are now both done by the DAD callout itself, which is now the only caller of nd6_dad_duplicated().

5. Use nd6_dad_init() to initialize the DAD queue and its rwlock. This is a bit superfluous, but it's consistent with the way the rest of the NDP queues and locks are initialized.

There are still some races in this code. In particular, we may try to remove a struct dadq from the queue twice if multiple threads attempt to remove the same struct dadq from the queue simultaneously. But I can't seem to test this without triggering a crash elsewhere in the IPv6 code, so before investigating that I was hoping to have some review of what I've done so far so that I can work on this incrementally.

I wrote some loops to manually add duplicate addresses on a pair of bridged VMs. This would previously cause a number of different crashes, but now works properly. I used vmstat -m while testing to make sure we aren't leaking memory from the ip6ndp bucket.

If multiple threads attempt to add/remove address on the same interface simultaneously, we get a crash elsewhere. I'll try to address that next.

rstone@ reported on freebsd-current that an earlier version of this patch fixed a reproducible DAD-related crash that he was seeing.