in6_ifattach_linklocal: handle immediate removal of the new LLA

[vangyzen]

Summary

If another thread immediately removes the link-local address
added by in6_update_ifa(), in6ifa_ifpforlinklocal() can return NULL,
so the following assertion (or dereference) is wrong.
Remove the assertion, and handle NULL somewhat better than panicking.
This matches all of the other callers of in6_update_ifa().

PR: 219250

[dab]

@vangyzen, Looks OK to me but I'm curious how you ran across this situation.

[bz]

To me this change seems wrong. The only caller for this function is exactly for the situation when the link-local address is missing.
If we are in the progress of "configuring" the interface and someone is already de-configuring it to me that sounds like a concurrency problem elsewhere.
This entire function seems to be based on the idea that there's a lock held around it and it's the only actor (which might very well still be coming from &Giant days of FreeBSD 4).

Rather than changing the behaviour, why don't we fix the problem and hold the appropriate lock (if possible) or make sure (otherwise) that the concurrency problem is solved?

[vangyzen]

@dab This is Isilon internal bug 240643.

@bz It's not wrong. It solves a small problem. I agree that it does not solve the larger problem, but it prevents the panic, so it's still an improvement.

In one sense, we can't solve the larger concurrency problem; we can only move it. If we IF_ADDR_WLOCK(ifp) around the link-local block in in6_ifattach(), the other thread will simply wait until we drop the lock and then remove the address. The result is the same.

I agree that some IPv6 code still seems to expect Giant to be held. I would like to improve that, but this panic shouldn't have to wait for that work.

[bz]

sys/netinet6/in6_ifattach.c

486 ↗

(On Diff #50141)

Can you turn this comment into something more meaningful given this is going to stay?

/* We lost a race with another thread already deleting the address. */

Or something like that? I keep wondering if another nd6log() would also help (if you do please use %s func).

[vangyzen]

sys/netinet6/in6_ifattach.c

486 ↗

(On Diff #50141)

Sure, I'll do both. The log might help the admin track down the race.

[vangyzen]

• improve comment; add nd6log

[ae]

In D17898#382455, @bz wrote:

To me this change seems wrong. The only caller for this function is exactly for the situation when the link-local address is missing.
If we are in the progress of "configuring" the interface and someone is already de-configuring it to me that sounds like a concurrency problem elsewhere.
This entire function seems to be based on the idea that there's a lock held around it and it's the only actor (which might very well still be coming from &Giant days of FreeBSD 4).
Rather than changing the behaviour, why don't we fix the problem and hold the appropriate lock (if possible) or make sure (otherwise) that the concurrency problem is solved?

It looks like inet6 code needs an sx lock to protect from concurrent address configuration, like we have in inet code.