Re: [PATCH] staging: ion: make the pte default none PTE_RDONLY
On 2016/1/16 7:23, Russell King - ARM Linux wrote: > On Fri, Jan 15, 2016 at 03:03:42PM -0800, Laura Abbott wrote: >> (adding linux-arm and a few people) >> >> On 01/14/2016 06:42 PM, Chen Feng wrote: >>> The page is already alloc at ion_alloc function, >>> ion_mmap map the alloced pages to user-space. >>> >>> The default prot can be PTE_RDONLY. Take a look at >>> here: >>> set_pte_at() >>> arch/arm64/include/asm: >>> if (pte_dirty(pte) && pte_write(pte)) >>> pte_val(pte) &= ~PTE_RDONLY; >>> else >>> pte_val(pte) |= PTE_RDONLY; >>> >>> So with the dirty bit,it can improve the efficiency >>> and donnot need to handle memory fault when use access. >>> >>> Signed-off-by: Chen Feng >>> Signed-off-by: Wei Dong >>> Reviewed-by: Zhuangluan Su >>> --- >>> drivers/staging/android/ion/ion.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/staging/android/ion/ion.c >>> b/drivers/staging/android/ion/ion.c >>> index e237e9f..dba5942 100644 >>> --- a/drivers/staging/android/ion/ion.c >>> +++ b/drivers/staging/android/ion/ion.c >>> @@ -1026,6 +1026,9 @@ static int ion_mmap(struct dma_buf *dmabuf, struct >>> vm_area_struct *vma) >>> if (!(buffer->flags & ION_FLAG_CACHED)) >>> vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >>> >>> + /*Default writeable*/ >>> + vma->vm_page_prot = pte_mkdirty(vma->vm_page_prot); >>> + >>> mutex_lock(&buffer->lock); >>> /* now map it to userspace */ >>> ret = buffer->heap->ops->map_user(buffer->heap, buffer, vma); >>> >> >> The extra fault is unfortunate but I'm skeptical about just setting >> pte_mkdirty. >> >> Catalin/Will, do you have any thoughts? Right now it seems like any >> range mapped with remap_pfn_range will have this extra fault >> behavior. Is marking the range dirty the best solution? Laura Abbott, I agree with you, it seems all the remap_pfn_range have this fault behavior. > > What happens if the mapping requested was read only - at the very > least, I don't think this should be done unconditionally. > Russell, I am not sure about doing this unconditionally, but it can waste memory&time while handling page fault with ion alloced page. And the page can be used directly. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl
On 2016/1/19 15:33, Greg KH wrote: > On Tue, Jan 19, 2016 at 11:45:36AM +0800, Chen Feng wrote: >> When a process fork a child process, we should not allow the >> child process use the binder which opened by parent process. >> >> But if the binder-object creater is a thread of one process who exit, >> the other thread can also use this binder-object normally. >> We can distinguish this by the member proc->tsk->mm. >> If the thread exit the tsk->mm will be NULL. >> >> proc->tsk->mm != current->mm && proc->tsk->mm >> >> So only allow the shared mm_struct to use the same binder-object and >> check the existence of mm_struct. >> >> Signed-off-by: Chen Feng >> Signed-off-by: Wei Dong >> Signed-off-by: Junmin Zhao >> Reviewed-by: Zhuangluan Su >> --- >> drivers/android/binder.c | 2 ++ >> 1 file changed, 2 insertions(+) > > Why resend? What changed from the previous version? > My fault, it's my error commit. mistake the current->mm with current->tsk->mm. The robot-compile finds out this error. > greg k-h > > . > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl
On 2016/1/19 16:35, Greg KH wrote: > On Tue, Jan 19, 2016 at 03:49:27PM +0800, chenfeng wrote: >> >> >> On 2016/1/19 15:33, Greg KH wrote: >>> On Tue, Jan 19, 2016 at 11:45:36AM +0800, Chen Feng wrote: >>>> When a process fork a child process, we should not allow the >>>> child process use the binder which opened by parent process. >>>> >>>> But if the binder-object creater is a thread of one process who exit, >>>> the other thread can also use this binder-object normally. >>>> We can distinguish this by the member proc->tsk->mm. >>>> If the thread exit the tsk->mm will be NULL. >>>> >>>> proc->tsk->mm != current->mm && proc->tsk->mm >>>> >>>> So only allow the shared mm_struct to use the same binder-object and >>>> check the existence of mm_struct. >>>> >>>> Signed-off-by: Chen Feng >>>> Signed-off-by: Wei Dong >>>> Signed-off-by: Junmin Zhao >>>> Reviewed-by: Zhuangluan Su >>>> --- >>>> drivers/android/binder.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>> >>> Why resend? What changed from the previous version? >>> >> My fault, it's my error commit. >> mistake the current->mm with current->tsk->mm. >> The robot-compile finds out this error. > > Then please make it a 'v2' patch, and say what you changed, otherwise > I'll assume it's identical to the first patch you sent in. > > And how did you test the first patch if it couldn't even compile? > It works well on our platform with hundreds of mobile phone. Since our working branch is not mainline,and the patch is send for mainline review. I made a mistake while making the patch. I will send a new V2 for this patch. Thanks! > greg k-h > > . > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] android: binder: Sanity check at binder ioctl
Hi,
On 2016/1/20 6:40, David Rientjes wrote:
> On Tue, 19 Jan 2016, Chen Feng wrote:
>
>> When a process fork a child process, we should not allow the
>> child process use the binder which opened by parent process.
>>
>> But if the binder-object creater is a thread of one process who exit,
>> the other thread can also use this binder-object normally.
>> We can distinguish this by the member proc->tsk->mm.
>> If the thread exit the tsk->mm will be NULL.
>>
>
> Why does exit_mm(), the point where tsk->mm == NULL, signify the point at
> which the binder can now be used by other threads?
Yes,the other threads used this shared mm for communication. A lots of APPs
have this issue.
For example: system_server process use many libs,libstagefright,
Libdrmframeworketc,
In these lib, binder global object exist like static sp
sDeathNotifier,
static sp sDrmManagerService;
When system_sever process fork failed, the new child process call exit, which
will call
hook destruction function.
In destruction, the parent process *binder reference count will be wrong*. To
protect
these exception, we need to add the code in binder.
For v2 patch, I agree with your opinion, some race condition exist in some
special case.
I found that the task->mm already cached at binder_mm.
proc->vma_vm_mm = vma->vm_mm;
I will change the condition to if (unlikely(current->mm != proc->tsk->mm)).
But in some scenes, the thread do communication before the mmap called.
So I add proc->vma_vm_mm = current->mm where the binder_open function.
Since the binder init process in libbinder, the fix won't produce side effect.
If these are accepted, I will send a new V3 version.
>
>> proc->tsk->mm != current->mm && proc->tsk->mm
>>
>> So only allow the shared mm_struct to use the same binder-object and
>> check the existence of mm_struct.
>>
>> V2: Fix compile error for error commit
>>
>> Signed-off-by: Chen Feng
>> Signed-off-by: Wei Dong
>> Signed-off-by: Junmin Zhao
>> Reviewed-by: Zhuangluan Su
>> ---
>> drivers/android/binder.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index a39e85f..279063c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2736,6 +2736,8 @@ static long binder_ioctl(struct file *filp, unsigned
>> int cmd, unsigned long arg)
>>
>> /*pr_info("binder_ioctl: %d:%d %x %lx\n",
>> proc->pid, current->pid, cmd, arg);*/
>> +if (unlikely(proc->tsk->mm != current->mm && proc->tsk->mm))
>> +return -EINVAL;
>>
>> trace_binder_ioctl(cmd, arg);
>>
>
> I would imagine that you would want to do READ_ONCE(proc->tsk->mm) so you
> are guaranteed to be testing the same value.
>
> .
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: android: ion: Add ion driver for Hi6220 SoC platform
On 2015/10/9 16:58, Dan Carpenter wrote:
> On Fri, Oct 09, 2015 at 11:53:32AM +0300, Dan Carpenter wrote:
>>> +out:
>>
>> Labels named "out" are bug prone because handling everything is harder
>> than using named labels and unwinding one step at a time. The bug here
>> is that we don't call ion_device_destroy().
>>
>>> + for (i = 0; i < num_heaps; ++i)
>>> + ion_heap_destroy(heaps[i]);
>>> + return err;
>>
>> Write it like this:
>>
>> err_free_heaps:
>> for (i = 0; i < num_heaps; ++i)
>> ion_heap_destroy(heaps[i]);
>> err_free_idev:
>> ion_device_destroy(idev);
>>
>> return err;
>>
>>> +}
>>> +
>>> +static int hi6220_ion_remove(struct platform_device *pdev)
>>> +{
>>> + int i;
>>> +
>>> + ion_device_destroy(idev);
>>> + for (i = 0; i < num_heaps; i++) {
>>> + if (!heaps[i])
>>> + continue;
>>
>> We don't really need this NULL check and it isn't there in the
>> hi6220_ion_probe() unwind code.
>>
>>> + ion_heap_destroy(heaps[i]);
>>> + heaps[i] = NULL;
>>> + }
>>> +
>
> Really the unwind from probe() and the remove() function should have
> similar code. For example, is it important to set heaps[i] to NULL?
> If so then we should do it in the probe function as well. If not then
> we could leave it out of the remove function.
>
> Also the ion_device_destroy(idev) should be after freeing heaps in the
> remove function.
>
Thanks.
I will modify this next version.
> regards,
> dan carpenter
>
>
> .
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V1 2/3] taging: android: ion: Add ion driver for Hi6220 SoC platform
On 2015/10/10 23:00, Dan Carpenter wrote:
> On Sat, Oct 10, 2015 at 02:48:22PM +0800, Chen Feng wrote:
>> +static int hi6220_ion_probe(struct platform_device *pdev)
>> +{
>> +int i;
>> +int err = 0;
>> +static struct ion_platform_heap *p_heap;
>> +
>> +idev = ion_device_create(NULL);
>> +hi6220_set_platform_data(pdev);
>> +heaps = devm_kzalloc(&pdev->dev,
>> + sizeof(struct ion_heap *) * num_heaps,
>> + GFP_KERNEL);
>> +if (!heaps)
>> +return -ENOMEM;
>> +
>> +/*
>> + * create the heaps as specified in the dts file
>> + */
>> +for (i = 0; i < num_heaps; i++) {
>> +p_heap = heaps_data[i];
>> +heaps[i] = ion_heap_create(p_heap);
>> +if (IS_ERR_OR_NULL(heaps[i])) {
>> +err = PTR_ERR(heaps[i]);
>> +goto err_free_heaps;
>> +}
>> +
>> +ion_device_add_heap(idev, heaps[i]);
>> +
>> +pr_info("%s: adding heap %s of type %d with %lx@%lx\n",
>> +__func__, p_heap->name, p_heap->type,
>> +p_heap->base, (unsigned long)p_heap->size);
>> +}
>> +return err;
>> +
>> +err_free_heaps:
>> +ion_device_destroy(idev);
>> +for (i = 0; i < num_heaps; ++i) {
>> +ion_heap_destroy(heaps[i]);
>> +heaps[i] = NULL;
>> +}
>> +
>> +return err;
>> +}
>
> Thanks this is better but still not quite right. You have to unwind in
> the reverse order from how you allocated things.
>
> err_free_heaps:
> for (i = 0; i < num_heaps; ++i) {
> ion_heap_destroy(heaps[i]);
> heaps[i] = NULL;
> }
> err_destroy_idev:
> ion_device_destroy(idev);
>
> return err;
>
> And earlier it should be:
>
> idev = ion_device_create(NULL);
> hi6220_set_platform_data(pdev);
> heaps = devm_kzalloc(&pdev->dev,
>sizeof(struct ion_heap *) * num_heaps,
>GFP_KERNEL);
> - if (!heaps)
> - return -ENOMEM;
> + if (!heaps) {
> + err = -ENOMEM;
> + goto err_destroy_idev;
> + }
>
> Otherwise we leak some resources if we can't allocate "heaps".
>
Yeah,it's right. I will fix this.
> regards,
> dan carpenter
>
>
> .
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
