rust: update Ref to use the kernel's refcount_t.

[wedsonaf]

Saturate the refcount instead of panicking. It also leverages existing C
code instead of reimplementing functionality.

This is the first step to deprecate Arc (that aborts on overflow) and BTreeMap (that panics on allocation failure).

Signed-off-by: Wedson Almeida Filho wedsonaf@google.com

[nbdd0121]

Aside from overflow behaviour, when should Ref be used but not Arc?

[wedsonaf]

Aside from overflow behaviour, when should Ref be used but not Arc?

I'm removing all usages of Arc from binder.

When weak references are needed, one would need to use Arc. But given that we currently don't have anything that needs this, I would ideally like to ban Arc. Eventually if the need for weak references arises, I think a saturating version would be preferable.

[nbdd0121]

I am actually more curious about the difference of handling ref count -- Ref requires the inner type to provide a RefCount while Arc supplies its own. Is the intention to create some similar to enable_shared_from_this so that function receivers can be "&Self"? It certainly fells "more Rust" if we instead make receiver self: &Ref<Self>.

[wedsonaf]

I am actually more curious about the difference of handling ref count -- Ref requires the inner type to provide a RefCount while Arc supplies its own. Is the intention to create some similar to enable_shared_from_this so that function receivers can be "&Self"? It certainly fells "more Rust" if we instead make receiver self: &Ref<Self>.

Yes, Ref was written so that we could "upgrade" from &T to Ref<T>.

self: &Ref<Self> isn't really possible AFAICT because the compiler only accepts a small set of types for self (which includes Arc). As a minor point, &self has the bonus advantage of avoiding the double indirection implied by &Ref<Self> and &Arc<Self>.

[nbdd0121]

It we are going to roll our own alloc crate then we can just make &Ref<T> a Receiver. If not, we could enable arbitrary_self_types. Manually implementing RefCounted needs unsafe so it's not very ideal.

Speaking of RefCounted, I think it should provide inc_ref and dec_ref methods instead just returning &RefCount; this way a C-managed wrapper can implement RefCounted as well, and for those defined in Rust it could just delegate to its RefCount instance.

[TheSven73]

Is the support for an initialization closure only there to support easy initialization of Pinned structures? Or do you believe it's more generally useful? Just asking out of general interest.

[wedsonaf]

See wedsonaf@61397b0 -- this allows us to obviate get_mut, which allows us to safely claim that Ref is in fact pinned.

[TheSven73]

Ah I see ! Yes, getting rid of get_mut() looks very good. Not sure why you'd need a mut closure for initialization, though? Arc also provides shared references only, but we are still able to initialize the Mutexes inside of it?

[wedsonaf]

See wedsonaf@e201bf1 -- allowing init on just &self (as opposed to &mut self) was an oversight that leads to unsafety (e.g., someone calling init on a synchronisation primitive that is in use).

[TheSven73]

Holy smoke! I was already wondering about that, but I'm coming from profound ignorance on Pinning and everything around it.

[nbdd0121]

The best way to avoid get_mut is through a wrapper type like UniqueArc https://doc.servo.org/servo_arc/struct.UniqueArc.html

[TheSven73]

allowing init on just &self (as opposed to &mut self) was an oversight that leads to unsafety

PS do we have an Issue to keep track of this?

[wedsonaf]

The best way to avoid get_mut is through a wrapper type like UniqueArc https://doc.servo.org/servo_arc/struct.UniqueArc.html

Nice, I like it.

Something for another PR though. (The reason I'm removing get_mut in this PR is that we don't have a refcount_t function with the right barriers for get_mut and I want this code to be simple wrappers around refcount_t).

allowing init on just &self (as opposed to &mut self) was an oversight that leads to unsafety

PS do we have an Issue to keep track of this?

I don't think so. Feel free to open one.

[TheSven73]

Should n be an int here?

linux/include/linux/refcount.h

Lines 129 to 137 in d8c6b9c

	/**
	* refcount_set - set a refcount's value
	* @r: the refcount
	* @n: value to which the refcount will be set
	*/
	static inline void refcount_set(refcount_t *r, int n)
	{
	atomic_set(&r->refs, n);
	}

[TheSven73]

I know that, in kernel C, lots of code is initializing refcount_t by simply zero-ing the memory, followed by a refcount_set(n). Yes, it'll work.

But, is REFCOUNT_INIT(n) more correct? Initialization is not identical to setting an initial value, even if it happens to work for now?

[wedsonaf]

Yes, I wanted to use REFCOUNT_INIT but the fact that it's an rvalue turned me off.

But since we need a helper routine anyway for refcount_set, we may as well replace with a refcount_init that just returns a value initialised with REFCOUNT_INIT. Then we call it in the constructor of RefCount.

Do you have anything better in mind?

[TheSven73]

Sounds good to me. In that case you won't even have to zero-init the structure using default(). And #[derive(Default)] on RefCount can go too.

[wedsonaf]

Sounds good to me. In that case you won't even have to zero-init the structure using default(). And #[derive(Default)] on RefCount can go too.

That's what I was planning, but we can't really trust the value in RefCount that is passed into try_new_and_init -- for example, if the caller sets the value to 0, we would be breaking the invariant. We need to initialise it again in try_new_and_init.

[nbdd0121]

Maybe we cab add the zero-refcount requirement to the safety requirement of RefCounted?

[TheSven73]

Yes, I see the problem. I'm not sure why Ref<T> requires its inner type to hold a RefCount, instead of working like Arc<T>? I'm sure there's a reason, I just don't know what it is.

[wedsonaf]

Maybe we cab add the zero-refcount requirement to the safety requirement of RefCounted?

I suppose we could. But given the fact that we can enforce this, I think it's better to just reinit the refcount in try_new_and_init and contain the safety requirements.

[wedsonaf]

Yes, I see the problem. I'm not sure why Ref<T> requires its inner type to hold a RefCount, instead of working like Arc<T>? I'm sure there's a reason, I just don't know what it is.

The reason for embedding the refcount is so that we can go from &T to Ref<T>, that is, we can increment the refcount on an object while having just a regular reference to it. We use it in Binder to increment the refcount on a Process object that implements FileOperations: in those functions we only get a &Process, using Arc we wouldn't have a safe path to go from &Process to Arc<Process>, which is need to do in some operations.

(We can actually build an Arc<T>-like abstractions from this. Perhaps we should use that instead to deprecate Arc.)

In my current design for red-black trees, we also only get &T when looking up elements stored as Box<T>, Arc<T> or Ref<T>, which is problematic for Arc when we need to take an extra reference.

[nbdd0121]

I suppose we could. But given the fact that we can enforce this, I think it's better to just reinit the refcount in try_new_and_init and contain the safety requirements.

Well, if we later change RefCounted to support C-manaaged pointers with inc_ref/dec_ref then this'll not be enforcable?

[wedsonaf]

It we are going to roll our own alloc crate then we can just make &Ref<T> a Receiver. If not, we could enable arbitrary_self_types. Manually implementing RefCounted needs unsafe so it's not very ideal.

I didn't know about the Receiver trait. That changes things. I'm now inclined to agree that an Arc-like object (backed by refcount_t) is preferable because it leaves things looking very much like idiomatic Rust. Then we can use something like ArcBorrow for the PointerWrapper case when we may need to increment the refcount but we don't have an Arc anywhere to reference.

Speaking of RefCounted, I think it should provide inc_ref and dec_ref methods instead just returning &RefCount; this way a C-managed wrapper can implement RefCounted as well, and for those defined in Rust it could just delegate to its RefCount instance.

Yes, I think you and I briefly discussed this elsewhere. I think this PR makes it easier now that we don't really need to look into the value anymore.

[wedsonaf]

Updated the Ref implementation to be like Arc. (Had to land some changes before that, but they're in now.)

PTAL.

[ojeda]

Two nits I noticed

[TheSven73]

I'm doing a somewhat in-depth review. Hoping to finish it today, if I can.

[TheSven73]

Very nice! This is a real improvement IMHO.

[TheSven73]

Is there a benefit to using NonNull<T> here instead of just *mut T? The documentation says:

If you’re not sure if you should use NonNull, just use *mut T!

So, what is the benefit that tips the scale in favour of NonNull<T> ? The "must never be null" could simply be formulated as a type invariant, no?

In addition, have we thought about covariance? I find the following very hard to understand, and I wonder if we have considered its full implications?

If your type cannot safely be covariant, you must ensure it contains some additional field to provide invariance. Often this field will be a PhantomData type like PhantomData<Cell> or PhantomData<&'a mut T>.

[nbdd0121]

IMO NonNull is correct here. Ref is conceptually &T so it should be covariant. *mut T is invariant and is thus not correct. NonNull also ensures that Option<Ref<T>> can be stored in a single usize.

[TheSven73]

Thanks for the advice Gary! I posted the covariance doc quote, because I don't really understand what it means. Maybe you could provide an example that would not work (or be unsound) if we used *mut T instead of NonNull<T> ?

Edited: I guess we don't need an additional PhantomData field, as Ref<T> is covariant. Is that correct?

[nbdd0121]

A pretty synthetic example:

use std::ptr::NonNull;

struct Ref<T: ?Sized> {
    x: NonNull<T>,
    // x: *mut T,
}

fn take<'a>(r: Ref<&'a u32>, y: &'a u32) {}

fn give() -> Ref<&'static u32> { todo!() }

fn test() {
    let y = 1;
    take(give(), &y);
}

If you change x to *mut T the code won't compile.

With that said, we still need additional PhantomData field here, but it's not about variance, but rather drop check: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomData.html#ownership-and-the-drop-check. As Ref can potentially cause T to be dropped, an PhantomData marker is still needed.

[TheSven73]

Thanks Gary, that's awesome! My naive view of covariance is this:

struct Dog;
struct Cat;

trait Animal {
    fn sound(&self);
}

impl Animal for Dog {
    fn sound(&self) {}
}

impl Animal for Cat {
    fn sound(&self) {}
}

let c = Arc::try_new(Cat)?;
let d = Arc::try_new(Dog)?;
let a: Arc<dyn Animal> = d; // this works ok, covariant

But for Ref<T> (our Arc replacement), that doesn't appear to work, what am I missing?

let c = Ref::try_new(Cat)?;
let d = Ref::try_new(Dog)?;
let a: Ref<dyn Animal> = c;

error[E0308]: mismatched types
    |
312 |         let a: Ref<dyn Animal> = c;
    |                ---------------   ^ expected trait object `dyn Animal`, found struct `Cat`

[wedsonaf]

This piece is missing. Gary helped me with this on zulip, here's a link the solution: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b1e7c2183fd2ee4f1970b19864d6ed6b

Since it requires 3 additional unstable features and we don't need it immediately (we will need it when I convert the usage of lists of dyn DeliverToRead in binder), I thought it would make sense to push it out to its own PR.

[TheSven73]

We
also need to limit the scope of obj as it cannot be accessed after the refcount is
decremented.

Is that statement technically correct?
Or does the inner reference stay valid right up until it goes out of scope at the end of the function?

[wedsonaf]

I need to clarify that this applies only when the count is non-zero. Let's say the count went to n (> 0) after the decrement; I cannot dereference the pointer anymore because other threads/CPUs may concurrently drop it to zero and free it.

If the count reaches zero, then no other thread/CPU can possibly be using it, and the current thread/CPU is tasked with freeing it.

I'll update the comment.

[wedsonaf]

As I was updating the code, I noticed that the comment is actually correct as I'm talking about the scope of the local reference.

I need all instances of self.ptr.as_ref() to be dropped before calling Box::from_raw(self.ptr.as_ptr()) otherwise I'll have a T and &T at the same time (for the same object), which is a violation of aliasing rules.

[TheSven73]

Technically, there's nothing wrong with keeping the inner reference alive for the whole duration of the function, as long as no-one "touches it" when it's invalid. But I agree that it's better to censor inner starting from the point where it becomes unsound to touch it.

I am guessing that in the current code, inner lives slightly too long. Would it be beneficial to drop it right after it becomes unsound to touch? For example:

    fn drop(&mut self) {
        let is_zero = {
            // SAFETY: By the type invariant, there is necessarily a reference to the object.
            let inner = unsafe { self.ptr.as_ref() };
            // INVARIANT: If the refcount reaches zero, there are no other instances of `Ref`, and
            // this instance is being dropped, so the broken invariant is not observable.
            // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
            // `inner` must no longer be accessed after the refcount is decremented.
            unsafe { rust_helper_refcount_dec_and_test(inner.refcount.get()) }
        };
        if !is_zero {
            return;
        }

        // The count reached zero, we must free the memory.
        //
        // SAFETY: The pointer was initialised from the result of `Box::leak`.
        unsafe { Box::from_raw(self.ptr.as_ptr()) };
    }

[wedsonaf]

Technically, there's nothing wrong with keeping the inner reference alive for the whole duration of the function, as long as no-one "touches it" when it's invalid. But I agree that it's better to censor inner starting from the point where it becomes unsound to touch it.

Well, the argument about not touching may be true for C, not really for Rust. The mere existence of incompatible references is considered UB, even if one of them is never touched. For example (playground):

fn main() {
    let v = 3u32;
    let v_mut = unsafe { &mut *((&v as *const u32) as *mut u32) }; 
    println!("{}", v)
}

We never touch v_mut, but Miri flags this as UB:

error: Undefined Behavior: trying to reborrow for Unique at alloc1426, but parent tag <untagged> does not have an appropriate item in the borrow stack
 --> src/main.rs:3:26
  |
3 |     let v_mut = unsafe { &mut *((&v as *const u32) as *mut u32) }; 
  |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique at alloc1426, but parent tag <untagged> does not have an appropriate item in the borrow stack
  |

I am guessing that in the current code, inner lives slightly too long. Would it be beneficial to drop it right after it becomes unsound to touch? For example:

    fn drop(&mut self) {
        let is_zero = {
            // SAFETY: By the type invariant, there is necessarily a reference to the object.
            let inner = unsafe { self.ptr.as_ref() };
            // INVARIANT: If the refcount reaches zero, there are no other instances of `Ref`, and
            // this instance is being dropped, so the broken invariant is not observable.
            // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
            // `inner` must no longer be accessed after the refcount is decremented.
            unsafe { rust_helper_refcount_dec_and_test(inner.refcount.get()) }
        };
        if !is_zero {
            return;
        }

        // The count reached zero, we must free the memory.
        //
        // SAFETY: The pointer was initialised from the result of `Box::leak`.
        unsafe { Box::from_raw(self.ptr.as_ptr()) };
    }

You're right, it lives for too long. In fact, even your suggested version also has UB: after rust_helper_refcount_dec_and_test we still have inner (&InnerRef<T>) while another thread may have freed it. So I think we need to avoid it altogether while calling rust_helper_refcount_dec_and_test. Something like:

    fn drop(&mut self) {
        // SAFETY: By the type invariant, there is necessarily a reference to the object.
        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();

        // INVARIANT: If the refcount reaches zero, there are no other instances of `Ref`, and
        // this instance is being dropped, so the broken invariant is not observable.
        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
        let is_zero = unsafe { rust_helper_refcount_dec_and_test(refcount) };
        if is_zero {
            // The count reached zero, we must free the memory.
            //
            // SAFETY: The pointer was initialised from the result of `Box::leak`.
            unsafe { Box::from_raw(self.ptr.as_ptr()) };
        }
    }

Since refcount is a raw pointer, it's not violating any aliasing rules.

[wedsonaf]

Does that mean it's valid for the shared invalid reference to exist, as long as it's not dereffed?

No. Here's another example (playground) without a function:

use std::ptr::NonNull;
use std::boxed::Box;

fn main() {
    let ptr = NonNull::from(Box::leak(Box::new(3u32)));
    let shared_ref = unsafe { ptr.as_ref() };
    unsafe { Box::from_raw(ptr.as_ptr()); }
    println!("I'm not touching _shared_ref");
    let another_ref = shared_ref;
}

Here neither shared_ref nor another_ref are ever dereferenced. But the usage of shared_ref afterwards prevents NLL from limiting shared_ref's lifetime. And miri flags UB.

[nbdd0121]

Does that mean it's valid for the shared invalid reference to exist, as long as it's not dereffed?

It's possible for invalid reference to exist even in safe Rust:

let mut v = vec![1];
let x = &v[0];
v.push(2);
// now x is invalid reference

This is accepted by NLL. Obviously borrow checking will prevent you from accessing x ever again, but after borrow checking all lifetimes are erased, and for codegen x is just an invalid reference after push.

let another_ref = shared_ref;

This is considered as an use. If you remove this then it's not UB.

[TheSven73]

So we have a consensus now? We'll keep a refcount: *mut bindings::refcount_t in scope, although it's not always valid, which nevertheless does not violate Rust's aliasing rules. And we briefly document all of this in the code.

Except @nbdd0121 if you'd like to suggest something better?

[wedsonaf]

Does that mean it's valid for the shared invalid reference to exist, as long as it's not dereffed?

It's possible for invalid reference to exist even in safe Rust:

let mut v = vec![1];
let x = &v[0];
v.push(2);
// now x is invalid reference

This is accepted by NLL. Obviously borrow checking will prevent you from accessing x ever again, but after borrow checking all lifetimes are erased, and for codegen x is just an invalid reference after push.

Right, if something is inaccessible it obviously doesn't count for the aliasing rules. If it did, something like:

fn f(p: &u32) {
    todo!()
}

fn g(q: &mut u32) {
    f(q)
}

Would be a violation of aliasing rules because you have a &T and &mut T at the same time. It isn't because q: &mut u32 becomes inaccessible during the call to f.

let another_ref = shared_ref;

This is considered as an use. If you remove this then it's not UB.

It's an use, of course, but not a dereferencing of the pointer.

[nbdd0121]

So we have a consensus now? We'll keep a refcount: *mut bindings::refcount_t in scope, although it's not always valid, which nevertheless does not violate Rust's aliasing rules. And we briefly document all of this in the code.

The current approach looks good to me. I just want to point out that it is okay to have a local (but not as function parameter) dangling/invalid reference as long as it is not used, but obviously without borrow checker doing the enforcement it is preferable to avoid having it in scope at all.

[wedsonaf]

New version:

• Suggestions from Miguel's review
• Added comment about implementing core::ops::Receiver
• Fixed typo in comment in try_new_and_init
• Updated drop to avoid potential UB by breaking aliasing rules with inner
• Added PhantomData to Ref for drop check

[bjorn3]

Are these not exported from the bindings module?

[TheSven73]

I don't think so, they're a static inline defined in a header unfortunately:

linux/include/linux/refcount.h

Lines 265 to 268 in a5a22ed

	static inline void refcount_inc(refcount_t *r)
	{
	__refcount_inc(r, NULL);
	}

linux/include/linux/refcount.h

Lines 331 to 334 in a5a22ed

	static inline __must_check bool refcount_dec_and_test(refcount_t *r)
	{
	return __refcount_dec_and_test(r, NULL);
	}

[bjorn3]

I mean the rust_helper_* methods. Or are those invisible to bindgen?

[TheSven73]

I think the rust_helper_* methods are a work-around to be able to call header inline/static functions. bindgen does not export them. So we create a C helper function in helpers.c which in turn calls the inline/static header function. Now we can call the helper function in Rust.

[ojeda]

It should work through bindgen too. We have #352 and #359.

[wedsonaf]

I'll move along with this as it's the current state of the art (however ugly it may be). We can update it once we have more automated support.

[bjorn3]

This feature is not meant to eventually be stabilized. It doesn't have a tracking issue and Receiver is marked as #[doc(hidden)].

[nbdd0121]

Given that we are going to have a in-tree copy of alloc crate I think it's okay to use it for now. We'll need DispatchFromDyn anyway which is also hidden. I think eventually we should push for a stable way in Rust to define custom smart pointers that are as usable as Arc.

[bjorn3]

Receiver could be stabilized if wanted. For DispatchFromDyn this is not as easy as by definition it relies on an unstable implementation detail. An incorrect implementation of it can cause an ICE or miscompilation I think. Basically for Foo to have a valid DispatchFromDyn impl, Foo<dyn Trait> must consist of only a single fat pointer with the data part being identical to the thin pointer in Foo<ConcreteType>. Rustc assumes that Foo<dyn Trait> is passed in two registers. The second register contains the vtable and is used to get the method function pointer. The first register is then passed as sole argument to the method. Turning Foo<ConcreteType> into Foo<dyn Trait> also depends on another unstable feature called CoerceUnsized, which itself depends on Unsize.

[wedsonaf]

To add some more context to what Gary already said: we had a technical meeting on the alloc crate last week. The consensus was that we'll have a "fork" of alloc in the kernel tree; changes directly to it are discouraged but in the event that they are needed, we will work with the Rust community to upstream them so in steady-state our fork and upstream should be the same.

A replacement for Arc was explicitly discussed, and @joshtriplett mentioned that it may be reasonable for Rust to allow Arc-like implementations that are not so tied to the compiler, and that we should explore this as a way to minimise changes to our fork of alloc. While this isn't a reality, and once our fork is committed, we can move this Ref implementation to our fork of alloc if we want to minimise the risk of other components accidentally relying on unstable features required by Ref.

[ksquirrel]

Review of 6ba4d643feb8:

• ✔️ Commit 6ba4d64: Looks fine!

[wedsonaf]

Updated comment. PTAL.