Page MenuHomeFreeBSD

in[6]_pcblookup_hash_locked() and callers locking after epoch(9)
Needs ReviewPublic

Authored by bz on Oct 17 2018, 12:04 AM.

Details

Summary

When in[6]_pcblookup_hash_locked() is called from in[6]_pcblookup_hash() only
the epoch(9) seems to protect it; that means we might be able to get an
inp out of the list which is marked FREED and we need to skip it.

The current solution only checks for FREED after a decision was made, which
in turn might yield an unexpected result, though that would only happen in
rare cases.

While here solve all the "XXX locking" cases in the in[6]_pcblookup_hash_locked()
functions.

I am still a bit uneasy about now taking an RLOCK on the inp where we have not
done so before despite the locking order is relaxed with epoch(9). In theory
the lock order should not change with this patch, given the previous solution
did also take the inp lock in in[6]_pcblookup_hash() (though only for the
assumed result).

PR: 232193

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 20257
Build 19734: arc lint + arc unit

Event Timeline

bz created this revision.Oct 17 2018, 12:04 AM
mmacy added a comment.Oct 17 2018, 5:54 AM

Is this in response to an observed bug? This is adding a lot of complexity when we can simply validate the result

mmacy added a subscriber: kbowling.Oct 17 2018, 5:57 AM
bz added a comment.Oct 17 2018, 6:01 AM

Is this in response to an observed bug? This is adding a lot of complexity when we can simply validate the result

The complexity mostly comes from the fact that we can (and do) now properly lock in the inp when going over the lists (which we couldn't before due to lock order) and with that are less prone to races. Hence this is not just about validating the inp is not FREED but one patch without the other half would make it worse. I can break them up into two if that makes it easier to review/progress.

bz added a comment.Oct 25 2018, 2:06 PM

Neither this nor the kernel version (though that one just less likely) is not deadlock safe. It can still happen.

mmacy added a comment.Oct 25 2018, 7:20 PM
In D17593#375500, @bz wrote:

Is this in response to an observed bug? This is adding a lot of complexity when we can simply validate the result

The complexity mostly comes from the fact that we can (and do) now properly lock in the inp when going over the lists (which we couldn't before due to lock order) and with that are less prone to races. Hence this is not just about validating the inp is not FREED but one patch without the other half would make it worse. I can break them up into two if that makes it easier to review/progress.

Yes, please.