Re: [PATCH] binder: Drop needless locking in binder_mmap()
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
> > Cc: "Arve Hjønnevåg"
> > Cc: Greg Kroah-Hartman
> > Cc: Riley Andrews
> > Cc: de...@driverdev.osuosl.org
> > Cc: Android Kernel Team
> > Signed-off-by: Dmitry Safonov
> > ---
> > 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
Re: [PATCH] binder: Drop needless locking in binder_mmap()
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
> > Cc: "Arve Hjønnevåg"
> > Cc: Greg Kroah-Hartman
> > Cc: Riley Andrews
> > Cc: de...@driverdev.osuosl.org
> > Cc: Android Kernel Team
> > Signed-off-by: Dmitry Safonov
> > ---
> > 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
Re: [PATCH] binder: Drop needless locking in binder_mmap()
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
Cc: "Arve Hjønnevåg"
Cc: Greg Kroah-Hartman
Cc: Riley Andrews
Cc: de...@driverdev.osuosl.org
Cc: Android Kernel Team
Signed-off-by: Dmitry Safonov
---
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(_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(_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(_mmap_lock);
vfree(proc->buffer);
proc->buffer = NULL;
err_get_vm_area_failed:
err_already_mapped:
- mutex_unlock(_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()
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
Cc: "Arve Hjønnevåg"
Cc: Greg Kroah-Hartman
Cc: Riley Andrews
Cc: de...@driverdev.osuosl.org
Cc: Android Kernel Team
Signed-off-by: Dmitry Safonov
---
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(_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(_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(_mmap_lock);
vfree(proc->buffer);
proc->buffer = NULL;
err_get_vm_area_failed:
err_already_mapped:
- mutex_unlock(_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);
[PATCH] 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
Cc: "Arve Hjønnevåg"
Cc: Greg Kroah-Hartman
Cc: Riley Andrews
Cc: de...@driverdev.osuosl.org
Cc: Android Kernel Team
Signed-off-by: Dmitry Safonov
---
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(_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(_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(_mmap_lock);
vfree(proc->buffer);
proc->buffer = NULL;
err_get_vm_area_failed:
err_already_mapped:
- mutex_unlock(_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
[PATCH] 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
Cc: "Arve Hjønnevåg"
Cc: Greg Kroah-Hartman
Cc: Riley Andrews
Cc: de...@driverdev.osuosl.org
Cc: Android Kernel Team
Signed-off-by: Dmitry Safonov
---
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(_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(_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(_mmap_lock);
vfree(proc->buffer);
proc->buffer = NULL;
err_get_vm_area_failed:
err_already_mapped:
- mutex_unlock(_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
