Page MenuHomeFreeBSD

in6_ifattach_linklocal: handle immediate removal of the new LLA
ClosedPublic

Authored by vangyzen on Wed, Nov 7, 11:07 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vangyzen created this revision.Wed, Nov 7, 11:07 PM
vangyzen edited the summary of this revision. (Show Details)Wed, Nov 7, 11:09 PM
vangyzen added a subscriber: melifaro.
dab accepted this revision.Thu, Nov 8, 2:35 AM
dab added a reviewer: dab.

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

bz requested changes to this revision.Thu, Nov 8, 12:55 PM

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?

This revision now requires changes to proceed.Thu, Nov 8, 12:55 PM

@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 added inline comments.Thu, Nov 8, 7:06 PM
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 marked 2 inline comments as done.Thu, Nov 8, 7:37 PM
vangyzen added inline comments.
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 updated this revision to Diff 50180.Thu, Nov 8, 7:37 PM
vangyzen marked an inline comment as done.
  • improve comment; add nd6log
This revision was not accepted when it landed; it landed in state Needs Review.Thu, Nov 8, 7:50 PM
This revision was automatically updated to reflect the committed changes.
ae added a comment.Sat, Nov 10, 9:29 AM
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.