binder: Drop needless locking in binder_mmap()
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.
--- drivers/android/binder.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/android/binder.c +0 -5
@@ -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
Quoted 27 lines · Dmitry Safonov
binder_mmap()protectedproc->bufferfrom concurrent changes. It was introduced with commitbd1eff9741af("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 allowmmap()by process other thanproc->tsk"). That commit prohibited mmap'ing of fd, that was opened by some other task.
That restriction has made locking inbinder_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]>Signed-off-by Dmitry Safonov <[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@@ -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);
Greg Kroah-Hartman
maintainerQuoted 32 lines · Dmitry Safonov
Quoted 27 lines · Dmitry Safonov
binder_mmap()protectedproc->bufferfrom concurrent changes. It was introduced with commitbd1eff9741af("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 allowmmap()by process other thanproc->tsk"). That commit prohibited mmap'ing of fd, that was opened by some other task.
That restriction has made locking inbinder_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]>Signed-off-by Dmitry Safonov <[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