[PATCH] binder: Drop needless locking in binder_mmap()

[PATCH] binder: Drop needless locking in binder_mmap()

[Dmitry Safonov]

binder_mmap() protected proc->buffer from concurrent changes.
It was introduced with commit bd1eff9741af ("Staging: android: binder:
Fix crashes when sharing a binder file between processes").

The commit fixed such races alike:

task1
=====
fd = open("/dev/binder",..)
fork()
---------------------------------------->task2
  |                                      =====
mmap(fd)				mmap(fd)

Afterwards the situation was changed with the commit a79f41ed9786
("binder: don't allow mmap() by process other than proc->tsk").
That commit prohibited mmap'ing of fd, that was opened by some
other task.
That restriction has made locking in binder_mmap() unnecessary.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: "Arve Hjønnevåg" <arve@android.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Riley Andrews <riandrews@android.com>
Cc: devel@driverdev.osuosl.org
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 drivers/android/binder.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index aae4d8d4be36..b5b32be85ab8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -49,7 +49,6 @@
 
 static DEFINE_MUTEX(binder_main_lock);
 static DEFINE_MUTEX(binder_deferred_lock);
-static DEFINE_MUTEX(binder_mmap_lock);
 
 static HLIST_HEAD(binder_devices);
 static HLIST_HEAD(binder_procs);
@@ -3381,7 +3380,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
 
-	mutex_lock(&binder_mmap_lock);
 	if (proc->buffer) {
 		ret = -EBUSY;
 		failure_string = "already mapped";
@@ -3396,7 +3394,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	proc->buffer = area->addr;
 	proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
-	mutex_unlock(&binder_mmap_lock);
 
 #ifdef CONFIG_CPU_CACHE_VIPT
 	if (cache_is_vipt_aliasing()) {
@@ -3441,12 +3438,10 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 	kfree(proc->pages);
 	proc->pages = NULL;
 err_alloc_pages_failed:
-	mutex_lock(&binder_mmap_lock);
 	vfree(proc->buffer);
 	proc->buffer = NULL;
 err_get_vm_area_failed:
 err_already_mapped:
-	mutex_unlock(&binder_mmap_lock);
 err_bad_arg:
 	pr_err("binder_mmap: %d %lx-%lx %s failed %d\n",
 	       proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
-- 
2.12.2

Re: [PATCH] binder: Drop needless locking in binder_mmap()

[Dmitry Safonov]

On 05/24/2017 05:51 PM, Dmitry Safonov wrote:
> binder_mmap() protected proc->buffer from concurrent changes.
> It was introduced with commit bd1eff9741af ("Staging: android: binder:
> Fix crashes when sharing a binder file between processes").
> 
> The commit fixed such races alike:
> 
> task1
> =====
> fd = open("/dev/binder",..)
> fork()
> ---------------------------------------->task2
>    |                                      =====
> mmap(fd)				mmap(fd)
> 
> Afterwards the situation was changed with the commit a79f41ed9786
> ("binder: don't allow mmap() by process other than proc->tsk").
> That commit prohibited mmap'ing of fd, that was opened by some
> other task.
> That restriction has made locking in binder_mmap() unnecessary.
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: "Arve Hjønnevåg" <arve@android.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Riley Andrews <riandrews@android.com>
> Cc: devel@driverdev.osuosl.org
> Cc: Android Kernel Team <kernel-team@android.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> ---
>   drivers/android/binder.c | 5 -----
>   1 file changed, 5 deletions(-)

Ping?

> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index aae4d8d4be36..b5b32be85ab8 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -49,7 +49,6 @@
>   
>   static DEFINE_MUTEX(binder_main_lock);
>   static DEFINE_MUTEX(binder_deferred_lock);
> -static DEFINE_MUTEX(binder_mmap_lock);
>   
>   static HLIST_HEAD(binder_devices);
>   static HLIST_HEAD(binder_procs);
> @@ -3381,7 +3380,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>   	}
>   	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
>   
> -	mutex_lock(&binder_mmap_lock);
>   	if (proc->buffer) {
>   		ret = -EBUSY;
>   		failure_string = "already mapped";
> @@ -3396,7 +3394,6 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>   	}
>   	proc->buffer = area->addr;
>   	proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
> -	mutex_unlock(&binder_mmap_lock);
>   
>   #ifdef CONFIG_CPU_CACHE_VIPT
>   	if (cache_is_vipt_aliasing()) {
> @@ -3441,12 +3438,10 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>   	kfree(proc->pages);
>   	proc->pages = NULL;
>   err_alloc_pages_failed:
> -	mutex_lock(&binder_mmap_lock);
>   	vfree(proc->buffer);
>   	proc->buffer = NULL;
>   err_get_vm_area_failed:
>   err_already_mapped:
> -	mutex_unlock(&binder_mmap_lock);
>   err_bad_arg:
>   	pr_err("binder_mmap: %d %lx-%lx %s failed %d\n",
>   	       proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
>

Re: [PATCH] binder: Drop needless locking in binder_mmap()

[Greg Kroah-Hartman]

On Mon, Jun 19, 2017 at 05:53:34PM +0300, Dmitry Safonov wrote:
> On 05/24/2017 05:51 PM, Dmitry Safonov wrote:
> > binder_mmap() protected proc->buffer from concurrent changes.
> > It was introduced with commit bd1eff9741af ("Staging: android: binder:
> > Fix crashes when sharing a binder file between processes").
> > 
> > The commit fixed such races alike:
> > 
> > task1
> > =====
> > fd = open("/dev/binder",..)
> > fork()
> > ---------------------------------------->task2
> >    |                                      =====
> > mmap(fd)				mmap(fd)
> > 
> > Afterwards the situation was changed with the commit a79f41ed9786
> > ("binder: don't allow mmap() by process other than proc->tsk").
> > That commit prohibited mmap'ing of fd, that was opened by some
> > other task.
> > That restriction has made locking in binder_mmap() unnecessary.
> > 
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: "Arve Hjønnevåg" <arve@android.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Riley Andrews <riandrews@android.com>
> > Cc: devel@driverdev.osuosl.org
> > Cc: Android Kernel Team <kernel-team@android.com>
> > Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> > ---
> >   drivers/android/binder.c | 5 -----
> >   1 file changed, 5 deletions(-)
> 
> Ping?

The "break up the binder lock" patches should have now made this
obsolete, right?  If not, can you please rebase it on my
char-misc-testing tree and resend?

thanks,

greg k-h