binder: skip dead binder_proc during binder_open

[PATCH] binder: skip dead binder_proc during binder_open

Posted by zhengyan 7 months, 3 weeks ago

During binder_open, the binder_proc list is travesed to check
for the existing binder_proc instances. binder_proc objects
are async released in a deferred work after binder_release,
and may remain temporarily on the binder_procs list even after
being marked as dead.

Without checking the flag, binder_open may face a crash as
"Unable to handle kernel paging request at virtual address
dead000000000140"

Signed-off-by: zhengyan <zhengyan@asrmicro.com>
---
 drivers/android/binder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 76052006bd87..43ab4350e589 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6041,6 +6041,8 @@ static int binder_open(struct inode *nodp, struct file *filp)
 
 	mutex_lock(&binder_procs_lock);
 	hlist_for_each_entry(itr, &binder_procs, proc_node) {
+		if (itr->is_dead)
+			continue;
 		if (itr->pid == proc->pid) {
 			existing_pid = true;
 			break;
-- 
2.25.1

Re: [PATCH] binder: skip dead binder_proc during binder_open

Posted by Carlos Llamas 7 months, 3 weeks ago

On Tue, Apr 29, 2025 at 07:50:30AM +0000, zhengyan wrote:
> During binder_open, the binder_proc list is travesed to check
> for the existing binder_proc instances. binder_proc objects
> are async released in a deferred work after binder_release,
> and may remain temporarily on the binder_procs list even after
> being marked as dead.
> 
> Without checking the flag, binder_open may face a crash as
> "Unable to handle kernel paging request at virtual address
> dead000000000140"
> 
> Signed-off-by: zhengyan <zhengyan@asrmicro.com>
> ---
>  drivers/android/binder.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 76052006bd87..43ab4350e589 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6041,6 +6041,8 @@ static int binder_open(struct inode *nodp, struct file *filp)
>  
>  	mutex_lock(&binder_procs_lock);
>  	hlist_for_each_entry(itr, &binder_procs, proc_node) {
> +		if (itr->is_dead)
> +			continue;
>  		if (itr->pid == proc->pid) {
>  			existing_pid = true;
>  			break;
> -- 
> 2.25.1

Can you please explain this scenario? My understanding is that dead
procs are removed from the binder_procs list (with appropriate locks)
before they are marked as ->is_dead. So how is this even possible?

--
Carlos Llamas

Re: [PATCH] binder: skip dead binder_proc during binder_open

Posted by Greg KH 7 months, 3 weeks ago

On Tue, Apr 29, 2025 at 07:50:30AM +0000, zhengyan wrote:
> During binder_open, the binder_proc list is travesed to check
> for the existing binder_proc instances. binder_proc objects
> are async released in a deferred work after binder_release,
> and may remain temporarily on the binder_procs list even after
> being marked as dead.

Nit, you do have a full 72 columns to use, can you use that?

> 
> Without checking the flag, binder_open may face a crash as
> "Unable to handle kernel paging request at virtual address
> dead000000000140"
> 
> Signed-off-by: zhengyan <zhengyan@asrmicro.com>

Please use your name, not just an email alias.

> ---
>  drivers/android/binder.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 76052006bd87..43ab4350e589 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6041,6 +6041,8 @@ static int binder_open(struct inode *nodp, struct file *filp)
>  
>  	mutex_lock(&binder_procs_lock);
>  	hlist_for_each_entry(itr, &binder_procs, proc_node) {
> +		if (itr->is_dead)
> +			continue;

What commit id does this fix? Should it go to stable kernels?  If so,
how far back?

How was this tested?

thanks,

greg k-h