Skip to contentSkip to conversation

binder: Drop needless locking in binder_mmap()

PATCH In review Dmitry Safonov 2 participants 3 messages May 24, 2017 – Jul 17, 2017 Android drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 binder: Drop needless locking in binder_mmap()

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 <[email protected]>
Cc "Arve Hjønnevåg" <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Riley Andrews <[email protected]>
Cc Android Kernel Team <[email protected]>
---
 drivers/android/binder.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/android/binder.c +0 -5
index aae4d8d4be36..b5b32be85ab8 100644
@@ -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);
Signature · 1 line
2.12.2
#1 lore raw
Quoted 27 lines · 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 <[email protected]>
Cc "Arve Hjønnevåg" <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Riley Andrews <[email protected]>
Cc Android Kernel Team <[email protected]>
Quoted 3 lines
---
  drivers/android/binder.c | 5 -----
  1 file changed, 5 deletions(-)

Ping?

Quoted 43 lines
drivers/android/binder.c +0 -5
index aae4d8d4be36..b5b32be85ab8 100644
@@ -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 lore raw
Quoted 32 lines · Dmitry Safonov
Quoted 27 lines · 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 <[email protected]>
Cc "Arve Hjønnevåg" <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Riley Andrews <[email protected]>
Cc Android Kernel Team <[email protected]>
Quoted 3 lines
---
  drivers/android/binder.c | 5 -----
  1 file changed, 5 deletions(-)
Quoted 1 line

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