rust_binder: add ownership assertion to Node::add_death

The // SAFETY: comment in NodeDeath::set_cleared assumes that a NodeDeath is never inserted into the death list of any Node other than its owner. However, this invariant is not enforced by the safe function Node::add_death, which inserts NodeDeath into the death list without checking that death.node == self, leaving a risk for future code that may miss this implicit invariant and cause undefined behavior.

Add an assertion to make this precondition explicit and catch potential violations early.

---
 drivers/android/binder/node.rs | 4 ++++
 1 file changed, 4 insertions(+)

index 69f757ff7461..20eaaefbd4cc 100644
@@ -333,6 +333,10 @@ pub(crate) fn add_death(
         death: ListArc<DTRWrap<NodeDeath>, 1>,
         guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
     ) {
+        assert!(
+            core::ptr::eq(self, &**death.node),
+            "attempt to add NodeDeath to the wrong death list"
+        );
         self.inner.access_mut(guard).death_list.push_back(death);
     }
 

base-commit: 287afdc7671a03081f48f3407bc59862c202bd4b

2.47.3

Yeah, there seems to be confusion about the asserts.

If there were a soundness issue, then an assert! may "fix" it, but it would usually the wrong way to do so, and a debug_assert! wouldn't be a fix it at all.

And for other cases, assert! is typically too strong for Linux (outside const context, tests, special cfgs, etc.).

In addition, please consider whether a pr_warn(_once)! or similar may be a good idea to pair with the debug_assert!.

Cheers,
Miguel

pr_warn is probably a bad idea here. Given the code relies it for soundness. You're either sure that it won't happen, then you use debug_assert!, or you are not sure, and use assert!.

There's no "I am fairly certain but the code should keep running despite invariance violation" for this one.

Best,
Gary

[ I keep getting your emails way later than they appear in the list...
I spotted this one as well in the mailing list. ]

I agree that continuining in this particular case is quite bad, and you know I would be stricter than the C side for this sort of thing -- it is closer to an indexing gone wrong where we panic as well.

But to clarify, the pr_warn! is not the important bit here -- I was giving the general rule that if debug_assert! is OK in a particular situation, then as usual we should consider a pr_warn! as well, i.e. that is the Erroneous Behavior combo for us.

And if it is not OK to continue in a certain situation, then something else entirely needs to be done.

That is what I pointed out in the GitHub issue, i.e. that the original debug_assert! suggestion cannot fix a soundness issue.

Cheers,
Miguel

If you're using gmail, you might be hitting the receive limit? You can receive at most 60 emails per minute.

If you have more emails then it'll bounce. My email provider will retry after 15 minutes for up to 6 times, so if you're consistently hitting the receive limit, then it might take up to 1.5 hr for you to receive my email (or for me to receive a delivery failure notification).

Best,
Gary

Hmm... I don't think that is the issue (I only noticed it with you so far). It is also way more than 1 hour. The other one, for instance, was about a day late, to the point that I got this last one of yours earlier than the other one (!).

I guess I will ask the admins.

Cheers,
Miguel

The // SAFETY: comment in NodeDeath::set_cleared assumes that a NodeDeath is never inserted into the death list of any Node other than its owner. However, this invariant is not enforced by the safe function Node::add_death, which inserts NodeDeath into the death list without checking that death.node == self, leaving a risk for future code that may miss this implicit invariant and cause undefined behavior.

Add an assertion to make this precondition explicit and catch potential violations early.

---
Changes in v2:
- Replace assert!() with pr_warn() + debug_assert() following feedback from Onur Özkan and Miguel Ojeda.

Link to v1: https://lore.kernel.org/rust-for-linux/[email protected]/

---
 drivers/android/binder/node.rs | 5 +++++
 1 file changed, 5 insertions(+)

index 69f757ff7461..425076405e1e 100644
@@ -333,6 +333,11 @@ pub(crate) fn add_death(
         death: ListArc<DTRWrap<NodeDeath>, 1>,
         guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
     ) {
+        let is_valid = core::ptr::eq(self, &**death.node);
+        if !is_valid {
+            pr_warn!("attempt to add NodeDeath to the wrong death list\n");
+        }
+        debug_assert!(is_valid);
         self.inner.access_mut(guard).death_list.push_back(death);
     }
 

base-commit: 287afdc7671a03081f48f3407bc59862c202bd4b

2.47.3

If the Binder maintainers decide they want the pr_warn! (_once?), then this could go inside the if with a false condition.

Cheers,
Miguel

Thank you for the feedback. Would you prefer we go back to the original patch suggestion with the assert!() that contains the message, without pr_warn()?

        assert!(
            core::ptr::eq(self, &**death.node),
            "attempt to add NodeDeath to the wrong death list"
        );

Best,
Georgios

Doing an assert is fine, thanks.

Alice

The // SAFETY: comment in NodeDeath::set_cleared assumes that a NodeDeath is never inserted into the death list of any Node other than its owner. However, this invariant is not enforced by the safe function Node::add_death, which inserts NodeDeath into the death list without checking that death.node == self, leaving a risk for future code that may miss this implicit invariant and cause undefined behavior.

Add an assertion to make this precondition explicit and catch potential violations early.

---
Changes in v3:
- Reverted to assert!() following feedback from Alice Ryhl.

Link to v2: https://lore.kernel.org/rust-for-linux/[email protected]/

---
 drivers/android/binder/node.rs | 4 ++++
 1 file changed, 4 insertions(+)

index 69f757ff7461..20eaaefbd4cc 100644
@@ -333,6 +333,10 @@ pub(crate) fn add_death(
         death: ListArc<DTRWrap<NodeDeath>, 1>,
         guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
     ) {
+        assert!(
+            core::ptr::eq(self, &**death.node),
+            "attempt to add NodeDeath to the wrong death list"
+        );
         self.inner.access_mut(guard).death_list.push_back(death);
     }
 

2.47.3