fix some IPv6 DAD races

[markj]

Summary

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.

Test Plan

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.

[ae]

I think it should be possible to not acquire extra reference in the nd6_dad_add. nd6_dad_add/nd6_dad_del should only work with the V_dadq. Another consumers will just acquire and release references. And last release will free dp.

[markj]

In D1258#4, @ae wrote:

I think it should be possible to not acquire extra reference in the nd6_dad_add. nd6_dad_add/nd6_dad_del should only work with the V_dadq. Another consumers will just acquire and release references. And last release will free dp.

Hm, not without making changes elsewhere. If nd6_dad_stop() stops the DAD before its callout has run, the nd6_dad_rele() call in nd6_dad_stop() will cause dp to be freed. But nothing has removed it from the queue yet, so the subsequent nd6_dad_find() call will return a pointer to freed memory. We need the V_dadq reference in this case.

[ae]

In D1258#5, @markj wrote:

Hm, not without making changes elsewhere. If nd6_dad_stop() stops the DAD before its callout has run, the nd6_dad_rele() call in nd6_dad_stop() will cause dp to be freed. But nothing has removed it from the queue yet, so the subsequent nd6_dad_find() call will return a pointer to freed memory. We need the V_dadq reference in this case.

That is what I mean:

1. In nd6_dad_add() replace refcount_acquire() with refcount_init(). Remove refcount_init() from nd6_dad_start().
2. Remove nd6_dad_rele() from nd6_dad_stoptimer() and from nd6_dad_timer().

[markj]

In D1258#6, @ae wrote:

That is what I mean:

1. In nd6_dad_add() replace refcount_acquire() with refcount_init(). Remove refcount_init() from nd6_dad_start().
2. Remove nd6_dad_rele() from nd6_dad_stoptimer() and from nd6_dad_timer().

Ah, I see. That seems to be correct. I'll do some more testing and upload in a bit.

[markj]

Don't give the callout a reference - it isn't necessary anymore.

[hrs]

Diff 2623 looks reasonable to me. Thank you for working on it.

[ae]

Ah, I see, you changed refcount_init() to zero :)

sys/netinet6/nd6_nbr.c

1189

This reference seems extra to me. You already have one after refcount_init.
When you do this:

refcount_init(&dp->refcnt, 1);
...
nd6_dad_rele(dp);

dp should be freed. But this:

refcount_init(&dp->refcnt, 1); // refcnt == 1
nd6_dad_add(dp); // refcnt == 2
...
nd6_dad_del(dp); // refcnt == 1

needs one extra refcount_release(&dp->refcnt);
Am I wrong?

[markj]

In D1258#13, @ae wrote:

Ah, I see, you changed refcount_init() to zero :)

Yeah, it seemed a bit odd to me to put the refcount_init() call in nd6_dad_add(). If you prefer that, I can change it.

[ae]

It looks like refcount(9) usage pattern in the tree uses refcount_init() with 1. May be just remove acquire from nd6_dad_add()?

[markj]

• Use M_ZERO instead of explicitly zeroing the DAD object.
• Grab the reference before calling nd6_dad_add().

[markj]

Closed by commit rS275593 (authored by @markj).