[PATCH] rust_binder: add ownership assertion to Node::add_death

[PATCH] rust_binder: add ownership assertion to Node::add_death

[Georgios Androutsopoulos]

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.

Link: https://github.com/Rust-for-Linux/linux/issues/1237

Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
---
 drivers/android/binder/node.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
index 69f757ff7461..20eaaefbd4cc 100644
--- a/drivers/android/binder/node.rs
+++ b/drivers/android/binder/node.rs
@@ -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

Re: [PATCH] rust_binder: add ownership assertion to Node::add_death

[Onur Özkan]

On Tue, 09 Jun 2026 23:55:44 -0400
Georgios Androutsopoulos <georgeandrout13@gmail.com> wrote:

> 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.
> 
> Link: https://github.com/Rust-for-Linux/linux/issues/1237
> 
> Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
> ---
>  drivers/android/binder/node.rs | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
> index 69f757ff7461..20eaaefbd4cc 100644
> --- a/drivers/android/binder/node.rs
> +++ b/drivers/android/binder/node.rs
> @@ -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"
> +        );

I think having a `debug_assert!` should be fine. That's also your suggestion in
the GH issue link above.

Thanks,
Onur

>          self.inner.access_mut(guard).death_list.push_back(death);
>      }
>  
> 
> base-commit: 287afdc7671a03081f48f3407bc59862c202bd4b
> -- 
> 2.47.3
> 

Re: [PATCH] rust_binder: add ownership assertion to Node::add_death

[Miguel Ojeda]

On Wed, Jun 10, 2026 at 7:45 AM Onur Özkan <work@onurozkan.dev> wrote:
>
> I think having a `debug_assert!` should be fine. That's also your suggestion in
> the GH issue link above.

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 `cfg`s, 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

Re: [PATCH] rust_binder: add ownership assertion to Node::add_death

[Gary Guo]

On Wed Jun 10, 2026 at 7:07 AM BST, Miguel Ojeda wrote:
> On Wed, Jun 10, 2026 at 7:45 AM Onur Özkan <work@onurozkan.dev> wrote:
>>
>> I think having a `debug_assert!` should be fine. That's also your suggestion in
>> the GH issue link above.
>
> 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 `cfg`s, etc.).
>
> In addition, please consider whether a `pr_warn(_once)!` or similar
> may be a good idea to pair with the `debug_assert!`.

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

Re: Re: [PATCH] rust_binder: add ownership assertion to Node::add_death

[Miguel Ojeda]

On Wed, 10 Jun 2026 14:39:46 +0100 Gary Guo <gary@garyguo.net> wrote:
>
> 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.

[ 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

Re: [PATCH] rust_binder: add ownership assertion to Node::add_death

[Gary Guo]

On Thu Jun 11, 2026 at 9:15 AM BST, Miguel Ojeda wrote:
> On Wed, 10 Jun 2026 14:39:46 +0100 Gary Guo <gary@garyguo.net> wrote:
>>
>> 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.
>
> [ I keep getting your emails way later than they appear in the list...
>   I spotted this one as well in the mailing list. ]

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

[PATCH v2] rust_binder: add ownership assertion to Node::add_death

[Georgios Androutsopoulos]

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.

Link: https://github.com/Rust-for-Linux/linux/issues/1237

Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
---
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/20260610035544.3333022-1-georgeandrout13@gmail.com/
---
 drivers/android/binder/node.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
index 69f757ff7461..425076405e1e 100644
--- a/drivers/android/binder/node.rs
+++ b/drivers/android/binder/node.rs
@@ -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

Re: [PATCH v2] rust_binder: add ownership assertion to Node::add_death

[Miguel Ojeda]

On Wed, Jun 10, 2026 at 3:32 PM Georgios Androutsopoulos
<georgeandrout13@gmail.com> wrote:
>
> debug_assert!(is_valid);

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

Cheers,
Miguel

Re: [PATCH v2] rust_binder: add ownership assertion to Node::add_death

[Alice Ryhl]

On Wed, Jun 10, 2026 at 09:32:39AM -0400, Georgios Androutsopoulos wrote:
> 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.
> 
> Link: https://github.com/Rust-for-Linux/linux/issues/1237
> 
> Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
> ---
> 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/20260610035544.3333022-1-georgeandrout13@gmail.com/
> ---
>  drivers/android/binder/node.rs | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
> index 69f757ff7461..425076405e1e 100644
> --- a/drivers/android/binder/node.rs
> +++ b/drivers/android/binder/node.rs
> @@ -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);

If this assertion fails we should not continue. Either use a full panic,
or do a warn_on! and return without adding it.

Alice