* [PATCH 0/2] Binder bug fixes @ 2012-01-21 3:56 Arve Hjønnevåg 2012-01-21 3:56 ` [PATCH 1/2] Staging: android: binder: Add some error checks Arve Hjønnevåg 2012-01-21 3:56 ` [PATCH 2/2] Staging: android: binder: Don't call dump_stack in binder_vma_open Arve Hjønnevåg 0 siblings, 2 replies; 11+ messages in thread From: Arve Hjønnevåg @ 2012-01-21 3:56 UTC (permalink / raw) To: linux-kernel; +Cc: Al Viro Fix some problems reported in https://lkml.org/lkml/2011/12/22/311 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] Staging: android: binder: Add some error checks 2012-01-21 3:56 [PATCH 0/2] Binder bug fixes Arve Hjønnevåg @ 2012-01-21 3:56 ` Arve Hjønnevåg 2012-01-21 8:22 ` Dan Carpenter 2012-01-21 3:56 ` [PATCH 2/2] Staging: android: binder: Don't call dump_stack in binder_vma_open Arve Hjønnevåg 1 sibling, 1 reply; 11+ messages in thread From: Arve Hjønnevåg @ 2012-01-21 3:56 UTC (permalink / raw) To: linux-kernel Cc: Al Viro, Arve Hjønnevåg, Greg Kroah-Hartman, Christopher Lais, devel - Add a mutex to protect against two processes mmapping the same binder_proc. - After locking mmap_sem, check that the vma we want to access (still) points to the same mm_struct. - Use proc->tsk instead of current to get the files struct since this is where we get the rlimit from. Signed-off-by: Arve Hjønnevåg <arve@android.com> --- drivers/staging/android/binder.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 7491801..846b429 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -38,6 +38,7 @@ static DEFINE_MUTEX(binder_lock); static DEFINE_MUTEX(binder_deferred_lock); +static DEFINE_MUTEX(binder_mmap_lock); static HLIST_HEAD(binder_procs); static HLIST_HEAD(binder_deferred_list); @@ -632,6 +633,11 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate, if (mm) { down_write(&mm->mmap_sem); vma = proc->vma; + if (vma && mm != vma->vm_mm) { + pr_err("binder: %d: vma mm and task mm mismatch\n", + proc->pid); + vma = NULL; + } } if (allocate == 0) @@ -2803,6 +2809,7 @@ 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"; @@ -2817,6 +2824,7 @@ 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()) { @@ -2849,7 +2857,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) binder_insert_free_buffer(proc, buffer); proc->free_async_space = proc->buffer_size / 2; barrier(); - proc->files = get_files_struct(current); + proc->files = get_files_struct(proc->tsk); proc->vma = vma; /*printk(KERN_INFO "binder_mmap: %d %lx-%lx maps %p\n", @@ -2860,10 +2868,12 @@ err_alloc_small_buf_failed: 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: printk(KERN_ERR "binder_mmap: %d %lx-%lx %s failed %d\n", proc->pid, vma->vm_start, vma->vm_end, failure_string, ret); -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-01-21 3:56 ` [PATCH 1/2] Staging: android: binder: Add some error checks Arve Hjønnevåg @ 2012-01-21 8:22 ` Dan Carpenter 2012-01-31 18:52 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2012-01-21 8:22 UTC (permalink / raw) To: Arve Hjønnevåg Cc: linux-kernel, devel, Christopher Lais, Greg Kroah-Hartman, Al Viro [-- Attachment #1: Type: text/plain, Size: 578 bytes --] On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: > - Add a mutex to protect against two processes mmapping the > same binder_proc. > - After locking mmap_sem, check that the vma we want to access > (still) points to the same mm_struct. > - Use proc->tsk instead of current to get the files struct since > this is where we get the rlimit from. This doesn't seem related to the locking change at all. Probably this patch should be split into three patches, one bugfix per patch, unless they are very closely related. regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-01-21 8:22 ` Dan Carpenter @ 2012-01-31 18:52 ` Greg KH 2012-01-31 23:20 ` Arve Hjønnevåg 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2012-01-31 18:52 UTC (permalink / raw) To: Dan Carpenter, Arve Hjønnevåg Cc: devel, Christopher Lais, Greg Kroah-Hartman, linux-kernel, Al Viro On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote: > On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: > > - Add a mutex to protect against two processes mmapping the > > same binder_proc. > > - After locking mmap_sem, check that the vma we want to access > > (still) points to the same mm_struct. > > - Use proc->tsk instead of current to get the files struct since > > this is where we get the rlimit from. > > This doesn't seem related to the locking change at all. Probably > this patch should be split into three patches, one bugfix per > patch, unless they are very closely related. I agree. Arve, is this all fixing one problem, or multiple ones? If multiple ones, we need this split up into multiple patches. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-01-31 18:52 ` Greg KH @ 2012-01-31 23:20 ` Arve Hjønnevåg 2012-02-01 6:53 ` Dan Carpenter 0 siblings, 1 reply; 11+ messages in thread From: Arve Hjønnevåg @ 2012-01-31 23:20 UTC (permalink / raw) To: Greg KH Cc: Dan Carpenter, devel, Christopher Lais, Greg Kroah-Hartman, linux-kernel, Al Viro 2012/1/31 Greg KH <greg@kroah.com>: > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote: >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: >> > - Add a mutex to protect against two processes mmapping the >> > same binder_proc. >> > - After locking mmap_sem, check that the vma we want to access >> > (still) points to the same mm_struct. >> > - Use proc->tsk instead of current to get the files struct since >> > this is where we get the rlimit from. >> >> This doesn't seem related to the locking change at all. Probably >> this patch should be split into three patches, one bugfix per >> patch, unless they are very closely related. > > I agree. Arve, is this all fixing one problem, or multiple ones? If > multiple ones, we need this split up into multiple patches. > That depend on your point of view. It fixes crashes if you use the same binder file pointer from multiple processes. It seemed excessive to have three patches for this. -- Arve Hjønnevåg ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-01-31 23:20 ` Arve Hjønnevåg @ 2012-02-01 6:53 ` Dan Carpenter 2012-02-01 22:29 ` Arve Hjønnevåg 0 siblings, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2012-02-01 6:53 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Greg KH, devel, Christopher Lais, Greg Kroah-Hartman, linux-kernel, Al Viro [-- Attachment #1: Type: text/plain, Size: 1602 bytes --] On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote: > 2012/1/31 Greg KH <greg@kroah.com>: > > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote: > >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: > >> > - Add a mutex to protect against two processes mmapping the > >> > same binder_proc. > >> > - After locking mmap_sem, check that the vma we want to access > >> > (still) points to the same mm_struct. > >> > - Use proc->tsk instead of current to get the files struct since > >> > this is where we get the rlimit from. > >> > >> This doesn't seem related to the locking change at all. Probably > >> this patch should be split into three patches, one bugfix per > >> patch, unless they are very closely related. > > > > I agree. Arve, is this all fixing one problem, or multiple ones? If > > multiple ones, we need this split up into multiple patches. > > > > That depend on your point of view. It fixes crashes if you use the > same binder file pointer from multiple processes. It seemed excessive > to have three patches for this. It would have helped you to write a better changelog. The subject says "[patch] android: grab bag of random fixes" and the the description matches that. You have no idea how annoyed I get at grab bag patches. Also don't ignore review comments. I review a lot of staging patches and I'm not an expert on every driver so my review comments are often wrong. I don't get upset when people tell me that. Review is part of the process. Everybody does it. regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-02-01 6:53 ` Dan Carpenter @ 2012-02-01 22:29 ` Arve Hjønnevåg 2012-02-01 22:47 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Arve Hjønnevåg @ 2012-02-01 22:29 UTC (permalink / raw) To: Dan Carpenter Cc: Greg KH, devel, Christopher Lais, Greg Kroah-Hartman, linux-kernel, Al Viro 2012/1/31 Dan Carpenter <dan.carpenter@oracle.com>: > On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote: >> 2012/1/31 Greg KH <greg@kroah.com>: >> > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote: >> >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: >> >> > - Add a mutex to protect against two processes mmapping the >> >> > same binder_proc. >> >> > - After locking mmap_sem, check that the vma we want to access >> >> > (still) points to the same mm_struct. >> >> > - Use proc->tsk instead of current to get the files struct since >> >> > this is where we get the rlimit from. >> >> >> >> This doesn't seem related to the locking change at all. Probably >> >> this patch should be split into three patches, one bugfix per >> >> patch, unless they are very closely related. >> > >> > I agree. Arve, is this all fixing one problem, or multiple ones? If >> > multiple ones, we need this split up into multiple patches. >> > >> >> That depend on your point of view. It fixes crashes if you use the >> same binder file pointer from multiple processes. It seemed excessive >> to have three patches for this. > > It would have helped you to write a better changelog. The subject > says "[patch] android: grab bag of random fixes" and the the > description matches that. You have no idea how annoyed I get at > grab bag patches. > Would the following be a better change description (or do you still want three patches): Staging: android: binder: Fix crashes when sharing a binder file between processes Opening the binder driver and sharing the file returned with other processes (e.g. by calling fork) can crash the kernel. Prevent these crashes with the following changes: - Add a mutex to protect against two processes mmapping the same binder_proc. - After locking mmap_sem, check that the vma we want to access (still) points to the same mm_struct. - Use proc->tsk instead of current to get the files struct since this is where we get the rlimit from. -- Arve Hjønnevåg ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-02-01 22:29 ` Arve Hjønnevåg @ 2012-02-01 22:47 ` Greg KH 2012-02-01 23:29 ` [PATCH] Staging: android: binder: Fix crashes when sharing a binder file between processes Arve Hjønnevåg 2012-02-02 6:27 ` [PATCH 1/2] Staging: android: binder: Add some error checks Dan Carpenter 0 siblings, 2 replies; 11+ messages in thread From: Greg KH @ 2012-02-01 22:47 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Dan Carpenter, devel, Christopher Lais, linux-kernel, Al Viro On Wed, Feb 01, 2012 at 02:29:36PM -0800, Arve Hjønnevåg wrote: > 2012/1/31 Dan Carpenter <dan.carpenter@oracle.com>: > > On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hjønnevåg wrote: > >> 2012/1/31 Greg KH <greg@kroah.com>: > >> > On Sat, Jan 21, 2012 at 11:22:08AM +0300, Dan Carpenter wrote: > >> >> On Fri, Jan 20, 2012 at 07:56:20PM -0800, Arve Hjønnevåg wrote: > >> >> > - Add a mutex to protect against two processes mmapping the > >> >> > same binder_proc. > >> >> > - After locking mmap_sem, check that the vma we want to access > >> >> > (still) points to the same mm_struct. > >> >> > - Use proc->tsk instead of current to get the files struct since > >> >> > this is where we get the rlimit from. > >> >> > >> >> This doesn't seem related to the locking change at all. Probably > >> >> this patch should be split into three patches, one bugfix per > >> >> patch, unless they are very closely related. > >> > > >> > I agree. Arve, is this all fixing one problem, or multiple ones? If > >> > multiple ones, we need this split up into multiple patches. > >> > > >> > >> That depend on your point of view. It fixes crashes if you use the > >> same binder file pointer from multiple processes. It seemed excessive > >> to have three patches for this. > > > > It would have helped you to write a better changelog. The subject > > says "[patch] android: grab bag of random fixes" and the the > > description matches that. You have no idea how annoyed I get at > > grab bag patches. > > > > Would the following be a better change description (or do you still > want three patches): > > Staging: android: binder: Fix crashes when sharing a binder file > between processes > > Opening the binder driver and sharing the file returned with > other processes (e.g. by calling fork) can crash the kernel. > Prevent these crashes with the following changes: > - Add a mutex to protect against two processes mmapping the > same binder_proc. > - After locking mmap_sem, check that the vma we want to access > (still) points to the same mm_struct. > - Use proc->tsk instead of current to get the files struct since > this is where we get the rlimit from. That looks good to me, as one patch, Dan? greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Staging: android: binder: Fix crashes when sharing a binder file between processes 2012-02-01 22:47 ` Greg KH @ 2012-02-01 23:29 ` Arve Hjønnevåg 2012-02-02 6:27 ` [PATCH 1/2] Staging: android: binder: Add some error checks Dan Carpenter 1 sibling, 0 replies; 11+ messages in thread From: Arve Hjønnevåg @ 2012-02-01 23:29 UTC (permalink / raw) To: gregkh; +Cc: dan.carpenter, linux-kernel, Arve Hjønnevåg Opening the binder driver and sharing the file returned with other processes (e.g. by calling fork) can crash the kernel. Prevent these crashes with the following changes: - Add a mutex to protect against two processes mmapping the same binder_proc. - After locking mmap_sem, check that the vma we want to access (still) points to the same mm_struct. - Use proc->tsk instead of current to get the files struct since this is where we get the rlimit from. Signed-off-by: Arve Hjønnevåg <arve@android.com> --- drivers/staging/android/binder.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 48cf27c..f0b7e66 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -38,6 +38,7 @@ static DEFINE_MUTEX(binder_lock); static DEFINE_MUTEX(binder_deferred_lock); +static DEFINE_MUTEX(binder_mmap_lock); static HLIST_HEAD(binder_procs); static HLIST_HEAD(binder_deferred_list); @@ -632,6 +633,11 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate, if (mm) { down_write(&mm->mmap_sem); vma = proc->vma; + if (vma && mm != vma->vm_mm) { + pr_err("binder: %d: vma mm and task mm mismatch\n", + proc->pid); + vma = NULL; + } } if (allocate == 0) @@ -2802,6 +2808,7 @@ 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"; @@ -2816,6 +2823,7 @@ 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()) { @@ -2848,7 +2856,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) binder_insert_free_buffer(proc, buffer); proc->free_async_space = proc->buffer_size / 2; barrier(); - proc->files = get_files_struct(current); + proc->files = get_files_struct(proc->tsk); proc->vma = vma; /*printk(KERN_INFO "binder_mmap: %d %lx-%lx maps %p\n", @@ -2859,10 +2867,12 @@ err_alloc_small_buf_failed: 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: printk(KERN_ERR "binder_mmap: %d %lx-%lx %s failed %d\n", proc->pid, vma->vm_start, vma->vm_end, failure_string, ret); -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Staging: android: binder: Add some error checks 2012-02-01 22:47 ` Greg KH 2012-02-01 23:29 ` [PATCH] Staging: android: binder: Fix crashes when sharing a binder file between processes Arve Hjønnevåg @ 2012-02-02 6:27 ` Dan Carpenter 1 sibling, 0 replies; 11+ messages in thread From: Dan Carpenter @ 2012-02-02 6:27 UTC (permalink / raw) To: Greg KH Cc: Arve Hjønnevåg, devel, Christopher Lais, linux-kernel, Al Viro [-- Attachment #1: Type: text/plain, Size: 190 bytes --] On Wed, Feb 01, 2012 at 02:47:08PM -0800, Greg KH wrote: > That looks good to me, as one patch, Dan? Looks good. Acked-by: Dan Carpenter <dan.carpenter@oracle.com> regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] Staging: android: binder: Don't call dump_stack in binder_vma_open 2012-01-21 3:56 [PATCH 0/2] Binder bug fixes Arve Hjønnevåg 2012-01-21 3:56 ` [PATCH 1/2] Staging: android: binder: Add some error checks Arve Hjønnevåg @ 2012-01-21 3:56 ` Arve Hjønnevåg 1 sibling, 0 replies; 11+ messages in thread From: Arve Hjønnevåg @ 2012-01-21 3:56 UTC (permalink / raw) To: linux-kernel Cc: Al Viro, Arve Hjønnevåg, Greg Kroah-Hartman, Christopher Lais, devel If user-space partially unmaps the driver, binder_vma_open would dump the kernel stack. This is not a kernel bug however and will be treated as if the whole area was unmapped once binder_vma_close gets called. Signed-off-by: Arve Hjønnevåg <arve@android.com> --- drivers/staging/android/binder.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 846b429..f0b7e66 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -2765,7 +2765,6 @@ static void binder_vma_open(struct vm_area_struct *vma) proc->pid, vma->vm_start, vma->vm_end, (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags, (unsigned long)pgprot_val(vma->vm_page_prot)); - dump_stack(); } static void binder_vma_close(struct vm_area_struct *vma) -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-02-02 6:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-21 3:56 [PATCH 0/2] Binder bug fixes Arve Hjønnevåg 2012-01-21 3:56 ` [PATCH 1/2] Staging: android: binder: Add some error checks Arve Hjønnevåg 2012-01-21 8:22 ` Dan Carpenter 2012-01-31 18:52 ` Greg KH 2012-01-31 23:20 ` Arve Hjønnevåg 2012-02-01 6:53 ` Dan Carpenter 2012-02-01 22:29 ` Arve Hjønnevåg 2012-02-01 22:47 ` Greg KH 2012-02-01 23:29 ` [PATCH] Staging: android: binder: Fix crashes when sharing a binder file between processes Arve Hjønnevåg 2012-02-02 6:27 ` [PATCH 1/2] Staging: android: binder: Add some error checks Dan Carpenter 2012-01-21 3:56 ` [PATCH 2/2] Staging: android: binder: Don't call dump_stack in binder_vma_open Arve Hjønnevåg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).