Page MenuHomeFreeBSD

Add new refcount API
AbandonedPublic

Authored by mjg on Feb 3 2020, 3:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 27 2024, 7:59 PM
Unknown Object (File)
Dec 20 2023, 6:46 AM
Unknown Object (File)
Dec 18 2023, 3:13 AM
Unknown Object (File)
Nov 11 2023, 9:24 AM
Unknown Object (File)
Sep 11 2023, 3:36 AM
Unknown Object (File)
Jul 26 2023, 2:28 AM
Unknown Object (File)
Jun 17 2023, 11:08 PM
Unknown Object (File)
Jun 16 2023, 5:26 PM
Subscribers

Details

Reviewers
jeff
kib
markj
jhb
Summary

Following up on D23469.

refcount(9) as implemented right now has the following properties:

  • always operates on an int type, allowing consumers to misuse the field by accident. this is fixable by requiring a struct to be passed instead.
  • supports waking up sleepers, implemented by adding a dedicated flag bit
  • performs range checks at runtime, making sure total count wont flip the bit (and previous would was just checking for overflow)

VFS was using it for the 2 vnode reference counts before the range check or flag bits were introduced, while sometimes mixing in custom fiddling without said routines. With introduction of flags said mixed behavior turned into a bug which can result in use-after-free (that is, not-gating update can push the count to a state where the top bit is set with it masked the count is just 1; then refcount_release will conclude the object is to be freed and there are waiters to wake up).

Pushing VFS completely out of recount(9) as implemented right now has the following benefits:

  • not braching on stuff we don't need for this usecase
  • custom assertion support -- that is, should anything go wrong we can always end up dumping the vnode with vn_printf instead of just stating that the count has invalid value

To that end, I implemented an API which is a simple wrapper around atomics.

Properties:

  • int and long sizes
  • some degree of type safety by providing refcntint_t and refcntlong_t (i.e., no accidental misuse)
  • custom callback for assertion failures
  • the FILE, LINE, func triple is gathered *before* going into an inline routine. this means when the assertion fires we get the actual user and not the inline routine itself

sys/_refcount.h header was added to avoid dependency issues with zfs. adding refcount.h to vnode.h causes significant compilation problems.

This requires more testing, but is roughly what I have in mind.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mjg edited the summary of this revision. (Show Details)
sys/sys/refcount.h
239

Don't you want to assert that (old + n) > 0 instead, modulo UB ? It is too late to assert old < 0.

263

Do you want to assert that there is no overflow ?

281

Again, what about overflow ...

351

Please see r320501.

  • sync fences use with refcount as of D23709
  • pass the failed assertion expression to panicking routine
  • assert more things
mjg marked 2 inline comments as done.Feb 16 2020, 12:40 AM
mjg added inline comments.
sys/sys/refcount.h
281

This one is for non-debug case and does not check on purpose.

mjg marked an inline comment as done.Feb 16 2020, 12:42 AM

So what about my comments in D23469? Why do we have to have 3 refcount APIs?