[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <723538c0-bee9-cf15-7dd9-6ddaebdd6b4e@virtuozzo.com>
Date: Mon, 19 Jun 2017 17:53:34 +0300
From: Dmitry Safonov <dsafonov@...tuozzo.com>
To: linux-kernel@...r.kernel.org
Cc: 0x7f454c46@...il.com, Al Viro <viro@...iv.linux.org.uk>,
Arve Hjønnevåg <arve@...roid.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Riley Andrews <riandrews@...roid.com>,
devel@...verdev.osuosl.org,
Android Kernel Team <kernel-team@...roid.com>
Subject: 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 <viro@...iv.linux.org.uk>
> Cc: "Arve Hjønnevåg" <arve@...roid.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Riley Andrews <riandrews@...roid.com>
> Cc: devel@...verdev.osuosl.org
> Cc: Android Kernel Team <kernel-team@...roid.com>
> Signed-off-by: Dmitry Safonov <dsafonov@...tuozzo.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);
>
Powered by blists - more mailing lists