Re: [RFC PATCH 2/3] firmware: Add support for PSA FF-A transport for VM partitions
On Mon, Jun 1, 2020 at 2:45 AM Sudeep Holla wrote:
>
> Initial support for PSA FF-A interface providing APIs for non-secure VM
> partitions.
>
...
> diff --git a/drivers/firmware/arm_psa_ffa/Kconfig
> b/drivers/firmware/arm_psa_ffa/Kconfig
> new file mode 100644
> index ..ba699ec68ec4
> --- /dev/null
> +++ b/drivers/firmware/arm_psa_ffa/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config ARM_PSA_FFA_TRANSPORT
> + tristate "Arm Platform Security Architecture Firmware Framework for
> Armv8-A"
> + depends on ARM64 && HAVE_ARM_SMCCC_DISCOVERY
Most of this driver should be usable on any platform, so it would be
better to only depend on ARM64 in the component that has the arm64
specific implementation of your low level conduit.
...
> diff --git a/drivers/firmware/arm_psa_ffa/driver.c
> b/drivers/firmware/arm_psa_ffa/driver.c
> new file mode 100644
> index ..700bd5850746
> --- /dev/null
> +++ b/drivers/firmware/arm_psa_ffa/driver.c
...
> +
> +typedef struct arm_smccc_res
> +(arm_psa_ffa_fn)(unsigned long, unsigned long, unsigned long, unsigned long,
> +unsigned long, unsigned long, unsigned long, unsigned long);
> +static arm_psa_ffa_fn *invoke_arm_psa_ffa_fn;
> +
...
> +struct arm_smccc_res
> +__arm_psa_ffa_fn_smc(unsigned long function_id,unsigned long arg0,
...
> +struct arm_smccc_res
> +__arm_psa_ffa_fn_hvc(unsigned long function_id,unsigned long arg0,
Can these two functions move out of this file so this driver only
depends on a function matching the arm_psa_ffa_fn type?
...
> +static int psa_ffa_probe(struct platform_device *pdev)
> +{
> + int ret;
> + enum arm_smccc_conduit conduit;
> +
> + if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> + return 0;
> +
> + conduit = arm_smccc_1_1_get_conduit();
If you make this device a child device of the conduit, then you don't
need this here. Other conduits can be added to for instance support
other architectures without adding entries to this enum and modifying
this driver.
--
Arve Hjønnevåg
Re: [PATCH v2] ANDROID: binder: change down_write to down_read
On Wed, Mar 28, 2018 at 6:00 PM, Minchan Kim <minc...@kernel.org> wrote:
> binder_update_page_range needs down_write of mmap_sem because
> vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless
> it is set. However, when I profile binder working, it seems
> every binder buffers should be mapped in advance by binder_mmap.
> It means we could set VM_MIXEDMAP in bider_mmap time which is
> already hold a mmap_sem as down_write so binder_update_page_range
> doesn't need to hold a mmap_sem as down_write.
>
> Android suffers from mmap_sem contention so let's reduce mmap_sem
> down_write.
>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Reviewed-by: Martijn Coenen <m...@android.com>
> Signed-off-by: Minchan Kim <minc...@kernel.org>
> ---
> From v1:
> * remove WARN_ON_ONCE - Greg
> * add reviewed-by - Martijn
>
> Martijn, I took your LGTM of v1 as Reviewed-by. If you don't like it
> or want to change it to acked-by, please, tell me.
>
> drivers/android/binder.c | 2 +-
> drivers/android/binder_alloc.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 764b63a5aade..9a14c6dd60c4 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -4722,7 +4722,7 @@ static int binder_mmap(struct file *filp, struct
> vm_area_struct *vma)
> failure_string = "bad vm_flags";
> goto err_bad_arg;
> }
> - vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
> + vma->vm_flags |= (VM_DONTCOPY | VM_MIXEDMAP) & ~VM_MAYWRITE;
Why did you change this to |=? You are no longer clearing VM_MAYWRITE.
> vma->vm_ops = _vm_ops;
> vma->vm_private_data = proc;
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 5a426c877dfb..4f382d51def1 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> mm = alloc->vma_vm_mm;
>
> if (mm) {
> - down_write(>mmap_sem);
> + down_read(>mmap_sem);
> vma = alloc->vma;
> }
>
> @@ -288,7 +288,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> /* vm_insert_page does not seem to increment the refcount */
> }
> if (mm) {
> - up_write(>mmap_sem);
> + up_read(>mmap_sem);
> mmput(mm);
> }
> return 0;
> @@ -321,7 +321,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> }
> err_no_vma:
> if (mm) {
> - up_write(>mmap_sem);
> + up_read(>mmap_sem);
> mmput(mm);
> }
> return vma ? -ENOMEM : -ESRCH;
> --
> 2.17.0.rc1.321.gba9d0f2565-goog
>
--
Arve Hjønnevåg
Re: [PATCH v2] ANDROID: binder: change down_write to down_read
On Wed, Mar 28, 2018 at 6:00 PM, Minchan Kim wrote:
> binder_update_page_range needs down_write of mmap_sem because
> vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless
> it is set. However, when I profile binder working, it seems
> every binder buffers should be mapped in advance by binder_mmap.
> It means we could set VM_MIXEDMAP in bider_mmap time which is
> already hold a mmap_sem as down_write so binder_update_page_range
> doesn't need to hold a mmap_sem as down_write.
>
> Android suffers from mmap_sem contention so let's reduce mmap_sem
> down_write.
>
> Cc: Todd Kjos
> Cc: Greg Kroah-Hartman
> Reviewed-by: Martijn Coenen
> Signed-off-by: Minchan Kim
> ---
> From v1:
> * remove WARN_ON_ONCE - Greg
> * add reviewed-by - Martijn
>
> Martijn, I took your LGTM of v1 as Reviewed-by. If you don't like it
> or want to change it to acked-by, please, tell me.
>
> drivers/android/binder.c | 2 +-
> drivers/android/binder_alloc.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 764b63a5aade..9a14c6dd60c4 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -4722,7 +4722,7 @@ static int binder_mmap(struct file *filp, struct
> vm_area_struct *vma)
> failure_string = "bad vm_flags";
> goto err_bad_arg;
> }
> - vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
> + vma->vm_flags |= (VM_DONTCOPY | VM_MIXEDMAP) & ~VM_MAYWRITE;
Why did you change this to |=? You are no longer clearing VM_MAYWRITE.
> vma->vm_ops = _vm_ops;
> vma->vm_private_data = proc;
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 5a426c877dfb..4f382d51def1 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> mm = alloc->vma_vm_mm;
>
> if (mm) {
> - down_write(>mmap_sem);
> + down_read(>mmap_sem);
> vma = alloc->vma;
> }
>
> @@ -288,7 +288,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> /* vm_insert_page does not seem to increment the refcount */
> }
> if (mm) {
> - up_write(>mmap_sem);
> + up_read(>mmap_sem);
> mmput(mm);
> }
> return 0;
> @@ -321,7 +321,7 @@ static int binder_update_page_range(struct binder_alloc
> *alloc, int allocate,
> }
> err_no_vma:
> if (mm) {
> - up_write(>mmap_sem);
> + up_read(>mmap_sem);
> mmput(mm);
> }
> return vma ? -ENOMEM : -ESRCH;
> --
> 2.17.0.rc1.321.gba9d0f2565-goog
>
--
Arve Hjønnevåg
Re: [PATCH v3] android: binder: use VM_ALLOC to get vm area
On Mon, Jan 22, 2018 at 9:02 AM, Todd Kjos <tk...@google.com> wrote:
> On Mon, Jan 22, 2018 at 7:54 AM, Greg KH <gre...@linuxfoundation.org> wrote:
>> On Wed, Jan 10, 2018 at 10:49:05AM +0800, Ganesh Mahendran wrote:
>>> VM_IOREMAP is used to access hardware through a mechanism called
>>> I/O mapped memory. Android binder is a IPC machanism which will
>>> not access I/O memory.
>>>
>>> And VM_IOREMAP has alignment requiement which may not needed in
>>> binder.
>>> __get_vm_area_node()
>>> {
>>> ...
>>> if (flags & VM_IOREMAP)
>>> align = 1ul << clamp_t(int, fls_long(size),
>>>PAGE_SHIFT, IOREMAP_MAX_ORDER);
>>> ...
>>> }
>>>
>>> This patch will save some kernel vm area, especially for 32bit os.
>>>
>>> In 32bit OS, kernel vm area is only 240MB. We may got below
>>> error when launching a app:
>>>
>>> <3>[ 4482.440053] binder_alloc: binder_alloc_mmap_handler: 15728
>>> 8ce67000-8cf65000 get_vm_area failed -12
>>> <3>[ 4483.218817] binder_alloc: binder_alloc_mmap_handler: 15745
>>> 8ce67000-8cf65000 get_vm_area failed -12
>>>
>>> Signed-off-by: Ganesh Mahendran <opensource.gan...@gmail.com>
>>>
>>> V3: update comments
>>> V2: update comments
>>> ---
>>> drivers/android/binder_alloc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Martijn and Todd, any objections to this patch?
>
> Looks fine to me. Arve, do you remember the rationale for using VM_IOREMAP?
>
I don't remember for sure, but I think it used alloc_vm_area at some
point, and that uses VM_IOREMAP.
--
Arve Hjønnevåg
Re: [PATCH v3] android: binder: use VM_ALLOC to get vm area
On Mon, Jan 22, 2018 at 9:02 AM, Todd Kjos wrote:
> On Mon, Jan 22, 2018 at 7:54 AM, Greg KH wrote:
>> On Wed, Jan 10, 2018 at 10:49:05AM +0800, Ganesh Mahendran wrote:
>>> VM_IOREMAP is used to access hardware through a mechanism called
>>> I/O mapped memory. Android binder is a IPC machanism which will
>>> not access I/O memory.
>>>
>>> And VM_IOREMAP has alignment requiement which may not needed in
>>> binder.
>>> __get_vm_area_node()
>>> {
>>> ...
>>> if (flags & VM_IOREMAP)
>>> align = 1ul << clamp_t(int, fls_long(size),
>>>PAGE_SHIFT, IOREMAP_MAX_ORDER);
>>> ...
>>> }
>>>
>>> This patch will save some kernel vm area, especially for 32bit os.
>>>
>>> In 32bit OS, kernel vm area is only 240MB. We may got below
>>> error when launching a app:
>>>
>>> <3>[ 4482.440053] binder_alloc: binder_alloc_mmap_handler: 15728
>>> 8ce67000-8cf65000 get_vm_area failed -12
>>> <3>[ 4483.218817] binder_alloc: binder_alloc_mmap_handler: 15745
>>> 8ce67000-8cf65000 get_vm_area failed -12
>>>
>>> Signed-off-by: Ganesh Mahendran
>>>
>>> V3: update comments
>>> V2: update comments
>>> ---
>>> drivers/android/binder_alloc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Martijn and Todd, any objections to this patch?
>
> Looks fine to me. Arve, do you remember the rationale for using VM_IOREMAP?
>
I don't remember for sure, but I think it used alloc_vm_area at some
point, and that uses VM_IOREMAP.
--
Arve Hjønnevåg
Re: [PATCH v2 1/4] android: binder: Don't get mm from task
On Tue, Oct 24, 2017 at 12:28 AM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Mon, Oct 23, 2017 at 11:18:52AM -0700, Arve Hjønnevåg wrote: >> On Sat, Oct 21, 2017 at 1:15 AM, Greg Kroah-Hartman >> <gre...@linuxfoundation.org> wrote: >> > On Fri, Oct 20, 2017 at 08:58:58PM -0400, Sherry Yang wrote: >> >> Use binder_alloc struct's mm_struct rather than getting >> >> a reference to the mm struct through get_task_mm to >> >> avoid a potential deadlock between lru lock, task lock and >> >> dentry lock, since a thread can be holding the task lock >> >> and the dentry lock while trying to acquire the lru lock. >> >> >> >> Acked-by: Arve Hjønnevåg <a...@android.com> >> >> Signed-off-by: Sherry Yang <sher...@android.com> >> >> --- >> >> drivers/android/binder_alloc.c | 22 +- >> >> drivers/android/binder_alloc.h | 1 - >> >> 2 files changed, 9 insertions(+), 14 deletions(-) >> > >> > I've applied these first 2 patches, but patches 3 and 4 I have already >> > applied to my char-misc-next tree, right? >> > >> > thanks, >> > >> > greg k-h >> >> I would expect you got a merge conflict from one of those. Using patch >> 3 and 4 in from this patchset should avoid that conflict if your >> eventual 4.15 branch is not based on your current char-misc-next >> branch. > > I've resolved the merge conflict so my char-misc-next branch should be > all caught up now. It would be wonderful if you could verify this. > > thanks, > > greg k-h I have not tested your branch directly, but the relevant code in char-misc-next is now identical to the code I tested. -- Arve Hjønnevåg
Re: [PATCH v2 1/4] android: binder: Don't get mm from task
On Tue, Oct 24, 2017 at 12:28 AM, Greg Kroah-Hartman wrote: > On Mon, Oct 23, 2017 at 11:18:52AM -0700, Arve Hjønnevåg wrote: >> On Sat, Oct 21, 2017 at 1:15 AM, Greg Kroah-Hartman >> wrote: >> > On Fri, Oct 20, 2017 at 08:58:58PM -0400, Sherry Yang wrote: >> >> Use binder_alloc struct's mm_struct rather than getting >> >> a reference to the mm struct through get_task_mm to >> >> avoid a potential deadlock between lru lock, task lock and >> >> dentry lock, since a thread can be holding the task lock >> >> and the dentry lock while trying to acquire the lru lock. >> >> >> >> Acked-by: Arve Hjønnevåg >> >> Signed-off-by: Sherry Yang >> >> --- >> >> drivers/android/binder_alloc.c | 22 +- >> >> drivers/android/binder_alloc.h | 1 - >> >> 2 files changed, 9 insertions(+), 14 deletions(-) >> > >> > I've applied these first 2 patches, but patches 3 and 4 I have already >> > applied to my char-misc-next tree, right? >> > >> > thanks, >> > >> > greg k-h >> >> I would expect you got a merge conflict from one of those. Using patch >> 3 and 4 in from this patchset should avoid that conflict if your >> eventual 4.15 branch is not based on your current char-misc-next >> branch. > > I've resolved the merge conflict so my char-misc-next branch should be > all caught up now. It would be wonderful if you could verify this. > > thanks, > > greg k-h I have not tested your branch directly, but the relevant code in char-misc-next is now identical to the code I tested. -- Arve Hjønnevåg
Re: [PATCH v2 1/4] android: binder: Don't get mm from task
On Sat, Oct 21, 2017 at 1:15 AM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Fri, Oct 20, 2017 at 08:58:58PM -0400, Sherry Yang wrote: >> Use binder_alloc struct's mm_struct rather than getting >> a reference to the mm struct through get_task_mm to >> avoid a potential deadlock between lru lock, task lock and >> dentry lock, since a thread can be holding the task lock >> and the dentry lock while trying to acquire the lru lock. >> >> Acked-by: Arve Hjønnevåg <a...@android.com> >> Signed-off-by: Sherry Yang <sher...@android.com> >> --- >> drivers/android/binder_alloc.c | 22 +- >> drivers/android/binder_alloc.h | 1 - >> 2 files changed, 9 insertions(+), 14 deletions(-) > > I've applied these first 2 patches, but patches 3 and 4 I have already > applied to my char-misc-next tree, right? > > thanks, > > greg k-h I would expect you got a merge conflict from one of those. Using patch 3 and 4 in from this patchset should avoid that conflict if your eventual 4.15 branch is not based on your current char-misc-next branch. -- Arve Hjønnevåg
Re: [PATCH v2 1/4] android: binder: Don't get mm from task
On Sat, Oct 21, 2017 at 1:15 AM, Greg Kroah-Hartman wrote: > On Fri, Oct 20, 2017 at 08:58:58PM -0400, Sherry Yang wrote: >> Use binder_alloc struct's mm_struct rather than getting >> a reference to the mm struct through get_task_mm to >> avoid a potential deadlock between lru lock, task lock and >> dentry lock, since a thread can be holding the task lock >> and the dentry lock while trying to acquire the lru lock. >> >> Acked-by: Arve Hjønnevåg >> Signed-off-by: Sherry Yang >> --- >> drivers/android/binder_alloc.c | 22 +- >> drivers/android/binder_alloc.h | 1 - >> 2 files changed, 9 insertions(+), 14 deletions(-) > > I've applied these first 2 patches, but patches 3 and 4 I have already > applied to my char-misc-next tree, right? > > thanks, > > greg k-h I would expect you got a merge conflict from one of those. Using patch 3 and 4 in from this patchset should avoid that conflict if your eventual 4.15 branch is not based on your current char-misc-next branch. -- Arve Hjønnevåg
Re: [PATCH] mm: Restore mmput_async
On Wed, Sep 13, 2017 at 3:57 PM, Andrew Morton
<a...@linux-foundation.org> wrote:
> On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <sher...@android.com> wrote:
>
>> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
>> <a...@linux-foundation.org> wrote:
>> > On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <sher...@android.com> wrote:
>> >
>> >> Restore asynchronous mmput, allowing mmput_async to be called
>> >> from an atomic context in Android binder shrinker callback.
>> >>
>> >> mmput_async was initially introduced in ec8d7c14e
>> >> ("mm, oom_reaper: do not mmput synchronously from the
>> >> oom reaper context"), and was removed in 212925802
>> >> ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
>> >
>> > Presumably there's a patch somewhere which adds a call to mmput_async()
>> > into drivers/android/binder.c? Where is that patch?
>>
>> The patch that uses mmput_async() is
>> https://lkml.org/lkml/2017/9/8/785. Gmail doesn't seem to respect
>> in-reply-to.
>
> (Top-posting repaired. Please don't!)
>
> Is it necessary for binder_alloc_free_page() to take a ref on the mm?
> As long as alloc->tsk doesn't exit during binder_alloc_free_page()'s
> execution, that task's reference on the mm should be sufficient to keep
> the mm alive?
>
alloc->tsk can exit during binder_alloc_free_page. We don't hold a
reference to the task's mm struct while we don't actively use it, as
this would prevent the driver from getting closed when a process dies.
--
Arve Hjønnevåg
Re: [PATCH] mm: Restore mmput_async
On Wed, Sep 13, 2017 at 3:57 PM, Andrew Morton
wrote:
> On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang wrote:
>
>> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
>> wrote:
>> > On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang wrote:
>> >
>> >> Restore asynchronous mmput, allowing mmput_async to be called
>> >> from an atomic context in Android binder shrinker callback.
>> >>
>> >> mmput_async was initially introduced in ec8d7c14e
>> >> ("mm, oom_reaper: do not mmput synchronously from the
>> >> oom reaper context"), and was removed in 212925802
>> >> ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
>> >
>> > Presumably there's a patch somewhere which adds a call to mmput_async()
>> > into drivers/android/binder.c? Where is that patch?
>>
>> The patch that uses mmput_async() is
>> https://lkml.org/lkml/2017/9/8/785. Gmail doesn't seem to respect
>> in-reply-to.
>
> (Top-posting repaired. Please don't!)
>
> Is it necessary for binder_alloc_free_page() to take a ref on the mm?
> As long as alloc->tsk doesn't exit during binder_alloc_free_page()'s
> execution, that task's reference on the mm should be sufficient to keep
> the mm alive?
>
alloc->tsk can exit during binder_alloc_free_page. We don't hold a
reference to the task's mm struct while we don't actively use it, as
this would prevent the driver from getting closed when a process dies.
--
Arve Hjønnevåg
Re: [PATCH v3 3/6] android: binder: Move buffer out of area shared with user space
On Wed, Aug 30, 2017 at 2:29 AM, Dan Carpenter <dan.carpen...@oracle.com> wrote: > On Tue, Aug 29, 2017 at 05:46:59PM -0700, Sherry Yang wrote: >> Binder driver allocates buffer meta data in a region that is mapped >> in user space. These meta data contain pointers in the kernel. >> >> This patch allocates buffer meta data on the kernel heap that is >> not mapped in user space, and uses a pointer to refer to the data mapped. >> >> Also move alloc->buffers initialization from mmap to init since it's >> now used even when mmap failed or was not called. >> >> Signed-off-by: Sherry Yang <sher...@android.com> >> --- > > The difference between v2 and v3 is that we've shifted some > initialization around to fix the crashing bug that kbuild found. You > should not that difference here under the --- cut off. > >> drivers/android/binder_alloc.c | 146 >> +++- >> drivers/android/binder_alloc.h | 2 +- >> drivers/android/binder_alloc_selftest.c | 11 ++- >> 3 files changed, 91 insertions(+), 68 deletions(-) > > But really we still need to have some answers or discussion about the > questions that Greg and I raised. Greg asked if the other Android devs > had Acked this. Please ping Arve to Ack this. > tk...@google.com replied and ack'ed v2. The changes have been reviewed on android-review.googlesource.com. Do you want and ack or review tag included in the patchset or do you want separate ack emails on each patchset (or on each patch)? > I was curious about the security impact or why we were writing this > patch 3/6. It seems we are fixing an information disclosure bug. Or is > it something worse than that? Or have I misunderstood entirely. > > We probably original put the buffers in userspace for accounting reasons > so we could kill programs that used too much RAM. This patch doesn't > create a problem with that hopefully? We're just moving the metadata to > kernel space? > The buffer headers have never been used by user-space. They are readable by user-space because the content after the header has to be readable from user-space (and only whole pages can be mapped). It was simpler to have the header in the same shared region as well. At the time this code was written the content of kernel pointers where not considered secret and available elsewhere anyway (e.g. kernel log, /proc/kallsyms). -- Arve Hjønnevåg
Re: [PATCH v3 3/6] android: binder: Move buffer out of area shared with user space
On Wed, Aug 30, 2017 at 2:29 AM, Dan Carpenter wrote: > On Tue, Aug 29, 2017 at 05:46:59PM -0700, Sherry Yang wrote: >> Binder driver allocates buffer meta data in a region that is mapped >> in user space. These meta data contain pointers in the kernel. >> >> This patch allocates buffer meta data on the kernel heap that is >> not mapped in user space, and uses a pointer to refer to the data mapped. >> >> Also move alloc->buffers initialization from mmap to init since it's >> now used even when mmap failed or was not called. >> >> Signed-off-by: Sherry Yang >> --- > > The difference between v2 and v3 is that we've shifted some > initialization around to fix the crashing bug that kbuild found. You > should not that difference here under the --- cut off. > >> drivers/android/binder_alloc.c | 146 >> +++- >> drivers/android/binder_alloc.h | 2 +- >> drivers/android/binder_alloc_selftest.c | 11 ++- >> 3 files changed, 91 insertions(+), 68 deletions(-) > > But really we still need to have some answers or discussion about the > questions that Greg and I raised. Greg asked if the other Android devs > had Acked this. Please ping Arve to Ack this. > tk...@google.com replied and ack'ed v2. The changes have been reviewed on android-review.googlesource.com. Do you want and ack or review tag included in the patchset or do you want separate ack emails on each patchset (or on each patch)? > I was curious about the security impact or why we were writing this > patch 3/6. It seems we are fixing an information disclosure bug. Or is > it something worse than that? Or have I misunderstood entirely. > > We probably original put the buffers in userspace for accounting reasons > so we could kill programs that used too much RAM. This patch doesn't > create a problem with that hopefully? We're just moving the metadata to > kernel space? > The buffer headers have never been used by user-space. They are readable by user-space because the content after the header has to be readable from user-space (and only whole pages can be mapped). It was simpler to have the header in the same shared region as well. At the time this code was written the content of kernel pointers where not considered secret and available elsewhere anyway (e.g. kernel log, /proc/kallsyms). -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <pet...@infradead.org> wrote: > On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote: > >> A previous attempt to fix this problem, changed the lock to use >> rt_mutex instead of mutex, but this apparently did not work as well as >> this patch. I believe the added overhead was noticeable, and it did >> not work when the preempted thread was in a different cgroup (I don't >> know if this is still the case). > > Do you actually have RR/FIFO/DL tasks? Currently PI isn't > defined/implemented for OTHER. > Most of the tasks here are not RR/FIFO/DL tasks. I don't see anything in the rtmutex code or documentation that indicates that they don't work for normal tasks. From what I can tell the priority gets boosted in every case. This may not work as well for CFS tasks as for realtime tasks, but it should at least help when there is a large priority difference. > cgroups should be irrelevant, PI is unaware of them. I don't think cgroups are irrelevant. PI being unaware of them explains the problem I described. If the task that holds the lock is in a cgroup that has a low cpu.shares value, then boosting the task's priority within that group does necessarily make it any more likely to run. -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra wrote: > On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote: > >> A previous attempt to fix this problem, changed the lock to use >> rt_mutex instead of mutex, but this apparently did not work as well as >> this patch. I believe the added overhead was noticeable, and it did >> not work when the preempted thread was in a different cgroup (I don't >> know if this is still the case). > > Do you actually have RR/FIFO/DL tasks? Currently PI isn't > defined/implemented for OTHER. > Most of the tasks here are not RR/FIFO/DL tasks. I don't see anything in the rtmutex code or documentation that indicates that they don't work for normal tasks. From what I can tell the priority gets boosted in every case. This may not work as well for CFS tasks as for realtime tasks, but it should at least help when there is a large priority difference. > cgroups should be irrelevant, PI is unaware of them. I don't think cgroups are irrelevant. PI being unaware of them explains the problem I described. If the task that holds the lock is in a cgroup that has a low cpu.shares value, then boosting the task's priority within that group does necessarily make it any more likely to run. -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Mon, Sep 12, 2016 at 11:42 PM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote: >> On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman >> <gre...@linuxfoundation.org> wrote: >> > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote: >> >> On Sat, 10 Sep 2016, Peter Zijlstra wrote: >> >> >> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote: >> >> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote: >> >> > > > In Android systems, the display pipeline relies on low >> >> > > > latency binder transactions and is therefore sensitive to >> >> > > > delays caused by contention for the global binder lock. >> >> > > > Jank is siginificantly reduced by disabling preemption >> >> > > > while the global binder lock is held. >> >> > > >> >> > > That's now how preempt_disable is supposed to use. It is for critical >> >> > >> >> > not, that's supposed to be _not_. Just to be absolutely clear, this is >> >> > NOT how you're supposed to use preempt_disable(). >> >> > >> >> > > sections that use per-cpu or similar resources. >> >> > > >> >> > > > >> >> > > > Originally-from: Riley Andrews <riandr...@google.com> >> >> > > > Signed-off-by: Todd Kjos <tk...@google.com> >> >> > >> >> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct >> >> > > > binder_proc *proc, int flags) >> >> > > > rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); >> >> > > > unlock_task_sighand(proc->tsk, ); >> >> > > > >> >> > > > - return __alloc_fd(files, 0, rlim_cur, flags); >> >> > > > + preempt_enable_no_resched(); >> >> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags); >> >> > > > + preempt_disable(); >> >> > >> >> > And the fact that people want to use preempt_enable_no_resched() shows >> >> > that they're absolutely clueless. >> >> > >> >> > This is so broken its not funny. >> >> > >> >> > NAK NAK NAK >> >> >> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place >> >> documents clearly that this is tinkering and not proper software >> >> engineering. >> > >> > I have pointed out in the other thread for this patch (the one that had >> > a patch that could be applied) that the single lock in the binder code >> > is the main problem here, it should be solved instead of this messing >> > around with priorities. >> > >> >> While removing the single lock in the binder driver would help reduce >> the problem that this patch tries to work around, it would not fix it. >> The largest problems occur when a very low priority thread gets >> preempted while holding the lock. When a high priority thread then >> needs the same lock it can't get it. Changing the driver to use more >> fine-grained locking would reduce the set of threads that can trigger >> this problem, but there are processes that receive work from both high >> and low priority threads and could still end up in the same situation. > > Ok, but how would this patch solve the problem? It only reduces the > window of when the preemption could occur, it doesn't stop it from > happening entirely. > I agree, this patch does not _solve_ the problem either. It only reduces it, as there are many places where it re-enables preemption for significant work. >> A previous attempt to fix this problem, changed the lock to use >> rt_mutex instead of mutex, but this apparently did not work as well as >> this patch. I believe the added overhead was noticeable, and it did >> not work when the preempted thread was in a different cgroup (I don't >> know if this is still the case). > > Why would rt_mutex solve this? If the task that holds the lock would run when you try to get the lock, there would be no long delay to get the lock. rt_mutex seems to be designed to solve this problem. > > I worry that this is only going to get worse as more portions of the > Android userspace/HAL get rewritten to use binder more and more. What > about trying to get rid of the lock entirely? That way the task > priority levels would work "properly" here. > I think a good first step would be to switch to locks with a smaller scope. I'm not how useful it would be to eliminate locks in this driver since it calls into other code that uses locks. > thanks, > > greg k-h -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Mon, Sep 12, 2016 at 11:42 PM, Greg Kroah-Hartman wrote: > On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote: >> On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman >> wrote: >> > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote: >> >> On Sat, 10 Sep 2016, Peter Zijlstra wrote: >> >> >> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote: >> >> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote: >> >> > > > In Android systems, the display pipeline relies on low >> >> > > > latency binder transactions and is therefore sensitive to >> >> > > > delays caused by contention for the global binder lock. >> >> > > > Jank is siginificantly reduced by disabling preemption >> >> > > > while the global binder lock is held. >> >> > > >> >> > > That's now how preempt_disable is supposed to use. It is for critical >> >> > >> >> > not, that's supposed to be _not_. Just to be absolutely clear, this is >> >> > NOT how you're supposed to use preempt_disable(). >> >> > >> >> > > sections that use per-cpu or similar resources. >> >> > > >> >> > > > >> >> > > > Originally-from: Riley Andrews >> >> > > > Signed-off-by: Todd Kjos >> >> > >> >> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct >> >> > > > binder_proc *proc, int flags) >> >> > > > rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); >> >> > > > unlock_task_sighand(proc->tsk, ); >> >> > > > >> >> > > > - return __alloc_fd(files, 0, rlim_cur, flags); >> >> > > > + preempt_enable_no_resched(); >> >> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags); >> >> > > > + preempt_disable(); >> >> > >> >> > And the fact that people want to use preempt_enable_no_resched() shows >> >> > that they're absolutely clueless. >> >> > >> >> > This is so broken its not funny. >> >> > >> >> > NAK NAK NAK >> >> >> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place >> >> documents clearly that this is tinkering and not proper software >> >> engineering. >> > >> > I have pointed out in the other thread for this patch (the one that had >> > a patch that could be applied) that the single lock in the binder code >> > is the main problem here, it should be solved instead of this messing >> > around with priorities. >> > >> >> While removing the single lock in the binder driver would help reduce >> the problem that this patch tries to work around, it would not fix it. >> The largest problems occur when a very low priority thread gets >> preempted while holding the lock. When a high priority thread then >> needs the same lock it can't get it. Changing the driver to use more >> fine-grained locking would reduce the set of threads that can trigger >> this problem, but there are processes that receive work from both high >> and low priority threads and could still end up in the same situation. > > Ok, but how would this patch solve the problem? It only reduces the > window of when the preemption could occur, it doesn't stop it from > happening entirely. > I agree, this patch does not _solve_ the problem either. It only reduces it, as there are many places where it re-enables preemption for significant work. >> A previous attempt to fix this problem, changed the lock to use >> rt_mutex instead of mutex, but this apparently did not work as well as >> this patch. I believe the added overhead was noticeable, and it did >> not work when the preempted thread was in a different cgroup (I don't >> know if this is still the case). > > Why would rt_mutex solve this? If the task that holds the lock would run when you try to get the lock, there would be no long delay to get the lock. rt_mutex seems to be designed to solve this problem. > > I worry that this is only going to get worse as more portions of the > Android userspace/HAL get rewritten to use binder more and more. What > about trying to get rid of the lock entirely? That way the task > priority levels would work "properly" here. > I think a good first step would be to switch to locks with a smaller scope. I'm not how useful it would be to eliminate locks in this driver since it calls into other code that uses locks. > thanks, > > greg k-h -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote: >> On Sat, 10 Sep 2016, Peter Zijlstra wrote: >> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote: >> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote: >> > > > In Android systems, the display pipeline relies on low >> > > > latency binder transactions and is therefore sensitive to >> > > > delays caused by contention for the global binder lock. >> > > > Jank is siginificantly reduced by disabling preemption >> > > > while the global binder lock is held. >> > > >> > > That's now how preempt_disable is supposed to use. It is for critical >> > >> > not, that's supposed to be _not_. Just to be absolutely clear, this is >> > NOT how you're supposed to use preempt_disable(). >> > >> > > sections that use per-cpu or similar resources. >> > > >> > > > >> > > > Originally-from: Riley Andrews <riandr...@google.com> >> > > > Signed-off-by: Todd Kjos <tk...@google.com> >> > >> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct >> > > > binder_proc *proc, int flags) >> > > > rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); >> > > > unlock_task_sighand(proc->tsk, ); >> > > > >> > > > - return __alloc_fd(files, 0, rlim_cur, flags); >> > > > + preempt_enable_no_resched(); >> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags); >> > > > + preempt_disable(); >> > >> > And the fact that people want to use preempt_enable_no_resched() shows >> > that they're absolutely clueless. >> > >> > This is so broken its not funny. >> > >> > NAK NAK NAK >> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place >> documents clearly that this is tinkering and not proper software >> engineering. > > I have pointed out in the other thread for this patch (the one that had > a patch that could be applied) that the single lock in the binder code > is the main problem here, it should be solved instead of this messing > around with priorities. > While removing the single lock in the binder driver would help reduce the problem that this patch tries to work around, it would not fix it. The largest problems occur when a very low priority thread gets preempted while holding the lock. When a high priority thread then needs the same lock it can't get it. Changing the driver to use more fine-grained locking would reduce the set of threads that can trigger this problem, but there are processes that receive work from both high and low priority threads and could still end up in the same situation. A previous attempt to fix this problem, changed the lock to use rt_mutex instead of mutex, but this apparently did not work as well as this patch. I believe the added overhead was noticeable, and it did not work when the preempted thread was in a different cgroup (I don't know if this is still the case). It would be useful to generic solution to this problem. > So don't worry, I'm not taking this change :) > > thanks, > > greg k-h -- Arve Hjønnevåg
Re: [PATCH] android: binder: Disable preemption while holding the global binder lock
On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman wrote: > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote: >> On Sat, 10 Sep 2016, Peter Zijlstra wrote: >> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote: >> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote: >> > > > In Android systems, the display pipeline relies on low >> > > > latency binder transactions and is therefore sensitive to >> > > > delays caused by contention for the global binder lock. >> > > > Jank is siginificantly reduced by disabling preemption >> > > > while the global binder lock is held. >> > > >> > > That's now how preempt_disable is supposed to use. It is for critical >> > >> > not, that's supposed to be _not_. Just to be absolutely clear, this is >> > NOT how you're supposed to use preempt_disable(). >> > >> > > sections that use per-cpu or similar resources. >> > > >> > > > >> > > > Originally-from: Riley Andrews >> > > > Signed-off-by: Todd Kjos >> > >> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct >> > > > binder_proc *proc, int flags) >> > > > rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); >> > > > unlock_task_sighand(proc->tsk, ); >> > > > >> > > > - return __alloc_fd(files, 0, rlim_cur, flags); >> > > > + preempt_enable_no_resched(); >> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags); >> > > > + preempt_disable(); >> > >> > And the fact that people want to use preempt_enable_no_resched() shows >> > that they're absolutely clueless. >> > >> > This is so broken its not funny. >> > >> > NAK NAK NAK >> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place >> documents clearly that this is tinkering and not proper software >> engineering. > > I have pointed out in the other thread for this patch (the one that had > a patch that could be applied) that the single lock in the binder code > is the main problem here, it should be solved instead of this messing > around with priorities. > While removing the single lock in the binder driver would help reduce the problem that this patch tries to work around, it would not fix it. The largest problems occur when a very low priority thread gets preempted while holding the lock. When a high priority thread then needs the same lock it can't get it. Changing the driver to use more fine-grained locking would reduce the set of threads that can trigger this problem, but there are processes that receive work from both high and low priority threads and could still end up in the same situation. A previous attempt to fix this problem, changed the lock to use rt_mutex instead of mutex, but this apparently did not work as well as this patch. I believe the added overhead was noticeable, and it did not work when the preempted thread was in a different cgroup (I don't know if this is still the case). It would be useful to generic solution to this problem. > So don't worry, I'm not taking this change :) > > thanks, > > greg k-h -- Arve Hjønnevåg
Re: [PATCH] staging: android: lowmemorykiller.c: Fix checkpatch warning
On Thu, Sep 1, 2016 at 11:42 AM, Joe Perches <j...@perches.com> wrote: > On Thu, 2016-09-01 at 20:33 +0200, Greg Kroah-Hartman wrote: >> On Thu, Sep 01, 2016 at 11:19:41AM -0700, Joe Perches wrote: >> > On Thu, 2016-09-01 at 17:36 +0200, Greg Kroah-Hartman wrote: >> > > On Thu, Aug 25, 2016 at 06:26:48PM +0300, Andrey Utkin wrote: >> > > > On Thu, Aug 25, 2016 at 11:10:25AM -0400, Anson Jacob wrote: >> > > > > Fix checkpatch.pl 'line over 80 characters' warning > [] >> > > > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > > > b/drivers/staging/android/lowmemorykiller.c > [] >> > > > > @@ -92,8 +92,8 @@ static unsigned long lowmem_scan(struct shrinker >> > > > > *s, struct shrink_control *sc) >> > > > > int array_size = ARRAY_SIZE(lowmem_adj); >> > > > > int other_free = global_page_state(NR_FREE_PAGES) - >> > > > > totalreserve_pages; >> > > > > int other_file = global_node_page_state(NR_FILE_PAGES) - >> > > > > - >> > > > > global_node_page_state(NR_SHMEM) - >> > > > > - >> > > > > total_swapcache_pages(); >> > > > > + global_node_page_state(NR_SHMEM) - >> > > > > + total_swapcache_pages(); >> > > > > >> > > > > if (lowmem_adj_size < array_size) >> > > > > array_size = lowmem_adj_size; >> > > > Indeed original alignment on opening brace here doesn't make sense. >> > > > I don't mind if your patch gets accepted, but out of curiosity I have >> > > > opened >> > > > this file in vim with kernel coding style plugin loaded >> > > > (https://github.com/vivien/vim-linux-coding-style), selected the >> > > > affected >> > > > lines, and reindented the selection by pressing 'gq'. It ended up with >> > > > this, >> > > > being both shorter and simpler: >> > > > >> > > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > > b/drivers/staging/android/lowmemorykiller.c > [] >> > > > @@ -92,8 +92,7 @@ static unsigned long lowmem_scan(struct shrinker *s, >> > > > struct shrink_control *sc) >> > > > int array_size = ARRAY_SIZE(lowmem_adj); >> > > > int other_free = global_page_state(NR_FREE_PAGES) - >> > > > totalreserve_pages; >> > > > int other_file = global_node_page_state(NR_FILE_PAGES) - >> > > > - >> > > > global_node_page_state(NR_SHMEM) - >> > > > - >> > > > total_swapcache_pages(); >> > > > + global_node_page_state(NR_SHMEM) - >> > > > total_swapcache_pages(); >> > > The original patch here is nicer to read, don't you think? >> > Strictly, these should probably be unsigned long instead of int >> > as that's the return type of global_[node_]page_state(). >> > >> > Maybe something like the below, but then maybe other_free/file >> > should become unsigned long too. > [] >> > diff --git a/drivers/staging/android/lowmemorykiller.c >> > b/drivers/staging/android/lowmemorykiller.c > [] >> > @@ -90,10 +90,11 @@ static unsigned long lowmem_scan(struct shrinker *s, >> > struct shrink_control *sc) >> > int selected_tasksize = 0; >> > short selected_oom_score_adj; >> > int array_size = ARRAY_SIZE(lowmem_adj); >> > - int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages; >> > - int other_file = global_node_page_state(NR_FILE_PAGES) - >> > - >> > global_node_page_state(NR_SHMEM) - >> > - total_swapcache_pages(); >> > + unsigned long nr_free = global_page_state(NR_FREE_PAGES); >> > + unsigned long nr_file = global_node_page_state(NR_FILE_PAGES); >> > + unsigned long nr_shmem = global_node_page_state(NR_SHMEM); >> > + int other_free = nr_free - totalreserve_pages; >> > + int other_file = nr_file - nr_shmem - total_swapcache_pages(); >> > >> > if (lowmem_adj_size < array_size) >> > array_size = lowmem_adj_size; >> I like it, much easier to follow. Care to resend it in a patch form I >> can apply it in? > > I think the android folk should figure out what the right thing > to do with the int/unsigned long calculation is first. > > Arve? Riley? > I think the end result is the same if you store it in an int. If you store the result in a long it will work better, but I don't know of anyone using lowmemorykilller on a system with more than 8TB of memory. -- Arve Hjønnevåg
Re: [PATCH] staging: android: lowmemorykiller.c: Fix checkpatch warning
On Thu, Sep 1, 2016 at 11:42 AM, Joe Perches wrote: > On Thu, 2016-09-01 at 20:33 +0200, Greg Kroah-Hartman wrote: >> On Thu, Sep 01, 2016 at 11:19:41AM -0700, Joe Perches wrote: >> > On Thu, 2016-09-01 at 17:36 +0200, Greg Kroah-Hartman wrote: >> > > On Thu, Aug 25, 2016 at 06:26:48PM +0300, Andrey Utkin wrote: >> > > > On Thu, Aug 25, 2016 at 11:10:25AM -0400, Anson Jacob wrote: >> > > > > Fix checkpatch.pl 'line over 80 characters' warning > [] >> > > > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > > > b/drivers/staging/android/lowmemorykiller.c > [] >> > > > > @@ -92,8 +92,8 @@ static unsigned long lowmem_scan(struct shrinker >> > > > > *s, struct shrink_control *sc) >> > > > > int array_size = ARRAY_SIZE(lowmem_adj); >> > > > > int other_free = global_page_state(NR_FREE_PAGES) - >> > > > > totalreserve_pages; >> > > > > int other_file = global_node_page_state(NR_FILE_PAGES) - >> > > > > - >> > > > > global_node_page_state(NR_SHMEM) - >> > > > > - >> > > > > total_swapcache_pages(); >> > > > > + global_node_page_state(NR_SHMEM) - >> > > > > + total_swapcache_pages(); >> > > > > >> > > > > if (lowmem_adj_size < array_size) >> > > > > array_size = lowmem_adj_size; >> > > > Indeed original alignment on opening brace here doesn't make sense. >> > > > I don't mind if your patch gets accepted, but out of curiosity I have >> > > > opened >> > > > this file in vim with kernel coding style plugin loaded >> > > > (https://github.com/vivien/vim-linux-coding-style), selected the >> > > > affected >> > > > lines, and reindented the selection by pressing 'gq'. It ended up with >> > > > this, >> > > > being both shorter and simpler: >> > > > >> > > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > > b/drivers/staging/android/lowmemorykiller.c > [] >> > > > @@ -92,8 +92,7 @@ static unsigned long lowmem_scan(struct shrinker *s, >> > > > struct shrink_control *sc) >> > > > int array_size = ARRAY_SIZE(lowmem_adj); >> > > > int other_free = global_page_state(NR_FREE_PAGES) - >> > > > totalreserve_pages; >> > > > int other_file = global_node_page_state(NR_FILE_PAGES) - >> > > > - >> > > > global_node_page_state(NR_SHMEM) - >> > > > - >> > > > total_swapcache_pages(); >> > > > + global_node_page_state(NR_SHMEM) - >> > > > total_swapcache_pages(); >> > > The original patch here is nicer to read, don't you think? >> > Strictly, these should probably be unsigned long instead of int >> > as that's the return type of global_[node_]page_state(). >> > >> > Maybe something like the below, but then maybe other_free/file >> > should become unsigned long too. > [] >> > diff --git a/drivers/staging/android/lowmemorykiller.c >> > b/drivers/staging/android/lowmemorykiller.c > [] >> > @@ -90,10 +90,11 @@ static unsigned long lowmem_scan(struct shrinker *s, >> > struct shrink_control *sc) >> > int selected_tasksize = 0; >> > short selected_oom_score_adj; >> > int array_size = ARRAY_SIZE(lowmem_adj); >> > - int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages; >> > - int other_file = global_node_page_state(NR_FILE_PAGES) - >> > - >> > global_node_page_state(NR_SHMEM) - >> > - total_swapcache_pages(); >> > + unsigned long nr_free = global_page_state(NR_FREE_PAGES); >> > + unsigned long nr_file = global_node_page_state(NR_FILE_PAGES); >> > + unsigned long nr_shmem = global_node_page_state(NR_SHMEM); >> > + int other_free = nr_free - totalreserve_pages; >> > + int other_file = nr_file - nr_shmem - total_swapcache_pages(); >> > >> > if (lowmem_adj_size < array_size) >> > array_size = lowmem_adj_size; >> I like it, much easier to follow. Care to resend it in a patch form I >> can apply it in? > > I think the android folk should figure out what the right thing > to do with the int/unsigned long calculation is first. > > Arve? Riley? > I think the end result is the same if you store it in an int. If you store the result in a long it will work better, but I don't know of anyone using lowmemorykilller on a system with more than 8TB of memory. -- Arve Hjønnevåg
Re: [PATCH] android: binder: use VM_ALLOC to get vm area.
On Thu, Sep 1, 2016 at 12:02 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>>
>> __get_vm_area_node()
>> {
>> ...
>> if (flags & VM_IOREMAP)
>> align = 1ul << clamp_t(int, fls_long(size),
>>PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>>
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.gan...@gmail.com>
>> ---
>> drivers/android/binder.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>> goto err_already_mapped;
>> }
>>
>> - area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> + area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>> if (area == NULL) {
>> ret = -ENOMEM;
>> failure_string = "get_vm_area";
>
> What change have you noticed with this patch? Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't? What kind of change does the system now run as because
> of this?
>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?
>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h
There is no alignment requirement on this area unless
cache_is_vipt_aliasing returns true. In that case the area needs to be
aligned with vma->vm_start which is done manually in the code right
after this allocation. If there are no other side effects of changing
this flag this change should be safe, but please run all the tests at
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/
to test it.
--
Arve Hjønnevåg
Re: [PATCH] android: binder: use VM_ALLOC to get vm area.
On Thu, Sep 1, 2016 at 12:02 PM, Greg KH wrote:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>>
>> __get_vm_area_node()
>> {
>> ...
>> if (flags & VM_IOREMAP)
>> align = 1ul << clamp_t(int, fls_long(size),
>>PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>>
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran
>> ---
>> drivers/android/binder.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>> goto err_already_mapped;
>> }
>>
>> - area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> + area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>> if (area == NULL) {
>> ret = -ENOMEM;
>> failure_string = "get_vm_area";
>
> What change have you noticed with this patch? Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't? What kind of change does the system now run as because
> of this?
>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?
>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h
There is no alignment requirement on this area unless
cache_is_vipt_aliasing returns true. In that case the area needs to be
aligned with vma->vm_start which is done manually in the code right
after this allocation. If there are no other side effects of changing
this flag this change should be safe, but please run all the tests at
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/
to test it.
--
Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
s is very RPMB specific for both MMC and UFS and >>> needs to do special operations for RPMB so I don't see how this awareness >>> can be removed or I'm not reading your proposal correctly. >> >> The interface we use specifies reliable-write, write and read >> operations on an rpmb partition. I don't think you need to know more >> than this in either mmc or ufs. I have not seen the ufs spec, but >> based on your code it looks like reliable-write and write can map to >> the same command there. > Yes, the interface can be also abstract on let’s call it raw rpmb packet > read/write level, but I didn’t see the value in at the time as RPMB > operations and the steps are well defined. > Maybe there is a place for to support for fing grained access or at leasting > adding RESULT_READ command as well, for the usecases like yours. > >>> Accessing RPMB is a multiple stepsoperation, the steps can be driven from >>> the user space as done in EMMC ioctl but hidning would reduce the number of >>> system calls and amount of data passed, >>> in some sense the same as in the new mmc MMC_IOC_MULTI_CMD is trying ot >>> achive. >>> >> The main purpose of using the MMC_IOC_MULTI_CMD is not to reduce the >> number of syscalls. It is to prevent other mmc operations from getting >> interleaved with the rpmb packets. Some emmc chips will only respond >> error packet if any other partitions are read from between the write >> and read operation on the rpmb partition. > > I didn’t encountered as our interface doesn’t suffer form that issue but yes > this just another reason to use the new rpmb interface The MMC_IOC_MULTI_CMD solves that problem and is already merged. >> >>>> I have not tested your code, but it looks like we would have to modify the >>>> storage proxy to interpret the data it currently passes through and remove >>>> all >>>> RESULT_READ packets. >>> Your proxy driver just sends ioctls so this wouldn't be much difference and >>> the rpmb code on the trusty w need rewrite just rpmb_send () function, >>> actually removing one set of buffer size. I will try to make that >>> modification if it helps? >>> >> >> No I don't want you to modify the code that runs in the secure OS. >> This would require additional code in boot loaders and proxy servers >> using the existing MMC_IOC_MULTI_CMD command as they too would have to >> interpret the packets to inject RESULT_READ packets. > > I’ve looked at the proxy and and secure OS, the rewirte is not so hard, > though. > I really cannot figure to boo loader and secure OS interactions, you have > this notion of TP and TDEA ports but those are not used from the secure OS. > Is there a software you can point me to? I don't have any bootloader code to share, if that is what you are asking for. The goal is to have access to the rpmb data before loading Linux. For this to work, the bootloader has to implement the rpmb proxy operations (which should be as simple to implement as possible). I don't want to maintain two protocols where the bootloader and old-linux clients use the full rpmb protocol and new linux clients use your rpmb protocol. > > Second if it won’t be possible to use the current implementation if the > storage type change UFS or NVMe anyhow and on the othernad > I’m not suggesting to kill MMC ioctl, so this won’t be breackages of the > existing software. I don't think you need to be compatible with existing Linux user-space code, but it would be possible by emulating the MMC_IOC_MULTI_CMD command. You should try to be compatible with existing secure os interfaces though, as there is no reason for this to be different for ufs and emmc. > > In bottom line I will try to add raw read/write access to RPMB to support > fine grained access and see if you can work with that. > Why not change your code to be compatible with code written against the existing specs instead? I don't see a need for an interface where the client has to prepare all the packets defined by the spec except for one special packet that the kernel will inject. > > I really appreciate your feedback. > Thanks > Tomas > > > > -- Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
fic for both MMC and UFS and >>> needs to do special operations for RPMB so I don't see how this awareness >>> can be removed or I'm not reading your proposal correctly. >> >> The interface we use specifies reliable-write, write and read >> operations on an rpmb partition. I don't think you need to know more >> than this in either mmc or ufs. I have not seen the ufs spec, but >> based on your code it looks like reliable-write and write can map to >> the same command there. > Yes, the interface can be also abstract on let’s call it raw rpmb packet > read/write level, but I didn’t see the value in at the time as RPMB > operations and the steps are well defined. > Maybe there is a place for to support for fing grained access or at leasting > adding RESULT_READ command as well, for the usecases like yours. > >>> Accessing RPMB is a multiple stepsoperation, the steps can be driven from >>> the user space as done in EMMC ioctl but hidning would reduce the number of >>> system calls and amount of data passed, >>> in some sense the same as in the new mmc MMC_IOC_MULTI_CMD is trying ot >>> achive. >>> >> The main purpose of using the MMC_IOC_MULTI_CMD is not to reduce the >> number of syscalls. It is to prevent other mmc operations from getting >> interleaved with the rpmb packets. Some emmc chips will only respond >> error packet if any other partitions are read from between the write >> and read operation on the rpmb partition. > > I didn’t encountered as our interface doesn’t suffer form that issue but yes > this just another reason to use the new rpmb interface The MMC_IOC_MULTI_CMD solves that problem and is already merged. >> >>>> I have not tested your code, but it looks like we would have to modify the >>>> storage proxy to interpret the data it currently passes through and remove >>>> all >>>> RESULT_READ packets. >>> Your proxy driver just sends ioctls so this wouldn't be much difference and >>> the rpmb code on the trusty w need rewrite just rpmb_send () function, >>> actually removing one set of buffer size. I will try to make that >>> modification if it helps? >>> >> >> No I don't want you to modify the code that runs in the secure OS. >> This would require additional code in boot loaders and proxy servers >> using the existing MMC_IOC_MULTI_CMD command as they too would have to >> interpret the packets to inject RESULT_READ packets. > > I’ve looked at the proxy and and secure OS, the rewirte is not so hard, > though. > I really cannot figure to boo loader and secure OS interactions, you have > this notion of TP and TDEA ports but those are not used from the secure OS. > Is there a software you can point me to? I don't have any bootloader code to share, if that is what you are asking for. The goal is to have access to the rpmb data before loading Linux. For this to work, the bootloader has to implement the rpmb proxy operations (which should be as simple to implement as possible). I don't want to maintain two protocols where the bootloader and old-linux clients use the full rpmb protocol and new linux clients use your rpmb protocol. > > Second if it won’t be possible to use the current implementation if the > storage type change UFS or NVMe anyhow and on the othernad > I’m not suggesting to kill MMC ioctl, so this won’t be breackages of the > existing software. I don't think you need to be compatible with existing Linux user-space code, but it would be possible by emulating the MMC_IOC_MULTI_CMD command. You should try to be compatible with existing secure os interfaces though, as there is no reason for this to be different for ufs and emmc. > > In bottom line I will try to add raw read/write access to RPMB to support > fine grained access and see if you can work with that. > Why not change your code to be compatible with code written against the existing specs instead? I don't see a need for an interface where the client has to prepare all the packets defined by the spec except for one special packet that the kernel will inject. > > I really appreciate your feedback. > Thanks > Tomas > > > > -- Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
On Thu, Jun 2, 2016 at 6:17 AM, Winkler, Tomas <tomas.wink...@intel.com> wrote: >> >> On Wed, Jun 1, 2016 at 2:41 PM, Tomas Winkler <tomas.wink...@intel.com> >> wrote: >> > Few storage technology such is EMMC, UFS, and NVMe support RPMB >> >hardware partition with common protocol and frame layout. >> > The RPMB partition cannot be accessed via standard block layer, but >> >by a set of specific commands: WRITE, READ, GET_WRITE_COUNTER, and >> >PROGRAM_KEY. >> >... >> >> If the same protocol is used by all these standards, why not export it >> directly >> (including the RESULT_READ command or not even knowing the command >> types)? > The protocol is the same, but the wrapping of the packets is storage type > specific so > I believe that the best abstraction is on those 4 operations level. I'm not > sure if the code would > be simpler if it would be exposed on a lower level. I disagree. With the two storage types you support, the packets are identical. The only difference is the low level commands you use to send and receive them. > RESULT_READ is a command to be issued for getting preceeding write > operation status, it's a kind of work around about the fact that you have to > issue a read operation > in order to retrieve data in this case a write operation result. It can be > successfully hidden and final result of the operation is delivered > to the user. > Yes, it is possible to hide the result read command, but that does not mean you should. The rpmb protocol is designed to let two endpoints communicate in a way that lets them detect tampering. While the packet you inject does not contain any protected data, you can still view it as a form of tampering. If a future rpmb protocol version adds features, you could loose the ability to inject packets. >> While I would prefer an rpmb specific interface over the existing raw >> mmc command interface, all I need is an rpmb operation that lets me send >> and receive buffers without interruption. > > I currently don't see an obstacle on doing the same with proposed interface, > what is the issue are seeing. > The main issue is that you are injecting commands, so code that follows the mmc spec will not work. >> You can find our exiting user-space code here at >> https://android.googlesource.com/platform/system/core/+/master/trusty/s >> torage/proxy/rpmb.c. >> If you use an interface more similar to this, I think your emmc and ufs >> specific code would be simpler. Also, if you don't need the in-kernel >> interface, the kernel would not need to know the details of the rpmb >> protocol at all. > > My major interest is the in-kernel protocol the user space API was more > intended for debug, but I found it would be even more useful. > The store type access is very RPMB specific for both MMC and UFS and needs > to do special operations for RPMB so I don't see how this awareness can be > removed or I'm not reading your proposal correctly. The interface we use specifies reliable-write, write and read operations on an rpmb partition. I don't think you need to know more than this in either mmc or ufs. I have not seen the ufs spec, but based on your code it looks like reliable-write and write can map to the same command there. > Accessing RPMB is a multiple stepsoperation, the steps can be driven from the > user space as done in EMMC ioctl but hidning would reduce the number of > system calls and amount of data passed, > in some sense the same as in the new mmc MMC_IOC_MULTI_CMD is trying ot > achive. > The main purpose of using the MMC_IOC_MULTI_CMD is not to reduce the number of syscalls. It is to prevent other mmc operations from getting interleaved with the rpmb packets. Some emmc chips will only respond error packet if any other partitions are read from between the write and read operation on the rpmb partition. >> I have not tested your code, but it looks like we would have to modify the >> storage proxy to interpret the data it currently passes through and remove >> all >> RESULT_READ packets. > Your proxy driver just sends ioctls so this wouldn't be much difference and > the rpmb code on the trusty w need rewrite just rpmb_send () function, > actually removing one set of buffer size. I will try to make that > modification if it helps? > No I don't want you to modify the code that runs in the secure OS. This would require additional code in boot loaders and proxy servers using the existing MMC_IOC_MULTI_CMD command as they too would have to interpret the packets to inject RESULT_READ packets. > Thanks for interest and your review. > Tomas > > -- Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
On Thu, Jun 2, 2016 at 6:17 AM, Winkler, Tomas wrote: >> >> On Wed, Jun 1, 2016 at 2:41 PM, Tomas Winkler >> wrote: >> > Few storage technology such is EMMC, UFS, and NVMe support RPMB >> >hardware partition with common protocol and frame layout. >> > The RPMB partition cannot be accessed via standard block layer, but >> >by a set of specific commands: WRITE, READ, GET_WRITE_COUNTER, and >> >PROGRAM_KEY. >> >... >> >> If the same protocol is used by all these standards, why not export it >> directly >> (including the RESULT_READ command or not even knowing the command >> types)? > The protocol is the same, but the wrapping of the packets is storage type > specific so > I believe that the best abstraction is on those 4 operations level. I'm not > sure if the code would > be simpler if it would be exposed on a lower level. I disagree. With the two storage types you support, the packets are identical. The only difference is the low level commands you use to send and receive them. > RESULT_READ is a command to be issued for getting preceeding write > operation status, it's a kind of work around about the fact that you have to > issue a read operation > in order to retrieve data in this case a write operation result. It can be > successfully hidden and final result of the operation is delivered > to the user. > Yes, it is possible to hide the result read command, but that does not mean you should. The rpmb protocol is designed to let two endpoints communicate in a way that lets them detect tampering. While the packet you inject does not contain any protected data, you can still view it as a form of tampering. If a future rpmb protocol version adds features, you could loose the ability to inject packets. >> While I would prefer an rpmb specific interface over the existing raw >> mmc command interface, all I need is an rpmb operation that lets me send >> and receive buffers without interruption. > > I currently don't see an obstacle on doing the same with proposed interface, > what is the issue are seeing. > The main issue is that you are injecting commands, so code that follows the mmc spec will not work. >> You can find our exiting user-space code here at >> https://android.googlesource.com/platform/system/core/+/master/trusty/s >> torage/proxy/rpmb.c. >> If you use an interface more similar to this, I think your emmc and ufs >> specific code would be simpler. Also, if you don't need the in-kernel >> interface, the kernel would not need to know the details of the rpmb >> protocol at all. > > My major interest is the in-kernel protocol the user space API was more > intended for debug, but I found it would be even more useful. > The store type access is very RPMB specific for both MMC and UFS and needs > to do special operations for RPMB so I don't see how this awareness can be > removed or I'm not reading your proposal correctly. The interface we use specifies reliable-write, write and read operations on an rpmb partition. I don't think you need to know more than this in either mmc or ufs. I have not seen the ufs spec, but based on your code it looks like reliable-write and write can map to the same command there. > Accessing RPMB is a multiple stepsoperation, the steps can be driven from the > user space as done in EMMC ioctl but hidning would reduce the number of > system calls and amount of data passed, > in some sense the same as in the new mmc MMC_IOC_MULTI_CMD is trying ot > achive. > The main purpose of using the MMC_IOC_MULTI_CMD is not to reduce the number of syscalls. It is to prevent other mmc operations from getting interleaved with the rpmb packets. Some emmc chips will only respond error packet if any other partitions are read from between the write and read operation on the rpmb partition. >> I have not tested your code, but it looks like we would have to modify the >> storage proxy to interpret the data it currently passes through and remove >> all >> RESULT_READ packets. > Your proxy driver just sends ioctls so this wouldn't be much difference and > the rpmb code on the trusty w need rewrite just rpmb_send () function, > actually removing one set of buffer size. I will try to make that > modification if it helps? > No I don't want you to modify the code that runs in the secure OS. This would require additional code in boot loaders and proxy servers using the existing MMC_IOC_MULTI_CMD command as they too would have to interpret the packets to inject RESULT_READ packets. > Thanks for interest and your review. > Tomas > > -- Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
On Wed, Jun 1, 2016 at 2:41 PM, Tomas Winkler <tomas.wink...@intel.com> wrote: > Few storage technology such is EMMC, UFS, and NVMe support RPMB > hardware partition with common protocol and frame layout. > The RPMB partition cannot be accessed via standard block layer, but > by a set of specific commands: WRITE, READ, GET_WRITE_COUNTER, and > PROGRAM_KEY. >... If the same protocol is used by all these standards, why not export it directly (including the RESULT_READ command or not even knowing the command types)? While I would prefer an rpmb specific interface over the existing raw mmc command interface, all I need is an rpmb operation that lets me send and receive buffers without interruption. You can find our exiting user-space code here at https://android.googlesource.com/platform/system/core/+/master/trusty/storage/proxy/rpmb.c. If you use an interface more similar to this, I think your emmc and ufs specific code would be simpler. Also, if you don't need the in-kernel interface, the kernel would not need to know the details of the rpmb protocol at all. I have not tested your code, but it looks like we would have to modify the storage proxy to interpret the data it currently passes through and remove all RESULT_READ packets. -- Arve Hjønnevåg
Re: [PATCH v4 0/8] Replay Protected Memory Block (RPMB) subsystem
On Wed, Jun 1, 2016 at 2:41 PM, Tomas Winkler wrote: > Few storage technology such is EMMC, UFS, and NVMe support RPMB > hardware partition with common protocol and frame layout. > The RPMB partition cannot be accessed via standard block layer, but > by a set of specific commands: WRITE, READ, GET_WRITE_COUNTER, and > PROGRAM_KEY. >... If the same protocol is used by all these standards, why not export it directly (including the RESULT_READ command or not even knowing the command types)? While I would prefer an rpmb specific interface over the existing raw mmc command interface, all I need is an rpmb operation that lets me send and receive buffers without interruption. You can find our exiting user-space code here at https://android.googlesource.com/platform/system/core/+/master/trusty/storage/proxy/rpmb.c. If you use an interface more similar to this, I think your emmc and ufs specific code would be simpler. Also, if you don't need the in-kernel interface, the kernel would not need to know the details of the rpmb protocol at all. I have not tested your code, but it looks like we would have to modify the storage proxy to interpret the data it currently passes through and remove all RESULT_READ packets. -- Arve Hjønnevåg
Re: Regression in v4.0.0-rc1 with Android Binder
On Thu, Feb 26, 2015 at 2:04 PM, Amit Pundir wrote:
> Hi,
>
> I ran into series of following binder mmap failures with v4.0.0-rc1:
> [ cut here ]
> WARNING: CPU: 0 PID: 1971 at mm/vmalloc.c:130
> vmap_page_range_noflush+0x119/0x144()
> CPU: 0 PID: 1971 Comm: healthd Not tainted 4.0.0-rc1-00399-g7da3fdc-dirty #157
> Hardware name: ARM-Versatile Express
> [] (unwind_backtrace) from [] (show_stack+0x11/0x14)
> [] (show_stack) from [] (dump_stack+0x59/0x7c)
> [] (dump_stack) from [] (warn_slowpath_common+0x55/0x84)
> [] (warn_slowpath_common) from []
> (warn_slowpath_null+0x17/0x1c)
> [] (warn_slowpath_null) from []
> (vmap_page_range_noflush+0x119/0x144)
> [] (vmap_page_range_noflush) from []
> (map_vm_area+0x27/0x48)
> [] (map_vm_area) from []
> (binder_update_page_range+0x12f/0x27c)
> [] (binder_update_page_range) from []
> (binder_mmap+0xbf/0x1ac)
> [] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4)
> [] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250)
> [] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60)
> [] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80)
> [] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c)
> ---[ end trace 48c2c4b9a1349e54 ]---
> binder: 1982: binder_alloc_buf failed to map page at f0e0 in kernel
> binder: binder_mmap: 1982 b6bde000-b6cdc000 alloc small buf failed -12
>
>
> Turned out that the following commit tripped off binder:
> --8<--
> commit 71394fe50146202f2c8d92cf50f5ebc761acf254
> Author: Andrey Ryabinin
> Date: Fri Feb 13 14:40:03 2015 -0800
>
> mm: vmalloc: add flag preventing guard hole allocation
> -->8--
>
>
> Explicitly disabling the vmalloc no guard (VM_NO_GUARD) flag in binder
> worked fine for me. So does a fix like this look reasonable enough to
> submit?
> --8<--
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -601,6 +601,7 @@ static int binder_update_page_range(struct
> binder_proc *proc, int allocate,
> goto err_alloc_page_failed;
> }
> tmp_area.addr = page_addr;
> + tmp_area.flags &= ~VM_NO_GUARD;
This variable is not initialized, so I would expect this to add a
warning. Setting it to VM_NO_GUARD and removing, " + PAGE_SIZE /*
guard page? */" fromt he next line would be better. However, the "new"
map_kernel_range_noflush api seems like a better api to use for this,
since it removes the need to create a dummy vm_struct at all.
> tmp_area.size = PAGE_SIZE + PAGE_SIZE /* guard page? */;
> ret = map_vm_area(_area, PAGE_KERNEL, page);
> if (ret) {
> -->8--
>
>
> Regards,
> Amit Pundir
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Regression in v4.0.0-rc1 with Android Binder
On Thu, Feb 26, 2015 at 2:04 PM, Amit Pundir amit.pun...@linaro.org wrote:
Hi,
I ran into series of following binder mmap failures with v4.0.0-rc1:
[ cut here ]
WARNING: CPU: 0 PID: 1971 at mm/vmalloc.c:130
vmap_page_range_noflush+0x119/0x144()
CPU: 0 PID: 1971 Comm: healthd Not tainted 4.0.0-rc1-00399-g7da3fdc-dirty #157
Hardware name: ARM-Versatile Express
[c001246d] (unwind_backtrace) from [c000f7f9] (show_stack+0x11/0x14)
[c000f7f9] (show_stack) from [c049a221] (dump_stack+0x59/0x7c)
[c049a221] (dump_stack) from [c001cf21] (warn_slowpath_common+0x55/0x84)
[c001cf21] (warn_slowpath_common) from [c001cfe3]
(warn_slowpath_null+0x17/0x1c)
[c001cfe3] (warn_slowpath_null) from [c00c66c5]
(vmap_page_range_noflush+0x119/0x144)
[c00c66c5] (vmap_page_range_noflush) from [c00c716b]
(map_vm_area+0x27/0x48)
[c00c716b] (map_vm_area) from [c038ddaf]
(binder_update_page_range+0x12f/0x27c)
[c038ddaf] (binder_update_page_range) from [c038e857]
(binder_mmap+0xbf/0x1ac)
[c038e857] (binder_mmap) from [c00c2dc7] (mmap_region+0x2eb/0x4d4)
[c00c2dc7] (mmap_region) from [c00c3197] (do_mmap_pgoff+0x1e7/0x250)
[c00c3197] (do_mmap_pgoff) from [c00b35b5] (vm_mmap_pgoff+0x45/0x60)
[c00b35b5] (vm_mmap_pgoff) from [c00c1f39] (SyS_mmap_pgoff+0x5d/0x80)
[c00c1f39] (SyS_mmap_pgoff) from [c000ce81] (ret_fast_syscall+0x1/0x5c)
---[ end trace 48c2c4b9a1349e54 ]---
binder: 1982: binder_alloc_buf failed to map page at f0e0 in kernel
binder: binder_mmap: 1982 b6bde000-b6cdc000 alloc small buf failed -12
Turned out that the following commit tripped off binder:
--8--
commit 71394fe50146202f2c8d92cf50f5ebc761acf254
Author: Andrey Ryabinin a.ryabi...@samsung.com
Date: Fri Feb 13 14:40:03 2015 -0800
mm: vmalloc: add flag preventing guard hole allocation
--8--
Explicitly disabling the vmalloc no guard (VM_NO_GUARD) flag in binder
worked fine for me. So does a fix like this look reasonable enough to
submit?
--8--
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -601,6 +601,7 @@ static int binder_update_page_range(struct
binder_proc *proc, int allocate,
goto err_alloc_page_failed;
}
tmp_area.addr = page_addr;
+ tmp_area.flags = ~VM_NO_GUARD;
This variable is not initialized, so I would expect this to add a
warning. Setting it to VM_NO_GUARD and removing, + PAGE_SIZE /*
guard page? */ fromt he next line would be better. However, the new
map_kernel_range_noflush api seems like a better api to use for this,
since it removes the need to create a dummy vm_struct at all.
tmp_area.size = PAGE_SIZE + PAGE_SIZE /* guard page? */;
ret = map_vm_area(tmp_area, PAGE_KERNEL, page);
if (ret) {
--8--
Regards,
Amit Pundir
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
d > *)buffer->data + size; > > > I don't really understand when buffer->data has to be page aligned and > when not. This code has very few comments. > buffer->data never needs to be page aligned. The code above rounds to page boundaries to allocate new pages as needed. >730 >731 list_add(_buffer->entry, >entry); >732 new_buffer->free = 1; >733 binder_insert_free_buffer(proc, new_buffer); >734 } > > Does the stop_on_user_error option work? There should be some > documentation for this stuff. > Yes. > regards, > dan carpenter > -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
} Does the stop_on_user_error option work? There should be some documentation for this stuff. Yes. regards, dan carpenter -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: binder: fix usage of uninit scalar in binder_transaction()
On Sat, May 3, 2014 at 4:15 PM, Christian Engelmayer wrote: > Fix the error path when a cookie mismatch is detected. In that case the > function jumps to the exit label without setting the uninitialized, local > variable 'return_error'. Detected by Coverity - CID 201453. > > Signed-off-by: Christian Engelmayer > --- > Compile tested and applies against branch staging-next of tree > git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > --- > drivers/staging/android/binder.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/android/binder.c > b/drivers/staging/android/binder.c > index 1f5e249..ca1b0e3 100644 > --- a/drivers/staging/android/binder.c > +++ b/drivers/staging/android/binder.c > @@ -1529,6 +1529,7 @@ static void binder_transaction(struct binder_proc *proc, > proc->pid, thread->pid, > (u64)fp->binder, node->debug_id, > (u64)fp->cookie, (u64)node->cookie); > + return_error = BR_FAILED_REPLY; > goto err_binder_get_ref_for_node_failed; > } > ref = binder_get_ref_for_node(target_proc, node); > -- > 1.9.1 Acked-by: Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: binder: fix usage of uninit scalar in binder_transaction()
On Sat, May 3, 2014 at 4:15 PM, Christian Engelmayer cenge...@gmx.at wrote: Fix the error path when a cookie mismatch is detected. In that case the function jumps to the exit label without setting the uninitialized, local variable 'return_error'. Detected by Coverity - CID 201453. Signed-off-by: Christian Engelmayer cenge...@gmx.at --- Compile tested and applies against branch staging-next of tree git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git --- drivers/staging/android/binder.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 1f5e249..ca1b0e3 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -1529,6 +1529,7 @@ static void binder_transaction(struct binder_proc *proc, proc-pid, thread-pid, (u64)fp-binder, node-debug_id, (u64)fp-cookie, (u64)node-cookie); + return_error = BR_FAILED_REPLY; goto err_binder_get_ref_for_node_failed; } ref = binder_get_ref_for_node(target_proc, node); -- 1.9.1 Acked-by: Arve Hjønnevåg a...@android.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 2:04 PM, John Stultz wrote: > On 02/21/2014 01:56 PM, Arve Hjønnevåg wrote: >> On Fri, Feb 21, 2014 at 1:29 PM, John Stultz wrote: >>> On 02/21/2014 12:59 PM, Arve Hjønnevåg wrote: >>>> On Fri, Feb 21, 2014 at 12:43 PM, John Stultz >>>> wrote: >>>>> From: Arve Hjønnevåg >>>>> >>>>> For 64bit systems we want to use the same binder interface for 32bit and >>>>> 64bit processes. Thus the size and the layout of the structures passed >>>>> between the kernel and the userspace has to be the same for both 32 and >>>>> 64bit processes. >>>>> >>>>> This change replaces all the uses of void* and size_t with >>>>> binder_uintptr_t and binder_size_t. These are then typedefed to specific >>>>> sizes depending on the use of the interface, as follows: >>>>>* __u32 - on legacy 32bit only userspace >>>>>* __u64 - on mixed 32/64bit userspace where all processes use the >>>>> same >>>>> interface. >>>>> >>>>> This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and >>>>> hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. >>>>> >>>> It only increments the version to 8 if the old 32 bit interface is not >>>> selected. >>>> >>>>> This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for >>>>> compatability, which if set which enables the old protocol on 32 bit >>>>> systems. >>> Ok. I thought that point was covered by the detail on >>> CONFIG_ANDROID_BINDER_IPC_32BIT, but maybe its not explicit enough. >>> >>> Would you be ok with: >>> >>> This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and >>> hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. >>> >>> This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for >>> compatability, which if set which enables the old protocol, setting >>> BINDER_CURRENT_PROTOCOL_VERSION to 7, on 32 bit systems. >>> >>> ? >>> >> Yes, but replacing "This change" with "Selecting the 64 bit interface" >> would also work. > > Ok.. I might stick to my wording above since with this patch, the 64bit > interface is now the unconditional case, with the 32bit interface having > the config option. So it might be more clear as there is no 64bit > interface option to select. > > I'll add that bit and resend. Everything else in the patch series ok by you? > Assuming you did not change the code, yes. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 1:29 PM, John Stultz wrote: > On 02/21/2014 12:59 PM, Arve Hjønnevåg wrote: >> On Fri, Feb 21, 2014 at 12:43 PM, John Stultz wrote: >>> From: Arve Hjønnevåg >>> >>> For 64bit systems we want to use the same binder interface for 32bit and >>> 64bit processes. Thus the size and the layout of the structures passed >>> between the kernel and the userspace has to be the same for both 32 and >>> 64bit processes. >>> >>> This change replaces all the uses of void* and size_t with >>> binder_uintptr_t and binder_size_t. These are then typedefed to specific >>> sizes depending on the use of the interface, as follows: >>>* __u32 - on legacy 32bit only userspace >>>* __u64 - on mixed 32/64bit userspace where all processes use the >>> same >>> interface. >>> >>> This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and >>> hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. >>> >> It only increments the version to 8 if the old 32 bit interface is not >> selected. >> >>> This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for >>> compatability, which if set which enables the old protocol on 32 bit >>> systems. > > Ok. I thought that point was covered by the detail on > CONFIG_ANDROID_BINDER_IPC_32BIT, but maybe its not explicit enough. > > Would you be ok with: > > This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and > hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. > > This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for > compatability, which if set which enables the old protocol, setting > BINDER_CURRENT_PROTOCOL_VERSION to 7, on 32 bit systems. > > ? > Yes, but replacing "This change" with "Selecting the 64 bit interface" would also work. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 12:43 PM, John Stultz wrote: > From: Arve Hjønnevåg > > For 64bit systems we want to use the same binder interface for 32bit and > 64bit processes. Thus the size and the layout of the structures passed > between the kernel and the userspace has to be the same for both 32 and > 64bit processes. > > This change replaces all the uses of void* and size_t with > binder_uintptr_t and binder_size_t. These are then typedefed to specific > sizes depending on the use of the interface, as follows: >* __u32 - on legacy 32bit only userspace >* __u64 - on mixed 32/64bit userspace where all processes use the same > interface. > > This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and > hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. > It only increments the version to 8 if the old 32 bit interface is not selected. > This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for > compatability, which if set which enables the old protocol on 32 bit > systems. > > Please note that all 64bit kernels will use the 64bit Binder ABI. > -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 12:43 PM, John Stultz john.stu...@linaro.org wrote: From: Arve Hjønnevåg a...@android.com For 64bit systems we want to use the same binder interface for 32bit and 64bit processes. Thus the size and the layout of the structures passed between the kernel and the userspace has to be the same for both 32 and 64bit processes. This change replaces all the uses of void* and size_t with binder_uintptr_t and binder_size_t. These are then typedefed to specific sizes depending on the use of the interface, as follows: * __u32 - on legacy 32bit only userspace * __u64 - on mixed 32/64bit userspace where all processes use the same interface. This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. It only increments the version to 8 if the old 32 bit interface is not selected. This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for compatability, which if set which enables the old protocol on 32 bit systems. Please note that all 64bit kernels will use the 64bit Binder ABI. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 1:29 PM, John Stultz john.stu...@linaro.org wrote: On 02/21/2014 12:59 PM, Arve Hjønnevåg wrote: On Fri, Feb 21, 2014 at 12:43 PM, John Stultz john.stu...@linaro.org wrote: From: Arve Hjønnevåg a...@android.com For 64bit systems we want to use the same binder interface for 32bit and 64bit processes. Thus the size and the layout of the structures passed between the kernel and the userspace has to be the same for both 32 and 64bit processes. This change replaces all the uses of void* and size_t with binder_uintptr_t and binder_size_t. These are then typedefed to specific sizes depending on the use of the interface, as follows: * __u32 - on legacy 32bit only userspace * __u64 - on mixed 32/64bit userspace where all processes use the same interface. This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. It only increments the version to 8 if the old 32 bit interface is not selected. This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for compatability, which if set which enables the old protocol on 32 bit systems. Ok. I thought that point was covered by the detail on CONFIG_ANDROID_BINDER_IPC_32BIT, but maybe its not explicit enough. Would you be ok with: This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for compatability, which if set which enables the old protocol, setting BINDER_CURRENT_PROTOCOL_VERSION to 7, on 32 bit systems. ? Yes, but replacing This change with Selecting the 64 bit interface would also work. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging: binder: Support concurrent 32 bit and 64 bit processes.
On Fri, Feb 21, 2014 at 2:04 PM, John Stultz john.stu...@linaro.org wrote: On 02/21/2014 01:56 PM, Arve Hjønnevåg wrote: On Fri, Feb 21, 2014 at 1:29 PM, John Stultz john.stu...@linaro.org wrote: On 02/21/2014 12:59 PM, Arve Hjønnevåg wrote: On Fri, Feb 21, 2014 at 12:43 PM, John Stultz john.stu...@linaro.org wrote: From: Arve Hjønnevåg a...@android.com For 64bit systems we want to use the same binder interface for 32bit and 64bit processes. Thus the size and the layout of the structures passed between the kernel and the userspace has to be the same for both 32 and 64bit processes. This change replaces all the uses of void* and size_t with binder_uintptr_t and binder_size_t. These are then typedefed to specific sizes depending on the use of the interface, as follows: * __u32 - on legacy 32bit only userspace * __u64 - on mixed 32/64bit userspace where all processes use the same interface. This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. It only increments the version to 8 if the old 32 bit interface is not selected. This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for compatability, which if set which enables the old protocol on 32 bit systems. Ok. I thought that point was covered by the detail on CONFIG_ANDROID_BINDER_IPC_32BIT, but maybe its not explicit enough. Would you be ok with: This change also increments the BINDER_CURRENT_PROTOCOL_VERSION to 8 and hooks the compat_ioctl entry for the mixed 32/64bit Android userspace. This patch also provides a CONFIG_ANDROID_BINDER_IPC_32BIT option for compatability, which if set which enables the old protocol, setting BINDER_CURRENT_PROTOCOL_VERSION to 7, on 32 bit systems. ? Yes, but replacing This change with Selecting the 64 bit interface would also work. Ok.. I might stick to my wording above since with this patch, the 64bit interface is now the unconditional case, with the 32bit interface having the config option. So it might be more clear as there is no 64bit interface option to select. I'll add that bit and resend. Everything else in the patch series ok by you? Assuming you did not change the code, yes. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/14] staging: binder: Improve Kconfig entry for ANDROID_BINDER_IPC_32BIT
On Mon, Feb 17, 2014 at 1:58 PM, John Stultz wrote: > Add a more clear explanation of the option in the prompt, and > make the config depend on ANDROID_BINDER_IPC being selected. > > Also sets the default to y, which matches AOSP. > I don't know if you want the default to be y. By the time this kernel is used, the user-space default may have switched to using the 64 bit interface by default. The android-3.10 kernel currently defaulting to y since it may get used with a 4.4 android user-space, and because the 32 bit arm kernel is missing the 8 byte get_user variant that it needs. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 12/14] staging: binder: Fix ABI for 64bit Android
On Tue, Feb 18, 2014 at 12:32 PM, Greg KH wrote:
> On Tue, Feb 18, 2014 at 12:02:07PM -0800, John Stultz wrote:
>> On Tue, Feb 18, 2014 at 11:49 AM, Greg KH wrote:
>> > On Tue, Feb 18, 2014 at 11:30:26AM -0800, John Stultz wrote:
>> >> On Tue, Feb 18, 2014 at 11:08 AM, Greg KH
>> >> wrote:
>> >> > On Mon, Feb 17, 2014 at 01:58:40PM -0800, John Stultz wrote:
>> >> >> From: Serban Constantinescu
>> >> >>
>> >> >> This patch fixes the ABI for 64bit Android userspace.
>> >> >> BC_REQUEST_DEATH_NOTIFICATION and BC_CLEAR_DEATH_NOTIFICATION claim
>> >> >> to be using struct binder_ptr_cookie, but they are using a 32bit handle
>> >> >> and a pointer.
>> >> >>
>> >> >> On 32bit systems the payload size is the same as the size of struct
>> >> >> binder_ptr_cookie, however for 64bit systems this will differ. This
>> >> >> patch adds struct binder_handle_cookie that fixes this issue for 64bit
>> >> >> Android.
>> >> >>
>> >> >> Since there are no 64bit users of this interface that we know of this
>> >> >> change should not affect any existing systems.
>> >> >
>> >> > But you are changing the ioctl structures here, what is that going to
>> >> > cause with old programs?
>> >>
>> >> So I'd be glad for Serban or Arve to clarify, but my understanding
>> >> (and as is described in the commit message) is that the assumption is
>> >> there are no 64bit binder users at this point, and the ioctl structure
>> >> changes are made such that existing 32bit applications are unaffected.
>> >
>> > How does changing the structure size, and contents, not affect any
>> > applications or the kernel code? What am I missing here?
>>
>> On 32bit pointers and ints are the same size? (Years ago I sat through
>> your presentation on this, so I'm worried I'm missing something here
>> :)
>>
>> struct binder_ptr_cookie {
>> void *ptr;
>> void *cookie;
>> };
>>
>> struct binder_handle_cookie {
>> __u32 handle;
>> void *cookie;
>> } __attribute__((packed));
>>
>>
>> On 32bit systems these are the same size. Now on 64bit systems, this
>> changes things, and would break users, but the assumption here is
>> there are no pre-existing 64bit binder users.
>
> But you added a field to the existing structure, right? I don't really
> remember the patch, it was a few hundred back in my review of stuff
> today, sorry...
>
> greg k-h
The existing structure is not changed. These two commands were defined
with wrong structure that did not match the code. Since a binder
pointer and handle are the same size on 32 bit systems, this change
does not affect them. On 64 bit systems, the ioctl number does change,
but these systems need the next patch to run 32 bit processes anyway,
so I don't expect anyone to ship a system without this change. The
main purpose of this patch is to add the binder_handle_cookie struct
so the service manager does not have to define its own version
(libbinder writes one field at a time so it does not use the struct).
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 12/14] staging: binder: Fix ABI for 64bit Android
On Tue, Feb 18, 2014 at 12:32 PM, Greg KH gre...@linuxfoundation.org wrote:
On Tue, Feb 18, 2014 at 12:02:07PM -0800, John Stultz wrote:
On Tue, Feb 18, 2014 at 11:49 AM, Greg KH gre...@linuxfoundation.org wrote:
On Tue, Feb 18, 2014 at 11:30:26AM -0800, John Stultz wrote:
On Tue, Feb 18, 2014 at 11:08 AM, Greg KH gre...@linuxfoundation.org
wrote:
On Mon, Feb 17, 2014 at 01:58:40PM -0800, John Stultz wrote:
From: Serban Constantinescu serban.constantine...@arm.com
This patch fixes the ABI for 64bit Android userspace.
BC_REQUEST_DEATH_NOTIFICATION and BC_CLEAR_DEATH_NOTIFICATION claim
to be using struct binder_ptr_cookie, but they are using a 32bit handle
and a pointer.
On 32bit systems the payload size is the same as the size of struct
binder_ptr_cookie, however for 64bit systems this will differ. This
patch adds struct binder_handle_cookie that fixes this issue for 64bit
Android.
Since there are no 64bit users of this interface that we know of this
change should not affect any existing systems.
But you are changing the ioctl structures here, what is that going to
cause with old programs?
So I'd be glad for Serban or Arve to clarify, but my understanding
(and as is described in the commit message) is that the assumption is
there are no 64bit binder users at this point, and the ioctl structure
changes are made such that existing 32bit applications are unaffected.
How does changing the structure size, and contents, not affect any
applications or the kernel code? What am I missing here?
On 32bit pointers and ints are the same size? (Years ago I sat through
your presentation on this, so I'm worried I'm missing something here
:)
struct binder_ptr_cookie {
void *ptr;
void *cookie;
};
struct binder_handle_cookie {
__u32 handle;
void *cookie;
} __attribute__((packed));
On 32bit systems these are the same size. Now on 64bit systems, this
changes things, and would break users, but the assumption here is
there are no pre-existing 64bit binder users.
But you added a field to the existing structure, right? I don't really
remember the patch, it was a few hundred back in my review of stuff
today, sorry...
greg k-h
The existing structure is not changed. These two commands were defined
with wrong structure that did not match the code. Since a binder
pointer and handle are the same size on 32 bit systems, this change
does not affect them. On 64 bit systems, the ioctl number does change,
but these systems need the next patch to run 32 bit processes anyway,
so I don't expect anyone to ship a system without this change. The
main purpose of this patch is to add the binder_handle_cookie struct
so the service manager does not have to define its own version
(libbinder writes one field at a time so it does not use the struct).
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/14] staging: binder: Improve Kconfig entry for ANDROID_BINDER_IPC_32BIT
On Mon, Feb 17, 2014 at 1:58 PM, John Stultz john.stu...@linaro.org wrote: Add a more clear explanation of the option in the prompt, and make the config depend on ANDROID_BINDER_IPC being selected. Also sets the default to y, which matches AOSP. I don't know if you want the default to be y. By the time this kernel is used, the user-space default may have switched to using the 64 bit interface by default. The android-3.10 kernel currently defaulting to y since it may get used with a 4.4 android user-space, and because the 32 bit arm kernel is missing the 8 byte get_user variant that it needs. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila wrote: > On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg wrote: >> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila >> wrote: >>> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: >>>> >>>> Assuming you are talking about a kernel compat layer that translates >>>> the flat_binder_object structs as they pass between 32 bit and 64 bit >>>> processes, that will not always work. The data portion of the message >>>> sometimes contain size values that are invisible to the kernel, but >>>> these values will be wrong if the kernel move data to make room for a >>>> different size flat_binder_object. >>>> >>> >>> Hi Arve, >>> >>> Yes, I was talking about translating flat_binder_objects. >>> >>> I understand the potential issue for the user data payload, however, >>> since most applications will use libbinder, the only problematic case >>> is readIntPtr/writeIntPtr, which we can deprecate and convert >>> applications that use it to readInt64. AFAICS there is only one user >>> in the AOSP for this API (libmedia). >>> >>> If you are referring to data blobs that application parses I don't >>> think there is anything we can do, even at libbinder level. >>> >>> Can you give me an example of the sort of problems you see? >>> >>> Thanks, >>> Tavi >> >> The specific problem I was told about can be found in >> frameworks/base/core/java/android/os/Bundle.java, but there could be >> other. The size of the bundle is stored in the parcel so the end of >> the bundle will be wrong if the bundle contains a flat_binder_object >> that the driver changes the size of. However, since the sending >> process gets the parcel size from libbinder, changing libbinder to >> always use the 64 bit version of flat_binder_object should work with >> this code. >> > > AFAICS the bundle size is stored as an int32 so there is no bit width > issues between the sending and receiving process. > > When I mentioned that flat_binder_objects will be translated, I meant > the whole transaction will be translated to preserve its user payload > intact. Something like this: > > 32bit64bit > ++++ > | o o oo oxx| -> | oo oo| > ++++ > > where o are binder objects, spaces are user data and x,y are offsets > pointers to binder objects (they are size_t so they need translation > as well). > > As long as the application does not use absolute offsets in the > payload and as long as the data types stored in the payload are fixed > bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of > intptr) doing the translation in kernel should be fine. I checked > libbinder and both assumptions seems to be true (except in a few cases > for the later which I already mentioned) > > So, what am I missing? The bundle size is wrong in the receiving process if the driver change the size of a flat_binder_object stored in the bundle. A simplified example where a flat_binder_object is a single pointer, and a bundle only adds a size to the data stored: If you send a bundle with an object and an int followed by an int from a 32 bit process to a 64 bit process you would get this transformation of the data (offsets not shown): bundle(obj, inta), int(intb) => parcel_32(8,obj,inta,intb) => parcel_64(8,objl,objh,inta,intb) => bundle(obj), int(inta), int(intb) The first bundle argument is missing an item in the receiving process, and the second argument is instead of . -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila octavian.purd...@intel.com wrote: On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg a...@android.com wrote: On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila octavian.purd...@intel.com wrote: On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg a...@android.com wrote: Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. Hi Arve, Yes, I was talking about translating flat_binder_objects. I understand the potential issue for the user data payload, however, since most applications will use libbinder, the only problematic case is readIntPtr/writeIntPtr, which we can deprecate and convert applications that use it to readInt64. AFAICS there is only one user in the AOSP for this API (libmedia). If you are referring to data blobs that application parses I don't think there is anything we can do, even at libbinder level. Can you give me an example of the sort of problems you see? Thanks, Tavi The specific problem I was told about can be found in frameworks/base/core/java/android/os/Bundle.java, but there could be other. The size of the bundle is stored in the parcel so the end of the bundle will be wrong if the bundle contains a flat_binder_object that the driver changes the size of. However, since the sending process gets the parcel size from libbinder, changing libbinder to always use the 64 bit version of flat_binder_object should work with this code. AFAICS the bundle size is stored as an int32 so there is no bit width issues between the sending and receiving process. When I mentioned that flat_binder_objects will be translated, I meant the whole transaction will be translated to preserve its user payload intact. Something like this: 32bit64bit ++++ | o o oo oxx| - | oo oo| ++++ where o are binder objects, spaces are user data and x,y are offsets pointers to binder objects (they are size_t so they need translation as well). As long as the application does not use absolute offsets in the payload and as long as the data types stored in the payload are fixed bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of intptr) doing the translation in kernel should be fine. I checked libbinder and both assumptions seems to be true (except in a few cases for the later which I already mentioned) So, what am I missing? The bundle size is wrong in the receiving process if the driver change the size of a flat_binder_object stored in the bundle. A simplified example where a flat_binder_object is a single pointer, and a bundle only adds a size to the data stored: If you send a bundle with an object and an int inta followed by an int intb from a 32 bit process to a 64 bit process you would get this transformation of the data (offsets not shown): bundle(obj, inta), int(intb) = parcel_32(8,obj,inta,intb) = parcel_64(8,objl,objh,inta,intb) = bundle(obj), int(inta), int(intb) The first bundle argument is missing an item in the receiving process, and the second argument is inta instead of intb. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila wrote: > On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: >> >> Assuming you are talking about a kernel compat layer that translates >> the flat_binder_object structs as they pass between 32 bit and 64 bit >> processes, that will not always work. The data portion of the message >> sometimes contain size values that are invisible to the kernel, but >> these values will be wrong if the kernel move data to make room for a >> different size flat_binder_object. >> > > Hi Arve, > > Yes, I was talking about translating flat_binder_objects. > > I understand the potential issue for the user data payload, however, > since most applications will use libbinder, the only problematic case > is readIntPtr/writeIntPtr, which we can deprecate and convert > applications that use it to readInt64. AFAICS there is only one user > in the AOSP for this API (libmedia). > > If you are referring to data blobs that application parses I don't > think there is anything we can do, even at libbinder level. > > Can you give me an example of the sort of problems you see? > > Thanks, > Tavi The specific problem I was told about can be found in frameworks/base/core/java/android/os/Bundle.java, but there could be other. The size of the bundle is stored in the parcel so the end of the bundle will be wrong if the bundle contains a flat_binder_object that the driver changes the size of. However, since the sending process gets the parcel size from libbinder, changing libbinder to always use the 64 bit version of flat_binder_object should work with this code. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila octavian.purd...@intel.com wrote: On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg a...@android.com wrote: Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. Hi Arve, Yes, I was talking about translating flat_binder_objects. I understand the potential issue for the user data payload, however, since most applications will use libbinder, the only problematic case is readIntPtr/writeIntPtr, which we can deprecate and convert applications that use it to readInt64. AFAICS there is only one user in the AOSP for this API (libmedia). If you are referring to data blobs that application parses I don't think there is anything we can do, even at libbinder level. Can you give me an example of the sort of problems you see? Thanks, Tavi The specific problem I was told about can be found in frameworks/base/core/java/android/os/Bundle.java, but there could be other. The size of the bundle is stored in the parcel so the end of the bundle will be wrong if the bundle contains a flat_binder_object that the driver changes the size of. However, since the sending process gets the parcel size from libbinder, changing libbinder to always use the 64 bit version of flat_binder_object should work with this code. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Mon, Dec 9, 2013 at 7:01 PM, Octavian Purdila wrote: > On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg wrote: >> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH wrote: >>> On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: >>>> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH wrote: >>>> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: >>>> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH >>>> >> wrote: >>>> >> >>>> >> >>>> >> > And finally, is this all really needed? Why not just fix the >>>> >> > structures >>>> >> > to be "correct", and then fix userspace to use the correct structures >>>> >> > as >>>> >> > well, thereby not needing a compat layer at all? >>>> >> >>>> >> Some of the binder ioctls take userspace pointers. Are you suggesting >>>> >> storing those pointers in a __u64 to avoid having to have a >>>> >> compat_ioctl? >>>> > >>>> > Yes, that's the best way to solve the issue, right? >>>> >>>> It's the least code, but in exchange you lose all the type safety and >>>> warnings when copying in and out of the pointers, as well as sparse >>>> checking on the __user attribute. >>> >>> Not if you make the cast right at the beginning, when you first "touch" >>> the data, but yes, it does take some of the type saftey away, at the >>> expense of simpler code to mess up :) >>> >>>> That doesn't seem like a good tradeoff to me. In addition it requires >>>> modifying the existing heavily used 32 bit api, which means a >>>> mostly-equivalent compat layer added in libbinder to support old >>>> kernels. >>> >>> Wait, I thought that libbinder would have to be changed anyway here, to >>> handle 64bit kernels (in both 32 and 64bit userspace). Since you are >>> already changing it, why not just "do it correctly"? >>> >> >> Yes libbinder will have to be changed to support calls between 32 bit >> and 64 bit processes, so I don't see much value in a patchset that >> only supports all 32 bit or all 64 bit processes. If user space is >> fixed to use 64 bit pointers on a 64 bit system, then much of the code >> added in this patchset becomes useless (and probably harmful as it >> appears to prevent 32 bit processes from communicating with 64 bit >> processes). >> > > Hi, > > Coincidentally, I have been working on a compat layer myself lately. > It is implemented in the binder driver with no changes in libbinder > and it includes support for mixed mode. > > Unless you think that the kernel compat layer is a dead end, I can > post the patches here for review. IMO the kernel compat layer gives > you greater flexibility because it keeps the 32bit ABI unchanged. Of > course it comes with the price of increased complexity. > > Thanks, > Tavi Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Mon, Dec 9, 2013 at 7:01 PM, Octavian Purdila tavi.purd...@gmail.com wrote: On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg a...@android.com wrote: On Wed, Dec 4, 2013 at 2:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? Yes libbinder will have to be changed to support calls between 32 bit and 64 bit processes, so I don't see much value in a patchset that only supports all 32 bit or all 64 bit processes. If user space is fixed to use 64 bit pointers on a 64 bit system, then much of the code added in this patchset becomes useless (and probably harmful as it appears to prevent 32 bit processes from communicating with 64 bit processes). Hi, Coincidentally, I have been working on a compat layer myself lately. It is implemented in the binder driver with no changes in libbinder and it includes support for mixed mode. Unless you think that the kernel compat layer is a dead end, I can post the patches here for review. IMO the kernel compat layer gives you greater flexibility because it keeps the 32bit ABI unchanged. Of course it comes with the price of increased complexity. Thanks, Tavi Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH wrote: > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH wrote: >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH >> >> wrote: >> >> >> >> >> >> > And finally, is this all really needed? Why not just fix the structures >> >> > to be "correct", and then fix userspace to use the correct structures as >> >> > well, thereby not needing a compat layer at all? >> >> >> >> Some of the binder ioctls take userspace pointers. Are you suggesting >> >> storing those pointers in a __u64 to avoid having to have a >> >> compat_ioctl? >> > >> > Yes, that's the best way to solve the issue, right? >> >> It's the least code, but in exchange you lose all the type safety and >> warnings when copying in and out of the pointers, as well as sparse >> checking on the __user attribute. > > Not if you make the cast right at the beginning, when you first "touch" > the data, but yes, it does take some of the type saftey away, at the > expense of simpler code to mess up :) > >> That doesn't seem like a good tradeoff to me. In addition it requires >> modifying the existing heavily used 32 bit api, which means a >> mostly-equivalent compat layer added in libbinder to support old >> kernels. > > Wait, I thought that libbinder would have to be changed anyway here, to > handle 64bit kernels (in both 32 and 64bit userspace). Since you are > already changing it, why not just "do it correctly"? > Yes libbinder will have to be changed to support calls between 32 bit and 64 bit processes, so I don't see much value in a patchset that only supports all 32 bit or all 64 bit processes. If user space is fixed to use 64 bit pointers on a 64 bit system, then much of the code added in this patchset becomes useless (and probably harmful as it appears to prevent 32 bit processes from communicating with 64 bit processes). > Or does this patch series mean that no userspace code is changed? Is > that a "requirement" here? > I don't think we need to support old 32 bit userspace framework code on a 64 bit system. I think it is more important to not prevent mixed mode systems. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? Yes libbinder will have to be changed to support calls between 32 bit and 64 bit processes, so I don't see much value in a patchset that only supports all 32 bit or all 64 bit processes. If user space is fixed to use 64 bit pointers on a 64 bit system, then much of the code added in this patchset becomes useless (and probably harmful as it appears to prevent 32 bit processes from communicating with 64 bit processes). Or does this patch series mean that no userspace code is changed? Is that a requirement here? I don't think we need to support old 32 bit userspace framework code on a 64 bit system. I think it is more important to not prevent mixed mode systems. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 0/6] Android Binder IPC Fixes
On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu wrote: > Hi all, > > Any feedback or comments on this patch set? > > Thanks, > Serban > The new patches look OK, but I would like to also see the patches that add support for 32 bit user-space on a 64 bit kernel. > > On 19/06/13 18:12, Serban Constantinescu wrote: >> >> Hi all, >> >> This set of patches will clean-up and fix some of the issues that arise >> with the current binder interface when moving to a 64bit kernel. All these >> changes will not affect the existing 32bit Android interface and are meant >> to stand as the base for the 64bit binder compat layer(kernel or >> userpsace). >> >> The patch set has been successfully tested with a 64bit Linux userspace >> and >> 64bit binder unit-tests. >> >> This patch set has been successfully tested on 32bit platforms(ARMv7 >> VExpress) >> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an >> in >> kernel binder compat layer. >> >> Changes for v5: >> 1 6/6: Moved patch to the end of the series; changed handle to use __u32 >> type >> 2 4/6: Removed some of the alignment/buffer changes introduced in previous >> versions of the patch. >> >> Changes for v4: >> 1: 5/6: Fix the offset buffer alignment such that it will work for cases >> where >> buffer start + buffer size are not aligned to (void *) >> >> Changes for v3: >> 1: Dropped the patch that was replacing uint32_t types with unsigned int >> 2: Dropped the patch fixing the IOCTL types(since it has been added to >> Greg's >> staging tree) >> 3: Split one patch into two: 'modify binder_write_read' and '64bit >> changes' >> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's >> review >> 5: Modified the binder command IOCTL declarations according to Arve's >> review >> >> Changes for v2: >> 1: 1/7: Modified the commit message according to Greg's feedback; >> 2: 3/7: Merged with the patch fixing the printk format specifiers. >> >> Serban Constantinescu (6): >>staging: android: binder: modify struct binder_write_read to use >> size_t >>staging: android: binder: fix BINDER_SET_MAX_THREADS declaration >>staging: android: binder: fix BC_FREE_BUFFER ioctl declaration >>staging: android: binder: fix alignment issues >>staging: android: binder: replace types with portable ones >>staging: android: binder: fix binder interface for 64bit compat layer >> >> drivers/staging/android/binder.c | 32 - >> drivers/staging/android/binder.h | 48 >> +++--- >> 2 files changed, 40 insertions(+), 40 deletions(-) >> > > > -- > Best Regards, > > Serban Constantinescu > PDSW Engineer ARM Ltd. > -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 6/6] staging: android: binder: fix binder interface for 64bit compat layer
604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
>
> file = fget(fp->handle);
> if (file == NULL) {
> - binder_user_error("%d:%d got transaction with
> invalid fd, %ld\n",
> + binder_user_error("%d:%d got transaction with
> invalid fd, %d\n",
> proc->pid, thread->pid, fp->handle);
> return_error = BR_FAILED_REPLY;
> goto err_fget_failed;
> @@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc
> *proc,
> task_fd_install(target_proc, target_fd, file);
> trace_binder_transaction_fd(t, fp->handle, target_fd);
> binder_debug(BINDER_DEBUG_TRANSACTION,
> -"fd %ld -> %d\n", fp->handle,
> target_fd);
> +"fd %d -> %d\n", fp->handle,
> target_fd);
> /* TODO: fput? */
> fp->handle = target_fd;
> } break;
>
> default:
> - binder_user_error("%d:%d got transaction with invalid
> object type, %lx\n",
> + binder_user_error("%d:%d got transaction with invalid
> object type, %x\n",
> proc->pid, thread->pid, fp->type);
> return_error = BR_FAILED_REPLY;
> goto err_bad_object_type;
> @@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> goto err;
> }
> binder_debug(BINDER_DEBUG_READ_WRITE,
> -"%d:%d write %zd at %08lx, read %zd at %08lx\n",
> +"%d:%d write %zd at %016lx, read %zd at
> %016lx\n",
> proc->pid, thread->pid, bwr.write_size,
> bwr.write_buffer, bwr.read_size,
> bwr.read_buffer);
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index dadfce0..b88b263 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -48,13 +48,13 @@ enum {
> */
> struct flat_binder_object {
> /* 8 bytes for large_flat_header. */
> - unsigned long type;
> - unsigned long flags;
> + __u32 type;
> + __u32 flags;
>
> /* 8 bytes of data. */
> union {
> void __user *binder;/* local object */
> - signed long handle; /* remote object */
> + __u32 handle; /* remote object */
> };
>
> /* extra data associated with local object */
> @@ -78,7 +78,7 @@ struct binder_write_read {
> /* Use with BINDER_VERSION, driver fills in fields. */
> struct binder_version {
> /* driver protocol version -- increment with incompatible change */
> - signed long protocol_version;
> + __s32 protocol_version;
> };
>
> /* This is the current protocol version. */
> @@ -119,7 +119,7 @@ struct binder_transaction_data {
> * identifying the target and contents of the transaction.
> */
> union {
> - size_t handle; /* target descriptor of command transaction */
> + __u32 handle; /* target descriptor of command transaction */
> void*ptr; /* target descriptor of return transaction */
> } target;
> void*cookie;/* target object cookie */
> --
> 1.7.9.5
>
Acked-by: Arve Hjønnevåg
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 5/6] staging: android: binder: replace types with portable ones
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
wrote:
> Since this driver is meant to be used on different types of processors
> and a portable driver should specify the size a variable expects to be
> this patch changes the types used throughout the binder interface.
>
> We use "userspace" types since this header will be exported and used by
> the Android filesystem.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu
> Acked-by: Arve Hjønnevåg
> ---
> drivers/staging/android/binder.h | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index b55bba9..dadfce0 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -123,10 +123,10 @@ struct binder_transaction_data {
> void*ptr; /* target descriptor of return transaction */
> } target;
> void*cookie;/* target object cookie */
> - unsigned intcode; /* transaction command */
> + __u32 code; /* transaction command */
>
> /* General information about the transaction. */
> - unsigned intflags;
> + __u32 flags;
> pid_t sender_pid;
> uid_t sender_euid;
> size_t data_size; /* number of bytes of data */
> @@ -143,7 +143,7 @@ struct binder_transaction_data {
> /* offsets from buffer to flat_binder_object structs
> */
> const void __user *offsets;
> } ptr;
> - uint8_t buf[8];
> + __u8buf[8];
> } data;
> };
>
> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
> };
>
> struct binder_pri_desc {
> - int priority;
> - int desc;
> + __s32 priority;
> + __s32 desc;
desc should be __u32 to be consistent with the other changes you are
making in this and the next patch.
> };
>
> struct binder_pri_ptr_cookie {
> - int priority;
> + __s32 priority;
> void *ptr;
> void *cookie;
> };
>
> enum binder_driver_return_protocol {
> - BR_ERROR = _IOR('r', 0, int),
> + BR_ERROR = _IOR('r', 0, __s32),
> /*
> * int: error code
> */
> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
> * binder_transaction_data: the received command.
> */
>
> - BR_ACQUIRE_RESULT = _IOR('r', 4, int),
> + BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
> /*
> * not currently supported
> * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
> * binder_transaction_data: the sent command.
> */
>
> - BC_ACQUIRE_RESULT = _IOW('c', 2, int),
> + BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
> /*
> * not currently supported
> * int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
> * void *: ptr to transaction data received on a read
> */
>
> - BC_INCREFS = _IOW('c', 4, int),
> - BC_ACQUIRE = _IOW('c', 5, int),
> - BC_RELEASE = _IOW('c', 6, int),
> - BC_DECREFS = _IOW('c', 7, int),
> + BC_INCREFS = _IOW('c', 4, __u32),
> + BC_ACQUIRE = _IOW('c', 5, __u32),
> + BC_RELEASE = _IOW('c', 6, __u32),
> + BC_DECREFS = _IOW('c', 7, __u32),
> /*
> * int: descriptor
> */
> --
> 1.7.9.5
>
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/6] staging: android: binder: fix alignment issues
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
wrote:
> The Android userspace aligns the data written to the binder buffers to
> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
> Android userspace we can have a buffer looking like this:
>
> platformbuffer(binder_cmd pointer) size
> 32/32 32b 32b 8B
> 64/32 32b 64b 12B
> 64/64 32b 64b 12B
>
> Thus the kernel needs to check that the buffer size is aligned to 4bytes
> not to (void *) that will be 8bytes on 64bit machines.
>
> The change does not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c
> b/drivers/staging/android/binder.c
> index ce70909..7450d56 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct
> binder_proc *proc,
> struct flat_binder_object *fp;
> if (*offp > buffer->data_size - sizeof(*fp) ||
> buffer->data_size < sizeof(*fp) ||
> - !IS_ALIGNED(*offp, sizeof(void *))) {
> + !IS_ALIGNED(*offp, sizeof(u32))) {
> pr_err("transaction release %d bad offset %zd, size
> %zd\n",
> debug_id, *offp, buffer->data_size);
> continue;
> @@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
> struct flat_binder_object *fp;
> if (*offp > t->buffer->data_size - sizeof(*fp) ||
> t->buffer->data_size < sizeof(*fp) ||
> - !IS_ALIGNED(*offp, sizeof(void *))) {
> + !IS_ALIGNED(*offp, sizeof(u32))) {
> binder_user_error("%d:%d got transaction with invalid
> offset, %zd\n",
> proc->pid, thread->pid, *offp);
> return_error = BR_FAILED_REPLY;
> --
> 1.7.9.5
>
Acked-by: Arve Hjønnevåg
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/6] staging: android: binder: fix alignment issues
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
The Android userspace aligns the data written to the binder buffers to
4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
Android userspace we can have a buffer looking like this:
platformbuffer(binder_cmd pointer) size
32/32 32b 32b 8B
64/32 32b 64b 12B
64/64 32b 64b 12B
Thus the kernel needs to check that the buffer size is aligned to 4bytes
not to (void *) that will be 8bytes on 64bit machines.
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index ce70909..7450d56 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct
binder_proc *proc,
struct flat_binder_object *fp;
if (*offp buffer-data_size - sizeof(*fp) ||
buffer-data_size sizeof(*fp) ||
- !IS_ALIGNED(*offp, sizeof(void *))) {
+ !IS_ALIGNED(*offp, sizeof(u32))) {
pr_err(transaction release %d bad offset %zd, size
%zd\n,
debug_id, *offp, buffer-data_size);
continue;
@@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
struct flat_binder_object *fp;
if (*offp t-buffer-data_size - sizeof(*fp) ||
t-buffer-data_size sizeof(*fp) ||
- !IS_ALIGNED(*offp, sizeof(void *))) {
+ !IS_ALIGNED(*offp, sizeof(u32))) {
binder_user_error(%d:%d got transaction with invalid
offset, %zd\n,
proc-pid, thread-pid, *offp);
return_error = BR_FAILED_REPLY;
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 5/6] staging: android: binder: replace types with portable ones
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
Since this driver is meant to be used on different types of processors
and a portable driver should specify the size a variable expects to be
this patch changes the types used throughout the binder interface.
We use userspace types since this header will be exported and used by
the Android filesystem.
The patch does not change in any way the functionality of the binder driver.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
Acked-by: Arve Hjønnevåg a...@android.com
---
drivers/staging/android/binder.h | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index b55bba9..dadfce0 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -123,10 +123,10 @@ struct binder_transaction_data {
void*ptr; /* target descriptor of return transaction */
} target;
void*cookie;/* target object cookie */
- unsigned intcode; /* transaction command */
+ __u32 code; /* transaction command */
/* General information about the transaction. */
- unsigned intflags;
+ __u32 flags;
pid_t sender_pid;
uid_t sender_euid;
size_t data_size; /* number of bytes of data */
@@ -143,7 +143,7 @@ struct binder_transaction_data {
/* offsets from buffer to flat_binder_object structs
*/
const void __user *offsets;
} ptr;
- uint8_t buf[8];
+ __u8buf[8];
} data;
};
@@ -153,18 +153,18 @@ struct binder_ptr_cookie {
};
struct binder_pri_desc {
- int priority;
- int desc;
+ __s32 priority;
+ __s32 desc;
desc should be __u32 to be consistent with the other changes you are
making in this and the next patch.
};
struct binder_pri_ptr_cookie {
- int priority;
+ __s32 priority;
void *ptr;
void *cookie;
};
enum binder_driver_return_protocol {
- BR_ERROR = _IOR('r', 0, int),
+ BR_ERROR = _IOR('r', 0, __s32),
/*
* int: error code
*/
@@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
* binder_transaction_data: the received command.
*/
- BR_ACQUIRE_RESULT = _IOR('r', 4, int),
+ BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
/*
* not currently supported
* int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
@@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
* binder_transaction_data: the sent command.
*/
- BC_ACQUIRE_RESULT = _IOW('c', 2, int),
+ BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
/*
* not currently supported
* int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
@@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
* void *: ptr to transaction data received on a read
*/
- BC_INCREFS = _IOW('c', 4, int),
- BC_ACQUIRE = _IOW('c', 5, int),
- BC_RELEASE = _IOW('c', 6, int),
- BC_DECREFS = _IOW('c', 7, int),
+ BC_INCREFS = _IOW('c', 4, __u32),
+ BC_ACQUIRE = _IOW('c', 5, __u32),
+ BC_RELEASE = _IOW('c', 6, __u32),
+ BC_DECREFS = _IOW('c', 7, __u32),
/*
* int: descriptor
*/
--
1.7.9.5
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 6/6] staging: android: binder: fix binder interface for 64bit compat layer
err_fget_failed;
@@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc
*proc,
task_fd_install(target_proc, target_fd, file);
trace_binder_transaction_fd(t, fp-handle, target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
-fd %ld - %d\n, fp-handle,
target_fd);
+fd %d - %d\n, fp-handle,
target_fd);
/* TODO: fput? */
fp-handle = target_fd;
} break;
default:
- binder_user_error(%d:%d got transaction with invalid
object type, %lx\n,
+ binder_user_error(%d:%d got transaction with invalid
object type, %x\n,
proc-pid, thread-pid, fp-type);
return_error = BR_FAILED_REPLY;
goto err_bad_object_type;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
int cmd, unsigned long arg)
goto err;
}
binder_debug(BINDER_DEBUG_READ_WRITE,
-%d:%d write %zd at %08lx, read %zd at %08lx\n,
+%d:%d write %zd at %016lx, read %zd at
%016lx\n,
proc-pid, thread-pid, bwr.write_size,
bwr.write_buffer, bwr.read_size,
bwr.read_buffer);
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index dadfce0..b88b263 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;
/* 8 bytes of data. */
union {
void __user *binder;/* local object */
- signed long handle; /* remote object */
+ __u32 handle; /* remote object */
};
/* extra data associated with local object */
@@ -78,7 +78,7 @@ struct binder_write_read {
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
};
/* This is the current protocol version. */
@@ -119,7 +119,7 @@ struct binder_transaction_data {
* identifying the target and contents of the transaction.
*/
union {
- size_t handle; /* target descriptor of command transaction */
+ __u32 handle; /* target descriptor of command transaction */
void*ptr; /* target descriptor of return transaction */
} target;
void*cookie;/* target object cookie */
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 0/6] Android Binder IPC Fixes
On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu serban.constantine...@arm.com wrote: Hi all, Any feedback or comments on this patch set? Thanks, Serban The new patches look OK, but I would like to also see the patches that add support for 32 bit user-space on a 64 bit kernel. On 19/06/13 18:12, Serban Constantinescu wrote: Hi all, This set of patches will clean-up and fix some of the issues that arise with the current binder interface when moving to a 64bit kernel. All these changes will not affect the existing 32bit Android interface and are meant to stand as the base for the 64bit binder compat layer(kernel or userpsace). The patch set has been successfully tested with a 64bit Linux userspace and 64bit binder unit-tests. This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress) and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in kernel binder compat layer. Changes for v5: 1 6/6: Moved patch to the end of the series; changed handle to use __u32 type 2 4/6: Removed some of the alignment/buffer changes introduced in previous versions of the patch. Changes for v4: 1: 5/6: Fix the offset buffer alignment such that it will work for cases where buffer start + buffer size are not aligned to (void *) Changes for v3: 1: Dropped the patch that was replacing uint32_t types with unsigned int 2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's staging tree) 3: Split one patch into two: 'modify binder_write_read' and '64bit changes' 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review 5: Modified the binder command IOCTL declarations according to Arve's review Changes for v2: 1: 1/7: Modified the commit message according to Greg's feedback; 2: 3/7: Merged with the patch fixing the printk format specifiers. Serban Constantinescu (6): staging: android: binder: modify struct binder_write_read to use size_t staging: android: binder: fix BINDER_SET_MAX_THREADS declaration staging: android: binder: fix BC_FREE_BUFFER ioctl declaration staging: android: binder: fix alignment issues staging: android: binder: replace types with portable ones staging: android: binder: fix binder interface for 64bit compat layer drivers/staging/android/binder.c | 32 - drivers/staging/android/binder.h | 48 +++--- 2 files changed, 40 insertions(+), 40 deletions(-) -- Best Regards, Serban Constantinescu PDSW Engineer ARM Ltd. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC][PATCH] kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on
The defconfig and Kconfig combination below, which is based on 3.10-rc4
Kconfigs, resulted in several options getting set to "m" instead of "y".
defconfig:
---
CONFIG_MODULES=y
CONFIG_USB_GADGET=y
CONFIG_USB_ZERO=y
---
Kconfig:
---
menuconfig MODULES
bool "Enable loadable module support"
config CONFIGFS_FS
tristate "Userspace-driven configuration filesystem"
config OCFS2_FS
tristate "OCFS2 file system support"
depends on CONFIGFS_FS
select CRC32
config USB_LIBCOMPOSITE
tristate
select CONFIGFS_FS
choice
tristate "USB Gadget Drivers"
default USB_ETH
config USB_ZERO
tristate "Gadget Zero (DEVELOPMENT)"
select USB_LIBCOMPOSITE
config USB_ETH
tristate "Ethernet Gadget (with CDC Ethernet support)"
select USB_LIBCOMPOSITE
endchoice
config CRC32
tristate "CRC32/CRC32c functions"
default y
choice
prompt "CRC32 implementation"
depends on CRC32
default CRC32_SLICEBY8
config CRC32_SLICEBY8
bool "Slice by 8 bytes"
endchoice
---
Signed-off-by: Arve Hjønnevåg
---
scripts/kconfig/confdata.c | 14 ++
scripts/kconfig/expr.h | 3 +++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/symbol.c | 11 +++
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 43eda40..35e0f16 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1083,7 +1083,7 @@ static void randomize_choice_values(struct symbol *csym)
csym->flags &= ~(SYMBOL_VALID);
}
-static void set_all_choice_values(struct symbol *csym)
+void set_all_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1100,7 +1100,7 @@ static void set_all_choice_values(struct symbol *csym)
}
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
- csym->flags &= ~(SYMBOL_VALID);
+ csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
void conf_set_all_new_symbols(enum conf_def_mode mode)
@@ -1202,6 +1202,14 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
* selected in a choice block and we set it to yes,
* and the rest to no.
*/
+ if (mode != def_random) {
+ for_all_symbols(i, csym) {
+ if ((sym_is_choice(csym) && !sym_has_value(csym)) ||
+ sym_is_choice_value(csym))
+ csym->flags |= SYMBOL_NEED_SET_CHOICE_VALUES;
+ }
+ }
+
for_all_symbols(i, csym) {
if (sym_has_value(csym) || !sym_is_choice(csym))
continue;
@@ -1209,7 +1217,5 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
randomize_choice_values(csym);
- else
- set_all_choice_values(csym);
}
}
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index cdd4860..df198a5 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -106,6 +106,9 @@ struct symbol {
#define SYMBOL_DEF3 0x4 /* symbol.def[S_DEF_3] is valid */
#define SYMBOL_DEF4 0x8 /* symbol.def[S_DEF_4] is valid */
+/* choice values need to be set before calculating this symbol value */
+#define SYMBOL_NEED_SET_CHOICE_VALUES 0x10
+
#define SYMBOL_MAXLENGTH 256
#define SYMBOL_HASHSIZE9973
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index f8aee5f..0c8d419 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -87,6 +87,7 @@ char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
void conf_set_all_new_symbols(enum conf_def_mode mode);
+void set_all_choice_values(struct symbol *csym);
struct conf_printer {
void (*print_symbol)(FILE *, struct symbol *, const char *, void *);
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ecc5aa5..ab8f4c8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -300,6 +300,14 @@ void sym_calc_value(struct symbol *sym)
if (sym->flags & SYMBOL_VALID)
return;
+
+ if (sym_is_choice_value(sym) &&
+ sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES) {
+ sym->flags &= ~SYMBOL_NEED_SET_CHOICE_VALUES;
+ prop = sym_get_choice_prop(sym);
+ sym_calc_value(prop_get_symbol(prop));
+ }
+
sym->flags |= SYMBOL_VALID;
oldval = sym->curr;
@@ -425,6 +433,9 @@ void sym_calc_value(struct symbol *sym)
if (sym
[RFC][PATCH] kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on
The defconfig and Kconfig combination below, which is based on 3.10-rc4
Kconfigs, resulted in several options getting set to m instead of y.
defconfig:
---
CONFIG_MODULES=y
CONFIG_USB_GADGET=y
CONFIG_USB_ZERO=y
---
Kconfig:
---
menuconfig MODULES
bool Enable loadable module support
config CONFIGFS_FS
tristate Userspace-driven configuration filesystem
config OCFS2_FS
tristate OCFS2 file system support
depends on CONFIGFS_FS
select CRC32
config USB_LIBCOMPOSITE
tristate
select CONFIGFS_FS
choice
tristate USB Gadget Drivers
default USB_ETH
config USB_ZERO
tristate Gadget Zero (DEVELOPMENT)
select USB_LIBCOMPOSITE
config USB_ETH
tristate Ethernet Gadget (with CDC Ethernet support)
select USB_LIBCOMPOSITE
endchoice
config CRC32
tristate CRC32/CRC32c functions
default y
choice
prompt CRC32 implementation
depends on CRC32
default CRC32_SLICEBY8
config CRC32_SLICEBY8
bool Slice by 8 bytes
endchoice
---
Signed-off-by: Arve Hjønnevåg a...@android.com
---
scripts/kconfig/confdata.c | 14 ++
scripts/kconfig/expr.h | 3 +++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/symbol.c | 11 +++
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 43eda40..35e0f16 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1083,7 +1083,7 @@ static void randomize_choice_values(struct symbol *csym)
csym-flags = ~(SYMBOL_VALID);
}
-static void set_all_choice_values(struct symbol *csym)
+void set_all_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1100,7 +1100,7 @@ static void set_all_choice_values(struct symbol *csym)
}
csym-flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
- csym-flags = ~(SYMBOL_VALID);
+ csym-flags = ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
void conf_set_all_new_symbols(enum conf_def_mode mode)
@@ -1202,6 +1202,14 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
* selected in a choice block and we set it to yes,
* and the rest to no.
*/
+ if (mode != def_random) {
+ for_all_symbols(i, csym) {
+ if ((sym_is_choice(csym) !sym_has_value(csym)) ||
+ sym_is_choice_value(csym))
+ csym-flags |= SYMBOL_NEED_SET_CHOICE_VALUES;
+ }
+ }
+
for_all_symbols(i, csym) {
if (sym_has_value(csym) || !sym_is_choice(csym))
continue;
@@ -1209,7 +1217,5 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
randomize_choice_values(csym);
- else
- set_all_choice_values(csym);
}
}
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index cdd4860..df198a5 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -106,6 +106,9 @@ struct symbol {
#define SYMBOL_DEF3 0x4 /* symbol.def[S_DEF_3] is valid */
#define SYMBOL_DEF4 0x8 /* symbol.def[S_DEF_4] is valid */
+/* choice values need to be set before calculating this symbol value */
+#define SYMBOL_NEED_SET_CHOICE_VALUES 0x10
+
#define SYMBOL_MAXLENGTH 256
#define SYMBOL_HASHSIZE9973
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index f8aee5f..0c8d419 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -87,6 +87,7 @@ char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
void conf_set_all_new_symbols(enum conf_def_mode mode);
+void set_all_choice_values(struct symbol *csym);
struct conf_printer {
void (*print_symbol)(FILE *, struct symbol *, const char *, void *);
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ecc5aa5..ab8f4c8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -300,6 +300,14 @@ void sym_calc_value(struct symbol *sym)
if (sym-flags SYMBOL_VALID)
return;
+
+ if (sym_is_choice_value(sym)
+ sym-flags SYMBOL_NEED_SET_CHOICE_VALUES) {
+ sym-flags = ~SYMBOL_NEED_SET_CHOICE_VALUES;
+ prop = sym_get_choice_prop(sym);
+ sym_calc_value(prop_get_symbol(prop));
+ }
+
sym-flags |= SYMBOL_VALID;
oldval = sym-curr;
@@ -425,6 +433,9 @@ void sym_calc_value(struct symbol *sym)
if (sym-flags SYMBOL_AUTO)
sym-flags = ~SYMBOL_WRITE;
+
+ if (sym-flags SYMBOL_NEED_SET_CHOICE_VALUES)
+ set_all_choice_values(sym);
}
void sym_clear_all_valid(void)
--
1.8.3
Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
wrote:
> On 03/06/13 22:41, Arve Hjønnevåg wrote:
>>
>> On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
>> wrote:
>>>
>>> The changes in this patch will fix the binder interface for use on 64bit
>>> machines and stand as the base of the 64bit compat support. The changes
>>> apply to the structures that are passed between the kernel and
>>> userspace.
>>>
>>> Most of the changes applied mirror the change to struct binder_version
>>> where there is no need for a 64bit wide protocol_version(on 64bit
>>> machines). The change inlines with the existing 32bit userspace(the
>>> structure has the same size) and simplifies the compat layer such that
>>> the same handler can service the BINDER_VERSION ioctl.
>>>
>>> Other changes make use of kernel types as well as user-exportable ones
>>> and fix format specifier issues.
>>>
>>> The changes do not affect existing 32bit ABI.
>>>
>>> Signed-off-by: Serban Constantinescu
>>> ---
>>> drivers/staging/android/binder.c | 20 ++--
>>> drivers/staging/android/binder.h |8
>>> 2 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/android/binder.c
>>> b/drivers/staging/android/binder.c
>>> index ce70909..ca79084 100644
>>> --- a/drivers/staging/android/binder.c
>>> +++ b/drivers/staging/android/binder.c
>>> @@ -1271,7 +1271,7 @@ static void
>>> binder_transaction_buffer_release(struct binder_proc *proc,
>>> case BINDER_TYPE_WEAK_HANDLE: {
>>> struct binder_ref *ref = binder_get_ref(proc,
>>> fp->handle);
>>> if (ref == NULL) {
>>> - pr_err("transaction release %d bad handle
>>> %ld\n",
>>> + pr_err("transaction release %d bad handle
>>> %d\n",
>>> debug_id, fp->handle);
>>> break;
>>> }
>>> @@ -1283,13 +1283,13 @@ static void
>>> binder_transaction_buffer_release(struct binder_proc *proc,
>>>
>>> case BINDER_TYPE_FD:
>>> binder_debug(BINDER_DEBUG_TRANSACTION,
>>> -"fd %ld\n", fp->handle);
>>> +"fd %d\n", fp->handle);
>>> if (failed_at)
>>> task_close_fd(proc, fp->handle);
>>> break;
>>>
>>> default:
>>> - pr_err("transaction release %d bad object type
>>> %lx\n",
>>> + pr_err("transaction release %d bad object type
>>> %x\n",
>>> debug_id, fp->type);
>>> break;
>>> }
>>> @@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc
>>> *proc,
>>> case BINDER_TYPE_WEAK_HANDLE: {
>>> struct binder_ref *ref = binder_get_ref(proc,
>>> fp->handle);
>>> if (ref == NULL) {
>>> - binder_user_error("%d:%d got transaction
>>> with invalid handle, %ld\n",
>>> + binder_user_error("%d:%d got transaction
>>> with invalid handle, %d\n",
>>> proc->pid,
>>> thread->pid,
>>> fp->handle);
>>> return_error = BR_FAILED_REPLY;
>>> @@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc
>>> *proc,
>>>
>>> if (reply) {
>>> if (!(in_reply_to->flags &
>>> TF_ACCEPT_FDS)) {
>>> - binder_user_error("%d:%d got
>>> reply with fd, %ld, but target does not allow fds\n",
>>> + binder_user_error("%d:%d got
>>> reply with fd, %d, but target does not allow fds\n",
>>>
Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
On 03/06/13 22:41, Arve Hjønnevåg wrote:
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
The changes in this patch will fix the binder interface for use on 64bit
machines and stand as the base of the 64bit compat support. The changes
apply to the structures that are passed between the kernel and
userspace.
Most of the changes applied mirror the change to struct binder_version
where there is no need for a 64bit wide protocol_version(on 64bit
machines). The change inlines with the existing 32bit userspace(the
structure has the same size) and simplifies the compat layer such that
the same handler can service the BINDER_VERSION ioctl.
Other changes make use of kernel types as well as user-exportable ones
and fix format specifier issues.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 20 ++--
drivers/staging/android/binder.h |8
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index ce70909..ca79084 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1271,7 +1271,7 @@ static void
binder_transaction_buffer_release(struct binder_proc *proc,
case BINDER_TYPE_WEAK_HANDLE: {
struct binder_ref *ref = binder_get_ref(proc,
fp-handle);
if (ref == NULL) {
- pr_err(transaction release %d bad handle
%ld\n,
+ pr_err(transaction release %d bad handle
%d\n,
debug_id, fp-handle);
break;
}
@@ -1283,13 +1283,13 @@ static void
binder_transaction_buffer_release(struct binder_proc *proc,
case BINDER_TYPE_FD:
binder_debug(BINDER_DEBUG_TRANSACTION,
-fd %ld\n, fp-handle);
+fd %d\n, fp-handle);
if (failed_at)
task_close_fd(proc, fp-handle);
break;
default:
- pr_err(transaction release %d bad object type
%lx\n,
+ pr_err(transaction release %d bad object type
%x\n,
debug_id, fp-type);
break;
}
@@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc
*proc,
case BINDER_TYPE_WEAK_HANDLE: {
struct binder_ref *ref = binder_get_ref(proc,
fp-handle);
if (ref == NULL) {
- binder_user_error(%d:%d got transaction
with invalid handle, %ld\n,
+ binder_user_error(%d:%d got transaction
with invalid handle, %d\n,
proc-pid,
thread-pid,
fp-handle);
return_error = BR_FAILED_REPLY;
@@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc
*proc,
if (reply) {
if (!(in_reply_to-flags
TF_ACCEPT_FDS)) {
- binder_user_error(%d:%d got
reply with fd, %ld, but target does not allow fds\n,
+ binder_user_error(%d:%d got
reply with fd, %d, but target does not allow fds\n,
proc-pid, thread-pid,
fp-handle);
return_error = BR_FAILED_REPLY;
goto err_fd_not_allowed;
}
} else if (!target_node-accept_fds) {
- binder_user_error(%d:%d got transaction
with fd, %ld, but target does not allow fds\n,
+ binder_user_error(%d:%d got transaction
with fd, %d, but target does not allow fds\n,
proc-pid, thread-pid,
fp-handle);
return_error = BR_FAILED_REPLY;
goto err_fd_not_allowed;
@@ -1604,7 +1604,7 @@ static void binder_transaction(struct binder_proc
*proc,
file = fget(fp-handle);
if (file == NULL) {
- binder_user_error(%d:%d got transaction
with invalid fd, %ld\n,
+ binder_user_error(%d:%d got transaction
with invalid fd, %d\n
Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Mon, Jun 3, 2013 at 8:02 AM, Greg KH wrote:
> On Fri, May 31, 2013 at 04:17:34PM -0700, Arve Hjønnevåg wrote:
>> On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
>> wrote:
>> > This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
>> > instead of size_t for setting the max threads. Thus using the same
>> > handler for 32 and 64bit kernels.
>> >
>> > This value is stored internally in struct binder_proc and set to 15
>> > on open_binder() in the libbinder API(thus no need for a 64bit size_t
>> > on 64bit platforms).
>> >
>> > The change does not affect existing 32bit ABI.
>> >
>> > Signed-off-by: Serban Constantinescu
>> > ---
>> > drivers/staging/android/binder.h |2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/staging/android/binder.h
>> > b/drivers/staging/android/binder.h
>> > index 2f94d16..1761541 100644
>> > --- a/drivers/staging/android/binder.h
>> > +++ b/drivers/staging/android/binder.h
>> > @@ -86,7 +86,7 @@ struct binder_version {
>> >
>> > #define BINDER_WRITE_READ _IOWR('b', 1, struct
>> > binder_write_read)
>> > #defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
>> > -#defineBINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
>> > +#defineBINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
>> > #defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, __s32)
>> > #defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
>> > #defineBINDER_THREAD_EXIT _IOW('b', 8, __s32)
>> > --
>> > 1.7.9.5
>> >
>>
>> Acked-by: Arve Hjønnevåg
>
> What about patches 1 and 2 in this series?
>
I apparently only responded privately to those. I just resent them.
> thanks,
>
> greg k-h
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
wrote:
> This change mirrors the userspace operation where struct binder_write_read
> members that specify the buffer size and consumed size are size_t elements.
>
> The patch also fixes the binder_thread_write() and binder_thread_read()
> functions prototypes to conform with the definition of binder_write_read.
>
> The changes do not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.c | 10 +-
> drivers/staging/android/binder.h |8
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c
> b/drivers/staging/android/binder.c
> index 1567ac2..ce70909 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
> }
>
> int binder_thread_write(struct binder_proc *proc, struct binder_thread
> *thread,
> - void __user *buffer, int size, signed long *consumed)
> + void __user *buffer, size_t size, size_t *consumed)
> {
> uint32_t cmd;
> void __user *ptr = buffer + *consumed;
> @@ -2080,8 +2080,8 @@ static int binder_has_thread_work(struct binder_thread
> *thread)
>
> static int binder_thread_read(struct binder_proc *proc,
> struct binder_thread *thread,
> - void __user *buffer, int size,
> - signed long *consumed, int non_block)
> + void __user *buffer, size_t size,
> + size_t *consumed, int non_block)
> {
> void __user *ptr = buffer + *consumed;
> void __user *end = buffer + size;
> @@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> goto err;
> }
> binder_debug(BINDER_DEBUG_READ_WRITE,
> -"%d:%d write %ld at %08lx, read %ld at %08lx\n",
> +"%d:%d write %zd at %08lx, read %zd at %08lx\n",
> proc->pid, thread->pid, bwr.write_size,
> bwr.write_buffer, bwr.read_size,
> bwr.read_buffer);
>
> @@ -2604,7 +2604,7 @@ static long binder_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> }
> }
> binder_debug(BINDER_DEBUG_READ_WRITE,
> -"%d:%d wrote %ld of %ld, read return %ld of
> %ld\n",
> +"%d:%d wrote %zd of %zd, read return %zd of
> %zd\n",
> proc->pid, thread->pid, bwr.write_consumed,
> bwr.write_size,
> bwr.read_consumed, bwr.read_size);
> if (copy_to_user(ubuf, , sizeof(bwr))) {
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index dbe81ce..edab249 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -67,11 +67,11 @@ struct flat_binder_object {
> */
>
> struct binder_write_read {
> - signed long write_size; /* bytes to write */
> - signed long write_consumed; /* bytes consumed by driver */
> + size_t write_size; /* bytes to write */
> + size_t write_consumed; /* bytes consumed by driver */
> unsigned long write_buffer;
> - signed long read_size; /* bytes to read */
> - signed long read_consumed; /* bytes consumed by driver */
> + size_t read_size; /* bytes to read */
> + size_t read_consumed; /* bytes consumed by driver */
> unsigned long read_buffer;
> };
>
> --
> 1.7.9.5
>
Acked-by: Arve Hjønnevåg
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
>
> file = fget(fp->handle);
> if (file == NULL) {
> - binder_user_error("%d:%d got transaction with
> invalid fd, %ld\n",
> + binder_user_error("%d:%d got transaction with
> invalid fd, %d\n",
> proc->pid, thread->pid, fp->handle);
> return_error = BR_FAILED_REPLY;
> goto err_fget_failed;
> @@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc
> *proc,
> task_fd_install(target_proc, target_fd, file);
> trace_binder_transaction_fd(t, fp->handle, target_fd);
> binder_debug(BINDER_DEBUG_TRANSACTION,
> -"fd %ld -> %d\n", fp->handle,
> target_fd);
> +"fd %d -> %d\n", fp->handle,
> target_fd);
> /* TODO: fput? */
> fp->handle = target_fd;
> } break;
>
> default:
> - binder_user_error("%d:%d got transaction with invalid
> object type, %lx\n",
> + binder_user_error("%d:%d got transaction with invalid
> object type, %x\n",
> proc->pid, thread->pid, fp->type);
> return_error = BR_FAILED_REPLY;
> goto err_bad_object_type;
> @@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> goto err;
> }
> binder_debug(BINDER_DEBUG_READ_WRITE,
> -"%d:%d write %zd at %08lx, read %zd at %08lx\n",
> +"%d:%d write %zd at %016lx, read %zd at
> %016lx\n",
> proc->pid, thread->pid, bwr.write_size,
> bwr.write_buffer, bwr.read_size,
> bwr.read_buffer);
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index edab249..2f94d16 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -48,13 +48,13 @@ enum {
> */
> struct flat_binder_object {
> /* 8 bytes for large_flat_header. */
> - unsigned long type;
> - unsigned long flags;
> + __u32 type;
> + __u32 flags;
>
> /* 8 bytes of data. */
> union {
> void __user *binder;/* local object */
> - signed long handle; /* remote object */
> + __s32 handle; /* remote object */
This should be unsigned to match the handle in binder_transaction_data
and other uses in the driver, but it is currently also used to pass
file descriptors. Perhaps this is better (if sou also change size of
the handle in binder_transaction_data to match):
__u32 handle; /* remote object */
__s32 fd; /* file descriptor */
> };
>
> /* extra data associated with local object */
> @@ -78,7 +78,7 @@ struct binder_write_read {
> /* Use with BINDER_VERSION, driver fills in fields. */
> struct binder_version {
> /* driver protocol version -- increment with incompatible change */
> - signed long protocol_version;
> + __s32 protocol_version;
> };
>
> /* This is the current protocol version. */
> --
> 1.7.9.5
>
I still think the protocol_version size change on 64 bit systems
should go after all your other changes that affect 64 bits systems.
That way you don't have to change the protocol version later.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
err_fget_failed;
@@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc
*proc,
task_fd_install(target_proc, target_fd, file);
trace_binder_transaction_fd(t, fp-handle, target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
-fd %ld - %d\n, fp-handle,
target_fd);
+fd %d - %d\n, fp-handle,
target_fd);
/* TODO: fput? */
fp-handle = target_fd;
} break;
default:
- binder_user_error(%d:%d got transaction with invalid
object type, %lx\n,
+ binder_user_error(%d:%d got transaction with invalid
object type, %x\n,
proc-pid, thread-pid, fp-type);
return_error = BR_FAILED_REPLY;
goto err_bad_object_type;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
int cmd, unsigned long arg)
goto err;
}
binder_debug(BINDER_DEBUG_READ_WRITE,
-%d:%d write %zd at %08lx, read %zd at %08lx\n,
+%d:%d write %zd at %016lx, read %zd at
%016lx\n,
proc-pid, thread-pid, bwr.write_size,
bwr.write_buffer, bwr.read_size,
bwr.read_buffer);
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index edab249..2f94d16 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;
/* 8 bytes of data. */
union {
void __user *binder;/* local object */
- signed long handle; /* remote object */
+ __s32 handle; /* remote object */
This should be unsigned to match the handle in binder_transaction_data
and other uses in the driver, but it is currently also used to pass
file descriptors. Perhaps this is better (if sou also change size of
the handle in binder_transaction_data to match):
__u32 handle; /* remote object */
__s32 fd; /* file descriptor */
};
/* extra data associated with local object */
@@ -78,7 +78,7 @@ struct binder_write_read {
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
};
/* This is the current protocol version. */
--
1.7.9.5
I still think the protocol_version size change on 64 bit systems
should go after all your other changes that affect 64 bits systems.
That way you don't have to change the protocol version later.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
This change mirrors the userspace operation where struct binder_write_read
members that specify the buffer size and consumed size are size_t elements.
The patch also fixes the binder_thread_write() and binder_thread_read()
functions prototypes to conform with the definition of binder_write_read.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 10 +-
drivers/staging/android/binder.h |8
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index 1567ac2..ce70909 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
}
int binder_thread_write(struct binder_proc *proc, struct binder_thread
*thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, size_t size, size_t *consumed)
{
uint32_t cmd;
void __user *ptr = buffer + *consumed;
@@ -2080,8 +2080,8 @@ static int binder_has_thread_work(struct binder_thread
*thread)
static int binder_thread_read(struct binder_proc *proc,
struct binder_thread *thread,
- void __user *buffer, int size,
- signed long *consumed, int non_block)
+ void __user *buffer, size_t size,
+ size_t *consumed, int non_block)
{
void __user *ptr = buffer + *consumed;
void __user *end = buffer + size;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned
int cmd, unsigned long arg)
goto err;
}
binder_debug(BINDER_DEBUG_READ_WRITE,
-%d:%d write %ld at %08lx, read %ld at %08lx\n,
+%d:%d write %zd at %08lx, read %zd at %08lx\n,
proc-pid, thread-pid, bwr.write_size,
bwr.write_buffer, bwr.read_size,
bwr.read_buffer);
@@ -2604,7 +2604,7 @@ static long binder_ioctl(struct file *filp, unsigned
int cmd, unsigned long arg)
}
}
binder_debug(BINDER_DEBUG_READ_WRITE,
-%d:%d wrote %ld of %ld, read return %ld of
%ld\n,
+%d:%d wrote %zd of %zd, read return %zd of
%zd\n,
proc-pid, thread-pid, bwr.write_consumed,
bwr.write_size,
bwr.read_consumed, bwr.read_size);
if (copy_to_user(ubuf, bwr, sizeof(bwr))) {
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index dbe81ce..edab249 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -67,11 +67,11 @@ struct flat_binder_object {
*/
struct binder_write_read {
- signed long write_size; /* bytes to write */
- signed long write_consumed; /* bytes consumed by driver */
+ size_t write_size; /* bytes to write */
+ size_t write_consumed; /* bytes consumed by driver */
unsigned long write_buffer;
- signed long read_size; /* bytes to read */
- signed long read_consumed; /* bytes consumed by driver */
+ size_t read_size; /* bytes to read */
+ size_t read_consumed; /* bytes consumed by driver */
unsigned long read_buffer;
};
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Mon, Jun 3, 2013 at 8:02 AM, Greg KH gre...@linuxfoundation.org wrote:
On Fri, May 31, 2013 at 04:17:34PM -0700, Arve Hjønnevåg wrote:
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
instead of size_t for setting the max threads. Thus using the same
handler for 32 and 64bit kernels.
This value is stored internally in struct binder_proc and set to 15
on open_binder() in the libbinder API(thus no need for a 64bit size_t
on 64bit platforms).
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index 2f94d16..1761541 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -86,7 +86,7 @@ struct binder_version {
#define BINDER_WRITE_READ _IOWR('b', 1, struct
binder_write_read)
#defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
-#defineBINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
+#defineBINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
#defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, __s32)
#defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
#defineBINDER_THREAD_EXIT _IOW('b', 8, __s32)
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
What about patches 1 and 2 in this series?
I apparently only responded privately to those. I just resent them.
thanks,
greg k-h
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/6] staging: android: binder: fix alignment issues
fer->data_size < sizeof(*fp) ||
> - !IS_ALIGNED(*offp, sizeof(void *))) {
> + !IS_ALIGNED(*offp, sizeof(u32))) {
> binder_user_error("%d:%d got transaction with invalid
> offset, %zd\n",
> proc->pid, thread->pid, *offp);
> return_error = BR_FAILED_REPLY;
> @@ -2332,7 +2332,7 @@ retry:
> proc->user_buffer_offset;
> tr.data.ptr.offsets = tr.data.ptr.buffer +
> ALIGN(t->buffer->data_size,
> - sizeof(void *));
> + sizeof(u32));
This change should also be removed.
>
> if (put_user(cmd, (uint32_t __user *)ptr))
> return -EFAULT;
> --
> 1.7.9.5
>
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 6/6] staging: android: binder: replace types with portable ones
On Wed, May 22, 2013 at 3:13 AM, Serban Constantinescu
wrote:
> Since this driver is meant to be used on different types of processors
> and a portable driver should specify the size a variable expects to be
> this patch changes the types used throughout the binder interface.
>
> We use "userspace" types since this header will be exported and used by
> the Android filesystem.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.h | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index c3562c4..bff1c74 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -123,10 +123,10 @@ struct binder_transaction_data {
> void*ptr; /* target descriptor of return transaction */
> } target;
> void*cookie;/* target object cookie */
> - unsigned intcode; /* transaction command */
> + __u32 code; /* transaction command */
>
> /* General information about the transaction. */
> - unsigned intflags;
> + __u32 flags;
> pid_t sender_pid;
> uid_t sender_euid;
> size_t data_size; /* number of bytes of data */
> @@ -143,7 +143,7 @@ struct binder_transaction_data {
> /* offsets from buffer to flat_binder_object structs
> */
> const void __user *offsets;
> } ptr;
> - uint8_t buf[8];
> + __u8buf[8];
> } data;
> };
>
> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
> };
>
> struct binder_pri_desc {
> - int priority;
> - int desc;
> + __s32 priority;
> + __s32 desc;
> };
>
> struct binder_pri_ptr_cookie {
> - int priority;
> + __s32 priority;
> void *ptr;
> void *cookie;
> };
>
> enum binder_driver_return_protocol {
> - BR_ERROR = _IOR('r', 0, int),
> + BR_ERROR = _IOR('r', 0, __s32),
> /*
> * int: error code
> */
> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
> * binder_transaction_data: the received command.
> */
>
> - BR_ACQUIRE_RESULT = _IOR('r', 4, int),
> + BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
> /*
> * not currently supported
> * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
> * binder_transaction_data: the sent command.
> */
>
> - BC_ACQUIRE_RESULT = _IOW('c', 2, int),
> + BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
> /*
> * not currently supported
> * int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
> * void *: ptr to transaction data received on a read
> */
>
> - BC_INCREFS = _IOW('c', 4, int),
> - BC_ACQUIRE = _IOW('c', 5, int),
> - BC_RELEASE = _IOW('c', 6, int),
> - BC_DECREFS = _IOW('c', 7, int),
> + BC_INCREFS = _IOW('c', 4, __u32),
> + BC_ACQUIRE = _IOW('c', 5, __u32),
> + BC_RELEASE = _IOW('c', 6, __u32),
> + BC_DECREFS = _IOW('c', 7, __u32),
> /*
> * int: descriptor
> */
> --
> 1.7.9.5
>
Acked-by: Arve Hjønnevåg
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
wrote:
> This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
> instead of size_t for setting the max threads. Thus using the same
> handler for 32 and 64bit kernels.
>
> This value is stored internally in struct binder_proc and set to 15
> on open_binder() in the libbinder API(thus no need for a 64bit size_t
> on 64bit platforms).
>
> The change does not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index 2f94d16..1761541 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -86,7 +86,7 @@ struct binder_version {
>
> #define BINDER_WRITE_READ _IOWR('b', 1, struct
> binder_write_read)
> #defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
> -#defineBINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
> +#defineBINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
> #defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, __s32)
> #defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
> #define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
> --
> 1.7.9.5
>
Acked-by: Arve Hjønnevåg
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
instead of size_t for setting the max threads. Thus using the same
handler for 32 and 64bit kernels.
This value is stored internally in struct binder_proc and set to 15
on open_binder() in the libbinder API(thus no need for a 64bit size_t
on 64bit platforms).
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index 2f94d16..1761541 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -86,7 +86,7 @@ struct binder_version {
#define BINDER_WRITE_READ _IOWR('b', 1, struct
binder_write_read)
#defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
-#defineBINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
+#defineBINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
#defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, __s32)
#defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
#defineBINDER_THREAD_EXIT _IOW('b', 8, __s32)
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 6/6] staging: android: binder: replace types with portable ones
On Wed, May 22, 2013 at 3:13 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
Since this driver is meant to be used on different types of processors
and a portable driver should specify the size a variable expects to be
this patch changes the types used throughout the binder interface.
We use userspace types since this header will be exported and used by
the Android filesystem.
The patch does not change in any way the functionality of the binder driver.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.h | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index c3562c4..bff1c74 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -123,10 +123,10 @@ struct binder_transaction_data {
void*ptr; /* target descriptor of return transaction */
} target;
void*cookie;/* target object cookie */
- unsigned intcode; /* transaction command */
+ __u32 code; /* transaction command */
/* General information about the transaction. */
- unsigned intflags;
+ __u32 flags;
pid_t sender_pid;
uid_t sender_euid;
size_t data_size; /* number of bytes of data */
@@ -143,7 +143,7 @@ struct binder_transaction_data {
/* offsets from buffer to flat_binder_object structs
*/
const void __user *offsets;
} ptr;
- uint8_t buf[8];
+ __u8buf[8];
} data;
};
@@ -153,18 +153,18 @@ struct binder_ptr_cookie {
};
struct binder_pri_desc {
- int priority;
- int desc;
+ __s32 priority;
+ __s32 desc;
};
struct binder_pri_ptr_cookie {
- int priority;
+ __s32 priority;
void *ptr;
void *cookie;
};
enum binder_driver_return_protocol {
- BR_ERROR = _IOR('r', 0, int),
+ BR_ERROR = _IOR('r', 0, __s32),
/*
* int: error code
*/
@@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
* binder_transaction_data: the received command.
*/
- BR_ACQUIRE_RESULT = _IOR('r', 4, int),
+ BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
/*
* not currently supported
* int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
@@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
* binder_transaction_data: the sent command.
*/
- BC_ACQUIRE_RESULT = _IOW('c', 2, int),
+ BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
/*
* not currently supported
* int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
@@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
* void *: ptr to transaction data received on a read
*/
- BC_INCREFS = _IOW('c', 4, int),
- BC_ACQUIRE = _IOW('c', 5, int),
- BC_RELEASE = _IOW('c', 6, int),
- BC_DECREFS = _IOW('c', 7, int),
+ BC_INCREFS = _IOW('c', 4, __u32),
+ BC_ACQUIRE = _IOW('c', 5, __u32),
+ BC_RELEASE = _IOW('c', 6, __u32),
+ BC_DECREFS = _IOW('c', 7, __u32),
/*
* int: descriptor
*/
--
1.7.9.5
Acked-by: Arve Hjønnevåg a...@android.com
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/6] staging: android: binder: fix alignment issues
= tr.data.ptr.buffer + ALIGN(t-buffer-data_size, - sizeof(void *)); + sizeof(u32)); This change should also be removed. if (put_user(cmd, (uint32_t __user *)ptr)) return -EFAULT; -- 1.7.9.5 -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/6] Android Binder IPC Fixes
On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu wrote: > Hi Arve, > > > On 30/04/13 00:13, Arve Hjønnevåg wrote: >> >> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu >> wrote: >>> >>> Hi all, >>> >>> Any feedback or comments on this patch set? >>> >> >> You don't seem to have addressed my feedback on the previous patch set. > > > For v3 I have modified the following according to your review: > > >> Changes for v3: >> 1: Dropped the patch that was replacing uint32_t types with unsigned int >> 2: Dropped the patch fixing the IOCTL types(since it has been added to >> Greg's >> staging tree) >> 3: Split one patch into two: 'modify binder_write_read' and '64bit >> changes' >> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's >> review >> 5: Modified the binder command IOCTL declarations according to Arve's >> review > > > The following were left out: > >> On 11/04/13 22:40, Arve Hjønnevåg wrote: >> OK, relaxing the alignment requirement for *offp to what the hardware >>> >>> requires makes sense. Is there any macros in the kernel to help with >>> this, instead of hard-coding it to 4 bytes? > > > There is no kernel macro that I know which will help here(one that springs > to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that > aligns to (u32)). Any ideas? > Perhaps using __alignof__(struct flat_binder_object) will work. This is the least important part of that change however. I saw no response to my concern that your changes can cause less memory to be allocated than you write to. >> On 11/04/13 21:38, Arve Hjønnevåg wrote: >> OK, but if you are using this change let a 64 bit user-space know that >>> >>> the driver has been fixed, then this patch needs to go after the >>> patches that change the structures on 64 bit systems. > > > For 32bit systems nothing has changed so they will continue to work as > before. For 64bit systems the size of binder_version was signed long before > the patch and __s32 after the patch is applied. Thus a 64bit system using > the old interface will fail immediately after opening the binder driver, > while cheeking the binder version (since the BINDER_VERSION ioctl will be > different pre/post patch - size of binder_version differs). > > For 64/32 systems once I will have the userspace wrapper ready I will add > another ioctl(as discussed) that will check if the driver is 64bit > ready(among the first things to do on binder_open). > Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before the driver is usable on a 64 bit system? > Please let me know if there is anything that skipped my review and you would > like to integrate in this patch set. > It may be better to reply to my original emails instead of copying bits of them here. > Thanks for your feedback and help, > Serban > -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/6] Android Binder IPC Fixes
On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu serban.constantine...@arm.com wrote: Hi Arve, On 30/04/13 00:13, Arve Hjønnevåg wrote: On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu serban.constantine...@arm.com wrote: Hi all, Any feedback or comments on this patch set? You don't seem to have addressed my feedback on the previous patch set. For v3 I have modified the following according to your review: Changes for v3: 1: Dropped the patch that was replacing uint32_t types with unsigned int 2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's staging tree) 3: Split one patch into two: 'modify binder_write_read' and '64bit changes' 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review 5: Modified the binder command IOCTL declarations according to Arve's review The following were left out: On 11/04/13 22:40, Arve Hjønnevåg wrote: OK, relaxing the alignment requirement for *offp to what the hardware requires makes sense. Is there any macros in the kernel to help with this, instead of hard-coding it to 4 bytes? There is no kernel macro that I know which will help here(one that springs to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that aligns to (u32)). Any ideas? Perhaps using __alignof__(struct flat_binder_object) will work. This is the least important part of that change however. I saw no response to my concern that your changes can cause less memory to be allocated than you write to. On 11/04/13 21:38, Arve Hjønnevåg wrote: OK, but if you are using this change let a 64 bit user-space know that the driver has been fixed, then this patch needs to go after the patches that change the structures on 64 bit systems. For 32bit systems nothing has changed so they will continue to work as before. For 64bit systems the size of binder_version was signed long before the patch and __s32 after the patch is applied. Thus a 64bit system using the old interface will fail immediately after opening the binder driver, while cheeking the binder version (since the BINDER_VERSION ioctl will be different pre/post patch - size of binder_version differs). For 64/32 systems once I will have the userspace wrapper ready I will add another ioctl(as discussed) that will check if the driver is 64bit ready(among the first things to do on binder_open). Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before the driver is usable on a 64 bit system? Please let me know if there is anything that skipped my review and you would like to integrate in this patch set. It may be better to reply to my original emails instead of copying bits of them here. Thanks for your feedback and help, Serban -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/6] Android Binder IPC Fixes
On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu wrote: > Hi all, > > Any feedback or comments on this patch set? > You don't seem to have addressed my feedback on the previous patch set. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/6] Android Binder IPC Fixes
On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu serban.constantine...@arm.com wrote: Hi all, Any feedback or comments on this patch set? You don't seem to have addressed my feedback on the previous patch set. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues
On Thu, Apr 11, 2013 at 12:02 PM, Serban Constantinescu wrote: > On 10/04/13 23:30, Arve Hjønnevåg wrote: >> >> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu >> wrote: >>> >>> On 10/04/13 00:58, Arve Hjønnevåg wrote: >>>> >>>> >>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu >>>> wrote: >>>>> >>>>> >>>>> The Android userspace aligns the data written to the binder buffers to >>>>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit >>>>> Android userspace we can have a buffer looking like this: >>>>> >>>>> platformbuffer(binder_cmd pointer) size >>>>> 32/32 32b 32b 8B >>>>> 64/32 32b 64b 12B >>>>> 64/64 32b 64b 12B >>>>> >>>>> Thus the kernel needs to check that the buffer size is aligned to >>>>> 4bytes >>>>> not to (void *) that will be 8bytes on 64bit machines. >>>>> >>>>> The change does not affect existing 32bit ABI. >>>>> >>>> >>>> Do we not want the pointers to be 8 byte aligned on 64bit platforms? >>> >>> >>> >>> No since here we do not align pointers we align binder_buffers and >>> offsets >>> in a buffer. >>> >> >> Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we >> should keep the start address of the struct 8 byte aligned as well. > > > Most of 64bit compilers will try to align pointers within a structure to > natural boundaries. However all 64bit variants of currently supported > Android architectures can service unaligned accesses(possibly with a > performance degradation compared to an aligned access). > > You can take a look at alignment requirements for AArch64 here > http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf > chapter 4. > > What we are modifying in this patch is the alignment requirements on the > buffer size(as seen above - arbitrary size 4byte aligned) and the alignment > check on offp. > OK, relaxing the alignment requirement for *offp to what the hardware requires makes sense. Is there any macros in the kernel to help with this, instead of hard-coding it to 4 bytes? I don't think there is any reason to not keep the binder_buffer and offsets buffer that the kernel allocates aligned to 8 bytes on a 64 bit system. Also, I don't see any changes to where the offsets buffer starts in this patch, so if datasize is not 8 bytes aligned you seem to allocate less memory than you use. > Let's take a look at what offp does. The userspace will write object > references to a buffer using: > >>> 820 status_t Parcel::writeObject(const flat_binder_object& val, bool >>> nullMetaData) >>> ... >>> 826 *reinterpret_cast(mData+mDataPos) = >>> val; > > > Buffer > |---|val > | | > |->mData|->mDataPos > > where mData is the start of the buffer and mDataPos the current position > within the buffer(equivalent to offp in the kernel space). Since the buffer > is guaranteed to be u32 aligned but not u64 aligned the pointer to > flat_binder_object might live on a unaligned boundary(offp will always be > aligned to sizeof(u32) - see Parcel::writeAligned()). > > However this will happen only on buffers where at the time we write the > next object reference(val) the buffer cursor(mDataPos) happens not to be on > a multiple of sizeof(void *). > > Adding an alignment check in the userspace might be more costly than > servicing the unaligned access(for AArch64 serviced in hardware). Also we > will save some memory by not adding the padding. > > On the other hand if instead of writing a pointer we write a 64bit mutex > lock to an unaligned address and than try to read it in the kernel side > things are not guaranteed to be sane. The compiler could make the assumption > that the lock is natural aligned and use load/store exclusive that will fail > on an unaligned address. However for this situation we can extend the > userspace API and add a mutex write primitive like: > > >> status_t Parcel::writeMutex(mutex lock) >> ... >> *reinterpret_cast(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock; > > > I am not aware of any situation where you will have 64bit mutexes passed in > a binder buffer but you would probably know more about this. Since all > writes to the buffer are 32bit aligned a 32bit mutex would not suffer any > alignment issues. > > Let me know what are your thoughts about this. > Storing a mutex in a parcel does not make sense. The data in the parcel is a copy of the data passed in, and the parcel seen by the remote process is a copy of the parcel that sender created. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Thu, Apr 11, 2013 at 8:13 AM, Serban Constantinescu
wrote:
...
>>>
>>>>> unsigned long read_buffer;
>>>>> };
>>>>>
>>>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>>>> struct binder_version {
>>>>> /* driver protocol version -- increment with incompatible change */
>>>>> - signed long protocol_version;
>>>>> + __s32 protocol_version;
>>>>
>>>>
>>>>
>>>> How does user-space know if it should use 32 bit or 64 bit pointers.
>>>> Without this change, the BINDER_VERSION ioctl would only match when
>>>> the size of long matches.
>>>
>>>
>>>
>>> The userspace can check the values returned by uname(). That will
>>> determine
>>> if the kernel is 32 or 64bit and depending on this select what binder
>>> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
>>> using protocol_version as 64bit signed long(that is old kernel versions
>>> with
>>> no 64bit support).
>>>
>>> Leaving this value as signed long would mean that older versions of the
>>> binder(without 64bit support) will pass the check. Furthermore the
>>> protocol
>>> version will probably never exceed the values that could be represented
>>> on
>>> 32bit. It will also mean that BINDER_VERSION will have a different
>>> userspace/kernel handler for 64/32 systems.
>>>
>>> Let me know what are your thoughts related to these changes,
>>> Thanks for your feedback,
>>> Serban
>>>
>>
>> I think user-space should get the binder pointer size from the binder
>> driver, not elsewhere. Since the current driver does not appear to be
>> functional on a 64 bit system, I think adding an ioctl to get the
>> size, or encoding it into the binder version (use an unsigned type if
>> you do this), would be best.
>
>
> I will think about the best way of getting the pointer size and add it to
> the patch set for binder compat. For this patch set I will only modify the
> protocol_version from long to __s32.
>
OK, but if you are using this change let a 64 bit user-space know that
the driver has been fixed, then this patch needs to go after the
patches that change the structures on 64 bit systems.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Thu, Apr 11, 2013 at 8:13 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
...
unsigned long read_buffer;
};
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.
The userspace can check the values returned by uname(). That will
determine
if the kernel is 32 or 64bit and depending on this select what binder
structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
using protocol_version as 64bit signed long(that is old kernel versions
with
no 64bit support).
Leaving this value as signed long would mean that older versions of the
binder(without 64bit support) will pass the check. Furthermore the
protocol
version will probably never exceed the values that could be represented
on
32bit. It will also mean that BINDER_VERSION will have a different
userspace/kernel handler for 64/32 systems.
Let me know what are your thoughts related to these changes,
Thanks for your feedback,
Serban
I think user-space should get the binder pointer size from the binder
driver, not elsewhere. Since the current driver does not appear to be
functional on a 64 bit system, I think adding an ioctl to get the
size, or encoding it into the binder version (use an unsigned type if
you do this), would be best.
I will think about the best way of getting the pointer size and add it to
the patch set for binder compat. For this patch set I will only modify the
protocol_version from long to __s32.
OK, but if you are using this change let a 64 bit user-space know that
the driver has been fixed, then this patch needs to go after the
patches that change the structures on 64 bit systems.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues
On Thu, Apr 11, 2013 at 12:02 PM, Serban Constantinescu serban.constantine...@arm.com wrote: On 10/04/13 23:30, Arve Hjønnevåg wrote: On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu serban.constantine...@arm.com wrote: On 10/04/13 00:58, Arve Hjønnevåg wrote: On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu serban.constantine...@arm.com wrote: The Android userspace aligns the data written to the binder buffers to 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit Android userspace we can have a buffer looking like this: platformbuffer(binder_cmd pointer) size 32/32 32b 32b 8B 64/32 32b 64b 12B 64/64 32b 64b 12B Thus the kernel needs to check that the buffer size is aligned to 4bytes not to (void *) that will be 8bytes on 64bit machines. The change does not affect existing 32bit ABI. Do we not want the pointers to be 8 byte aligned on 64bit platforms? No since here we do not align pointers we align binder_buffers and offsets in a buffer. Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we should keep the start address of the struct 8 byte aligned as well. Most of 64bit compilers will try to align pointers within a structure to natural boundaries. However all 64bit variants of currently supported Android architectures can service unaligned accesses(possibly with a performance degradation compared to an aligned access). You can take a look at alignment requirements for AArch64 here http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf chapter 4. What we are modifying in this patch is the alignment requirements on the buffer size(as seen above - arbitrary size 4byte aligned) and the alignment check on offp. OK, relaxing the alignment requirement for *offp to what the hardware requires makes sense. Is there any macros in the kernel to help with this, instead of hard-coding it to 4 bytes? I don't think there is any reason to not keep the binder_buffer and offsets buffer that the kernel allocates aligned to 8 bytes on a 64 bit system. Also, I don't see any changes to where the offsets buffer starts in this patch, so if datasize is not 8 bytes aligned you seem to allocate less memory than you use. Let's take a look at what offp does. The userspace will write object references to a buffer using: 820 status_t Parcel::writeObject(const flat_binder_object val, bool nullMetaData) ... 826 *reinterpret_castflat_binder_object*(mData+mDataPos) = val; Buffer |---|val | | |-mData|-mDataPos where mData is the start of the buffer and mDataPos the current position within the buffer(equivalent to offp in the kernel space). Since the buffer is guaranteed to be u32 aligned but not u64 aligned the pointer to flat_binder_object might live on a unaligned boundary(offp will always be aligned to sizeof(u32) - see Parcel::writeAligned()). However this will happen only on buffers where at the time we write the next object reference(val) the buffer cursor(mDataPos) happens not to be on a multiple of sizeof(void *). Adding an alignment check in the userspace might be more costly than servicing the unaligned access(for AArch64 serviced in hardware). Also we will save some memory by not adding the padding. On the other hand if instead of writing a pointer we write a 64bit mutex lock to an unaligned address and than try to read it in the kernel side things are not guaranteed to be sane. The compiler could make the assumption that the lock is natural aligned and use load/store exclusive that will fail on an unaligned address. However for this situation we can extend the userspace API and add a mutex write primitive like: status_t Parcel::writeMutex(mutex lock) ... *reinterpret_castmutex(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock; I am not aware of any situation where you will have 64bit mutexes passed in a binder buffer but you would probably know more about this. Since all writes to the buffer are 32bit aligned a 32bit mutex would not suffer any alignment issues. Let me know what are your thoughts about this. Storing a mutex in a parcel does not make sense. The data in the parcel is a copy of the data passed in, and the parcel seen by the remote process is a copy of the parcel that sender created. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues
On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu wrote: > On 10/04/13 00:58, Arve Hjønnevåg wrote: >> >> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu >> wrote: >>> >>> The Android userspace aligns the data written to the binder buffers to >>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit >>> Android userspace we can have a buffer looking like this: >>> >>> platformbuffer(binder_cmd pointer) size >>> 32/32 32b 32b 8B >>> 64/32 32b 64b 12B >>> 64/64 32b 64b 12B >>> >>> Thus the kernel needs to check that the buffer size is aligned to 4bytes >>> not to (void *) that will be 8bytes on 64bit machines. >>> >>> The change does not affect existing 32bit ABI. >>> >> >> Do we not want the pointers to be 8 byte aligned on 64bit platforms? > > > No since here we do not align pointers we align binder_buffers and offsets > in a buffer. > Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we should keep the start address of the struct 8 byte aligned as well. > Let's assume that from the userspace we receive a sequence of BC_INCREFS and > BC_FREE_BUFFER. According to their definitions the buffer would look like: > > Buffer: > [addr] [element] > 0 BC_INCREFS > 4 __u32 > 8 BC_FREE_BUFFER > 12 void *//(8 bytes for 64bit or 4 bytes for 32bit) > > Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit systems(4bytes > aligned). Same explanation for offp where it represents the offset form the > start of the buffer to a flat_binder_object(for example here the offset to > void* - 12bytes). > Does this work on every 64 bit system? -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Wed, Apr 10, 2013 at 6:01 AM, Serban Constantinescu
wrote:
> On 10/04/13 00:48, Arve Hjønnevåg wrote:
>>>
>>> diff --git a/drivers/staging/android/binder.c
>>> b/drivers/staging/android/binder.c
>>> index 5794cf6..a2cdd9e 100644
>>> --- a/drivers/staging/android/binder.c
>>> +++ b/drivers/staging/android/binder.c
>>
>> ...
>>>
>>> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
>>> }
>>>
>>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>>> *thread,
>>> - void __user *buffer, int size, signed long *consumed)
>>> + void __user *buffer, size_t size, size_t *consumed)
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> This change is related to the userspace handling of the struct
> binder_write_read. The userspace writes write_size and read_size as size_t
> values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).
>
> On return from a BINDER_WRITE_READ ioctl write_consumed and read_consumed
> are checked against positive values(these values will represent the
> difference between the start of the buffer cursor and the current buffer
> start - positive values since buffer cursor = buffer start ++).
>
> However if there is any plan for these values to be handled as signed longs
> at some point we can change the patch such that we modify just the function
> prototype to:
>
>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>> *thread,
>> - void __user *buffer, int size, signed long *consumed)
>> + void __user *buffer, signed long size, signed long
>> *consumed)
>
>
> I will break this change into its own patch such that it is easier to grasp.
>
OK.
>
>>
>> ...
>>>
>>> diff --git a/drivers/staging/android/binder.h
>>> b/drivers/staging/android/binder.h
>>> index dbe81ce..8012921 100644
>>> --- a/drivers/staging/android/binder.h
>>> +++ b/drivers/staging/android/binder.h
>>> @@ -48,13 +48,13 @@ enum {
>>> */
>>> struct flat_binder_object {
>>> /* 8 bytes for large_flat_header. */
>>> - unsigned long type;
>>> - unsigned long flags;
>>> + __u32 type;
>>> + __u32 flags;
>>>
>>> /* 8 bytes of data. */
>>> union {
>>> void __user *binder; /* local object */
>>> - signed long handle; /* remote object */
>>> + __s32 handle; /* remote object */
>>
>>
>> Why limit the handle to 32 bits when the pointer that it shares
>> storage with need to be 64 bit on 64 bit systems?
>
>
> Here I have mirrored the type being passed in handle - a file
> descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
> BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
> the kernel/userspace(as 32bit value on 64bit systems). However this change
> does not limit the extension of the API since we can read the value as 64bit
> - binder(on 64bit systems).
>
> I can remove this change if you consider that is the better solution.
>
I was asking if we should just use 64 bit handles on 64 bit systems,
not adding casts. It would require another union member for a file
descriptor however.
>
>>> };
>>>
>>> /* extra data associated with local object */
>>> @@ -67,18 +67,18 @@ struct flat_binder_object {
>>> */
>>>
>>> struct binder_write_read {
>>> - signed long write_size; /* bytes to write */
>>> - signed long write_consumed; /* bytes consumed by driver */
>>> + size_t write_size; /* bytes to write */
>>> + size_t write_consumed; /* bytes consumed by driver */
>>> unsigned long write_buffer;
>>> - signed long read_size; /* bytes to read */
>>> - signed long read_consumed; /* bytes consumed by driver */
>>> + size_t read_size; /* bytes to read */
>>> + size_t read_consumed; /* bytes consumed by driver */
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> See above explanation for binder_thread_write() change, I will break this
> into its own patch.
>
>
>>> unsigned long read_buffer;
>>> };
>>>
>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>> struct binder_version {
>>> /* driver protocol version -- increment with incompatible change */
>>> - signed long protocol_version;
>>&
Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Wed, Apr 10, 2013 at 5:37 AM, Serban Constantinescu wrote: > On 10/04/13 00:53, Arve Hjønnevåg wrote: >> >> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu >> wrote: >>> >>> This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32 >>> instead of size_t for setting the max threads. Thus using the same >>> handler for 32 and 64bit kernels. >>> >>> This value is stored internally in struct binder_proc as an int and >>> is set to 15 on open_binder() in the libbinder API (thus no need for >>> a 64bit size_t on 64bit platforms). >>> >> >> Why switch to a signed type? > > > The value passed through BINDER_SET_MAX_THREADS ioctl is stored in a > binder_proc structure as an int.It also mimics the use of pid_t(typedef int > __kernel_pid_t). > This is a thread count not a pid. > However using __s32 or __u32 here would have the same effect since the ioctl > macro will compute both as sizeof(32bit). > > Let me know if you would like this changed to __u32. > The user-space api uses a size_t, so __u32 would be a closer match. Keeping it a size_t would also work since this value is not shared between processes. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Wed, Apr 10, 2013 at 5:37 AM, Serban Constantinescu serban.constantine...@arm.com wrote: On 10/04/13 00:53, Arve Hjønnevåg wrote: On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu serban.constantine...@arm.com wrote: This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32 instead of size_t for setting the max threads. Thus using the same handler for 32 and 64bit kernels. This value is stored internally in struct binder_proc as an int and is set to 15 on open_binder() in the libbinder API (thus no need for a 64bit size_t on 64bit platforms). Why switch to a signed type? The value passed through BINDER_SET_MAX_THREADS ioctl is stored in a binder_proc structure as an int.It also mimics the use of pid_t(typedef int __kernel_pid_t). This is a thread count not a pid. However using __s32 or __u32 here would have the same effect since the ioctl macro will compute both as sizeof(32bit). Let me know if you would like this changed to __u32. The user-space api uses a size_t, so __u32 would be a closer match. Keeping it a size_t would also work since this value is not shared between processes. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Wed, Apr 10, 2013 at 6:01 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
On 10/04/13 00:48, Arve Hjønnevåg wrote:
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index 5794cf6..a2cdd9e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
...
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
}
int binder_thread_write(struct binder_proc *proc, struct binder_thread
*thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, size_t size, size_t *consumed)
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
This change is related to the userspace handling of the struct
binder_write_read. The userspace writes write_size and read_size as size_t
values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).
On return from a BINDER_WRITE_READ ioctl write_consumed and read_consumed
are checked against positive values(these values will represent the
difference between the start of the buffer cursor and the current buffer
start - positive values since buffer cursor = buffer start ++).
However if there is any plan for these values to be handled as signed longs
at some point we can change the patch such that we modify just the function
prototype to:
int binder_thread_write(struct binder_proc *proc, struct binder_thread
*thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, signed long size, signed long
*consumed)
I will break this change into its own patch such that it is easier to grasp.
OK.
...
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index dbe81ce..8012921 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;
/* 8 bytes of data. */
union {
void __user *binder; /* local object */
- signed long handle; /* remote object */
+ __s32 handle; /* remote object */
Why limit the handle to 32 bits when the pointer that it shares
storage with need to be 64 bit on 64 bit systems?
Here I have mirrored the type being passed in handle - a file
descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
the kernel/userspace(as 32bit value on 64bit systems). However this change
does not limit the extension of the API since we can read the value as 64bit
- binder(on 64bit systems).
I can remove this change if you consider that is the better solution.
I was asking if we should just use 64 bit handles on 64 bit systems,
not adding casts. It would require another union member for a file
descriptor however.
};
/* extra data associated with local object */
@@ -67,18 +67,18 @@ struct flat_binder_object {
*/
struct binder_write_read {
- signed long write_size; /* bytes to write */
- signed long write_consumed; /* bytes consumed by driver */
+ size_t write_size; /* bytes to write */
+ size_t write_consumed; /* bytes consumed by driver */
unsigned long write_buffer;
- signed long read_size; /* bytes to read */
- signed long read_consumed; /* bytes consumed by driver */
+ size_t read_size; /* bytes to read */
+ size_t read_consumed; /* bytes consumed by driver */
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
See above explanation for binder_thread_write() change, I will break this
into its own patch.
unsigned long read_buffer;
};
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.
The userspace can check the values returned by uname(). That will determine
if the kernel is 32 or 64bit and depending on this select what binder
structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
using protocol_version as 64bit signed long(that is old kernel versions with
no 64bit support).
Leaving this value as signed long would mean that older versions of the
binder(without 64bit support) will pass the check. Furthermore the protocol
version will probably never exceed the values that could be represented on
32bit. It will also mean that BINDER_VERSION will have a different
userspace/kernel handler for 64/32 systems.
Let me know what are your thoughts related to these changes,
Thanks for your feedback,
Serban
I think user-space should get
Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues
On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu serban.constantine...@arm.com wrote: On 10/04/13 00:58, Arve Hjønnevåg wrote: On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu serban.constantine...@arm.com wrote: The Android userspace aligns the data written to the binder buffers to 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit Android userspace we can have a buffer looking like this: platformbuffer(binder_cmd pointer) size 32/32 32b 32b 8B 64/32 32b 64b 12B 64/64 32b 64b 12B Thus the kernel needs to check that the buffer size is aligned to 4bytes not to (void *) that will be 8bytes on 64bit machines. The change does not affect existing 32bit ABI. Do we not want the pointers to be 8 byte aligned on 64bit platforms? No since here we do not align pointers we align binder_buffers and offsets in a buffer. Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we should keep the start address of the struct 8 byte aligned as well. Let's assume that from the userspace we receive a sequence of BC_INCREFS and BC_FREE_BUFFER. According to their definitions the buffer would look like: Buffer: [addr] [element] 0 BC_INCREFS 4 __u32 8 BC_FREE_BUFFER 12 void *//(8 bytes for 64bit or 4 bytes for 32bit) Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit systems(4bytes aligned). Same explanation for offp where it represents the offset form the start of the buffer to a flat_binder_object(for example here the offset to void* - 12bytes). Does this work on every 64 bit system? -- Arve Hjønnevåg -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/7] staging: android: binder: replace IOCTL types with user-exportable types
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
wrote:
> This patch modifies the IOCTL macros to use user-exportable data types,
> as they are the referred kernel types for the user/kernel interface.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu
Acked-by: Arve Hjønnevåg
> ---
> drivers/staging/android/binder.h |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index f240464..dbe81ce 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -85,11 +85,11 @@ struct binder_version {
> #define BINDER_CURRENT_PROTOCOL_VERSION 7
>
> #define BINDER_WRITE_READ _IOWR('b', 1, struct
> binder_write_read)
> -#defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, int64_t)
> +#defineBINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
> #defineBINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
> -#defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, int)
> -#defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, int)
> -#defineBINDER_THREAD_EXIT _IOW('b', 8, int)
> +#defineBINDER_SET_IDLE_PRIORITY_IOW('b', 6, __s32)
> +#defineBINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
> +#defineBINDER_THREAD_EXIT _IOW('b', 8, __s32)
BINDER_SET_CONTEXT_MGR and BINDER_THREAD_EXIT don't actually take an argument.
> #define BINDER_VERSION _IOWR('b', 9, struct binder_version)
>
> /*
> --
> 1.7.9.5
>
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/7] staging: android: binder: clean-up uint32_t types
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu wrote: > uint32_t types are used inconsistently throughout the driver. This patch > replaces "uint32_t" types with "unsigned int" ones. > I don't like this change. You fix the header in a later patch to use explicit sizes, so it does not make sense to do the opposite here. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 7/7] staging: android: binder: replace types with portable ones
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
wrote:
> Since this driver is meant to be used on different types of processors
> and a portable driver should specify the size a variable expects to be
> this patch changes the types used throughout the binder interface.
>
> We use "userspace" types since this header will be exported and used by
> the Android filesystem.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.h | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index 8789baa..f3ffacd 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -123,10 +123,10 @@ struct binder_transaction_data {
> void*ptr; /* target descriptor of return transaction */
> } target;
> void*cookie;/* target object cookie */
> - unsigned intcode; /* transaction command */
> + __u32 code; /* transaction command */
>
> /* General information about the transaction. */
> - unsigned intflags;
> + __u32 flags;
> pid_t sender_pid;
> uid_t sender_euid;
> size_t data_size; /* number of bytes of data */
> @@ -143,7 +143,7 @@ struct binder_transaction_data {
> /* offsets from buffer to flat_binder_object structs
> */
> const void __user *offsets;
> } ptr;
> - uint8_t buf[8];
> + __u8buf[8];
> } data;
> };
>
> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
> };
>
> struct binder_pri_desc {
> - int priority;
> - int desc;
> + __s32 priority;
> + __s32 desc;
> };
>
> struct binder_pri_ptr_cookie {
> - int priority;
> + __s32 priority;
> void *ptr;
> void *cookie;
> };
>
> enum binder_driver_return_protocol {
> - BR_ERROR = _IOR('r', 0, int),
> + BR_ERROR = _IOR('r', 0, __s32),
> /*
> * int: error code
> */
> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
> * binder_transaction_data: the received command.
> */
>
> - BR_ACQUIRE_RESULT = _IOR('r', 4, int),
> + BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
> /*
> * not currently supported
> * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
> * binder_transaction_data: the sent command.
> */
>
> - BC_ACQUIRE_RESULT = _IOW('c', 2, int),
> + BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
> /*
> * not currently supported
> * int: 0 if the last BR_ATTEMPT_ACQUIRE was not successful.
> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
> * void *: ptr to transaction data received on a read
> */
>
> - BC_INCREFS = _IOW('c', 4, int),
> - BC_ACQUIRE = _IOW('c', 5, int),
> - BC_RELEASE = _IOW('c', 6, int),
> - BC_DECREFS = _IOW('c', 7, int),
> + BC_INCREFS = _IOW('c', 4, __s32),
> + BC_ACQUIRE = _IOW('c', 5, __s32),
> + BC_RELEASE = _IOW('c', 6, __s32),
> + BC_DECREFS = _IOW('c', 7, __s32),
These four are actually read as unsigned values, so it would be better
to use __u32 here.
> /*
> * int: descriptor
> */
> --
> 1.7.9.5
>
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu wrote: > The Android userspace aligns the data written to the binder buffers to > 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit > Android userspace we can have a buffer looking like this: > > platformbuffer(binder_cmd pointer) size > 32/32 32b 32b 8B > 64/32 32b 64b 12B > 64/64 32b 64b 12B > > Thus the kernel needs to check that the buffer size is aligned to 4bytes > not to (void *) that will be 8bytes on 64bit machines. > > The change does not affect existing 32bit ABI. > Do we not want the pointers to be 8 byte aligned on 64bit platforms? -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 5/7] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu wrote: > BinderDriverCommands mirror the ioctl usage. Thus the size of the > structure passed through the interface should be used to generate the > ioctl No. > > The change reflects the type being passed from the user space-a pointer > to a binder_buffer. This change should not affect the existing 32bit > user space since BC_FREE_BUFFER is computed as: > >#define _IOW(type,nr,size) \ > ((type) << _IOC_TYPESHIFT) |\ > ((nr) << _IOC_NRSHIFT) | \ > ((size) << _IOC_SIZESHIFT)) > > and for a 32bit compiler BC_FREE_BUFFER will have the same computed > value. This change will also ease our work in differentiating > BC_FREE_BUFFER from COMPAT_BC_FREE_BUFFER. > > The change does not affect existing 32bit ABI. > > Signed-off-by: Serban Constantinescu Acked-by: Arve Hjønnevåg -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu wrote: > This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32 > instead of size_t for setting the max threads. Thus using the same > handler for 32 and 64bit kernels. > > This value is stored internally in struct binder_proc as an int and > is set to 15 on open_binder() in the libbinder API (thus no need for > a 64bit size_t on 64bit platforms). > Why switch to a signed type? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
wrote:
> The changes in this patch will fix the binder interface for use on 64bit
> machines and stand as the base of the 64bit compat support. The changes
> apply to the structures that are passed between the kernel and
> userspace.
>
> Most of the changes applied mirror the change to struct binder_version
> where there is no need for a 64bit wide protocol_version(on 64bit
> machines). The change inlines with the existing 32bit userspace(the
> structure has the same size) and simplifies the compat layer such that
> the same handler can service the BINDER_VERSION ioctl.
>
> Other changes fix the function prototypes for binder_thread_read/write,
> make use of kernel types as well as user-exportable ones and fix
> format specifier issues.
>
> The changes do not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu
> ---
> drivers/staging/android/binder.c | 28 ++--
> drivers/staging/android/binder.h | 16
> 2 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c
> b/drivers/staging/android/binder.c
> index 5794cf6..a2cdd9e 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
...
> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
> }
>
> int binder_thread_write(struct binder_proc *proc, struct binder_thread
> *thread,
> - void __user *buffer, int size, signed long *consumed)
> + void __user *buffer, size_t size, size_t *consumed)
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
...
> diff --git a/drivers/staging/android/binder.h
> b/drivers/staging/android/binder.h
> index dbe81ce..8012921 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -48,13 +48,13 @@ enum {
> */
> struct flat_binder_object {
> /* 8 bytes for large_flat_header. */
> - unsigned long type;
> - unsigned long flags;
> + __u32 type;
> + __u32 flags;
>
> /* 8 bytes of data. */
> union {
> void __user *binder; /* local object */
> - signed long handle; /* remote object */
> + __s32 handle; /* remote object */
Why limit the handle to 32 bits when the pointer that it shares
storage with need to be 64 bit on 64 bit systems?
> };
>
> /* extra data associated with local object */
> @@ -67,18 +67,18 @@ struct flat_binder_object {
> */
>
> struct binder_write_read {
> - signed long write_size; /* bytes to write */
> - signed long write_consumed; /* bytes consumed by driver */
> + size_t write_size; /* bytes to write */
> + size_t write_consumed; /* bytes consumed by driver */
> unsigned long write_buffer;
> - signed long read_size; /* bytes to read */
> - signed long read_consumed; /* bytes consumed by driver */
> + size_t read_size; /* bytes to read */
> + size_t read_consumed; /* bytes consumed by driver */
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
> unsigned long read_buffer;
> };
>
> /* Use with BINDER_VERSION, driver fills in fields. */
> struct binder_version {
> /* driver protocol version -- increment with incompatible change */
> - signed long protocol_version;
> + __s32 protocol_version;
How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.
> };
>
> /* This is the current protocol version. */
> --
> 1.7.9.5
>
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
serban.constantine...@arm.com wrote:
The changes in this patch will fix the binder interface for use on 64bit
machines and stand as the base of the 64bit compat support. The changes
apply to the structures that are passed between the kernel and
userspace.
Most of the changes applied mirror the change to struct binder_version
where there is no need for a 64bit wide protocol_version(on 64bit
machines). The change inlines with the existing 32bit userspace(the
structure has the same size) and simplifies the compat layer such that
the same handler can service the BINDER_VERSION ioctl.
Other changes fix the function prototypes for binder_thread_read/write,
make use of kernel types as well as user-exportable ones and fix
format specifier issues.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 28 ++--
drivers/staging/android/binder.h | 16
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index 5794cf6..a2cdd9e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
...
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
}
int binder_thread_write(struct binder_proc *proc, struct binder_thread
*thread,
- void __user *buffer, int size, signed long *consumed)
+ void __user *buffer, size_t size, size_t *consumed)
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
...
diff --git a/drivers/staging/android/binder.h
b/drivers/staging/android/binder.h
index dbe81ce..8012921 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
*/
struct flat_binder_object {
/* 8 bytes for large_flat_header. */
- unsigned long type;
- unsigned long flags;
+ __u32 type;
+ __u32 flags;
/* 8 bytes of data. */
union {
void __user *binder; /* local object */
- signed long handle; /* remote object */
+ __s32 handle; /* remote object */
Why limit the handle to 32 bits when the pointer that it shares
storage with need to be 64 bit on 64 bit systems?
};
/* extra data associated with local object */
@@ -67,18 +67,18 @@ struct flat_binder_object {
*/
struct binder_write_read {
- signed long write_size; /* bytes to write */
- signed long write_consumed; /* bytes consumed by driver */
+ size_t write_size; /* bytes to write */
+ size_t write_consumed; /* bytes consumed by driver */
unsigned long write_buffer;
- signed long read_size; /* bytes to read */
- signed long read_consumed; /* bytes consumed by driver */
+ size_t read_size; /* bytes to read */
+ size_t read_consumed; /* bytes consumed by driver */
What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.
unsigned long read_buffer;
};
/* Use with BINDER_VERSION, driver fills in fields. */
struct binder_version {
/* driver protocol version -- increment with incompatible change */
- signed long protocol_version;
+ __s32 protocol_version;
How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.
};
/* This is the current protocol version. */
--
1.7.9.5
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu serban.constantine...@arm.com wrote: This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32 instead of size_t for setting the max threads. Thus using the same handler for 32 and 64bit kernels. This value is stored internally in struct binder_proc as an int and is set to 15 on open_binder() in the libbinder API (thus no need for a 64bit size_t on 64bit platforms). Why switch to a signed type? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
