On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hj?nnev?g wrote:
> 2012/1/31 Greg KH <[email protected]>:
> > 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
2012/1/31 Dan Carpenter <[email protected]>:
> On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hj?nnev?g wrote:
>> 2012/1/31 Greg KH <[email protected]>:
>> > 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
On Wed, Feb 01, 2012 at 02:29:36PM -0800, Arve Hj?nnev?g wrote:
> 2012/1/31 Dan Carpenter <[email protected]>:
> > On Tue, Jan 31, 2012 at 03:20:30PM -0800, Arve Hj?nnev?g wrote:
> >> 2012/1/31 Greg KH <[email protected]>:
> >> > 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
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 <[email protected]>
---
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
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 <[email protected]>
regards,
dan carpenter