netinet6/nd6: Sprinkle missing prefix refcounting

[markj]

Summary

When we drop the prefix lock to call nd6_prefix_offlink() or
nd6_prefix_onlink(), make sure to keep the correpsonding prefix
structure alive. It is possible for a concurrent nd6_timer() to expire
the prefix while the lock is dropped.

Reported by: Maik Muench of Secfault Security

[pouria]

sys/netinet6/nd6_rtr.c

1962–1963

shouldn't be after rlock?

2237–2238

same here

[pouria]

sys/netinet6/nd6_rtr.c

1962–1963

shouldn't be after rlock?

We don't call it after RLOCK. So it should be fine there.

2237–2238

same here

Maybe even after a_failure. Because we unlock it again and try to dereference it after.

[markj]

sys/netinet6/nd6_rtr.c

1962–1963

There is no reason to acquire it after the read lock is reacquired.

2237–2238

I'm not sure what you are suggesting with a_failure.

Note, if the prefix was unlinked, then V_nd6_list_genid will be incremented. Conversely, if V_nd6_list_genid wasn't incremented, then the prefix must still be linked into the prefix list, so it's safe to use it to continue iterating.

[pouria]

sys/netinet6/nd6_rtr.c

2254

I'm talking about here.
If I understand it correctly:
Consider we unlock above again and we trying to dereference pr here.
So if timer runs right after above unlock and expire our pr, same thing could happen, right?
Or am I missing something?

[markj]

sys/netinet6/nd6_rtr.c

2254

The caller of nd6_prefix_offlink() is expected to hold a ref on pr. With this patch, I think that's handled correctly.

[zlei]

Looks good to me. @markj Do you plan to MFC ?

[markj]

In D58423#1340496, @zlei wrote:

Looks good to me. @markj Do you plan to MFC ?

Certainly, yes.