Re: [PATCH 3/7] rust_binder: Implement the BINDER_DEBUG_USER_ERROR logging mask for reference counting and death notification operations

[Alice Ryhl]

On Fri, Jul 03, 2026 at 03:29:24PM +0000, Jahnavi MN wrote:
> This adds dynamic debug logs for:
> - Decrementing handle reference counts that are already zero.
> - Mismatched reference states (calling inc_ref_done with no active
>   inc_refs, or using a weak reference as a strong reference).
> - Requesting or clearing death notifications on invalid references,
>   already active notifications, or with mismatched cookies.
> 
> Signed-off-by: Jahnavi MN <jahnavimn@xxxxxxxxxx>

The commit title is pretty long. Could we shorten it? For example:

rust_binder: use BINDER_DEBUG_USER_ERROR mask for refcount errors

>              *count += 1;
>          } else {
>              if *count == 0 {
> -                pr_warn!(
> -                    "pid {} performed invalid decrement on ref\n",
> -                    kernel::current!().pid()
> +                binder_debug!(
> +                    crate::debug::BINDER_DEBUG_USER_ERROR,
> +                    "performed invalid decrement on ref (strong: {})",
> +                    strong

Instead of printing (strong: true) or (strong: false), I think it would
be nicer to print strong or weak directly in the text:

	binder_debug!(
	    crate::debug::BINDER_DEBUG_USER_ERROR,
	    "performed invalid {} decrement on ref",
	    if strong { "strong" } else { "weak" }

>      pub(crate) fn get_transaction_node(&self, handle: u32) -> BinderResult<NodeRef> {
> -        if handle == 0 {
> -            Ok(self.ctx.get_manager_node(true)?)
> +        // When handle is zero, try to get the context manager.
> +        let res = if handle == 0 {
> +            self.ctx.get_manager_node(true)
>          } else {
> -            Ok(self.get_node_from_handle(handle, true)?)
> +            self.get_node_from_handle(handle, true).map_err(Into::into)
> +        };
> +
> +        match res {
> +            Ok(node_ref) => Ok(node_ref),
> +            Err(err) => {
> +                binder_debug!(
> +                    crate::debug::BINDER_DEBUG_USER_ERROR,
> +                    "got transaction to invalid handle {}",
> +                    handle
> +                );
> +                Err(err)
> +            }

This is probably only a user error if `handle != 0`. The application has
no way of knowing the context manager is dead, so that case isn't a bug
in the application.

So I think we can move this println inside the else {} block.

> @@ -1268,17 +1291,27 @@ pub(crate) fn clear_death(&self, reader: &mut UserSliceReader, thread: &Thread)
>  
>          let mut refs = self.node_refs.lock();
>          let Some(info) = refs.by_handle.get_mut(&handle) else {
> -            pr_warn!("BC_CLEAR_DEATH_NOTIFICATION invalid ref {handle}\n");
> +            binder_debug!(
> +                crate::debug::BINDER_DEBUG_USER_ERROR,
> +                "BC_CLEAR_DEATH_NOTIFICATION invalid ref {}",
> +                handle
> +            );

Does the {handle} syntax that the println is currently using work here?
If not, then I think we should try to get it working.

Alice