* [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
@ 2013-05-22 10:12 ` Serban Constantinescu
2013-06-03 21:42 ` Arve Hjønnevåg
2013-05-22 10:12 ` [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer Serban Constantinescu
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:12 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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.constantinescu@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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t
2013-05-22 10:12 ` [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t Serban Constantinescu
@ 2013-06-03 21:42 ` Arve Hjønnevåg
0 siblings, 0 replies; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-06-03 21:42 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
<serban.constantinescu@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.constantinescu@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 <arve@android.com>
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
2013-05-22 10:12 ` [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t Serban Constantinescu
@ 2013-05-22 10:12 ` Serban Constantinescu
2013-06-03 21:41 ` Arve Hjønnevåg
2013-05-22 10:12 ` [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration Serban Constantinescu
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:12 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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.constantinescu@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",
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 */
};
/* 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-05-22 10:12 ` [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer Serban Constantinescu
@ 2013-06-03 21:41 ` Arve Hjønnevåg
2013-06-04 8:54 ` Serban Constantinescu
0 siblings, 1 reply; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-06-03 21:41 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
<serban.constantinescu@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.constantinescu@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",
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-06-03 21:41 ` Arve Hjønnevåg
@ 2013-06-04 8:54 ` Serban Constantinescu
2013-06-04 23:58 ` Arve Hjønnevåg
0 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-06-04 8:54 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On 03/06/13 22:41, Arve Hjønnevåg wrote:
> On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
> <serban.constantinescu@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.constantinescu@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",
>> 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 */
I will add this union and fix any uses of remote object/ file descriptor
accordingly.
>
>> };
>>
>> /* 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.
At the end of this patch set we will have support for 32/32 and 64/64
binder calls. This patch does not add compat support for 64/32 systems
and will not work for this configuration.
However until we add:
> static const struct file_operations binder_fops = {
> .owner = THIS_MODULE,
> .poll = binder_poll,
> .unlocked_ioctl = binder_ioctl,
> + .compat_ioctl = binder_ioctl,
> .mmap = binder_mmap,
> .open = binder_open,
> .flush = binder_flush,
The return value for any binder ioctl from a 32bit userspace running on
top of a 64bit kernel will be EINVAL (this happens only for 64/32
systems). Once we have the compat layer upstreamed we will add the above
change, but until that point querying the binder version or any other
binder iocall will fail (on 64/32).
Let me know if you consider that changing the binder version to use
__s32 when adding the compat layer would be better.
Thanks for your feedback and help,
Serban
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-06-04 8:54 ` Serban Constantinescu
@ 2013-06-04 23:58 ` Arve Hjønnevåg
2013-06-05 8:28 ` Serban Constantinescu
2013-06-18 16:18 ` Serban Constantinescu
0 siblings, 2 replies; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-06-04 23:58 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
<Serban.Constantinescu@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.constantinescu@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.constantinescu@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",
>>> 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 */
>
>
> I will add this union and fix any uses of remote object/ file descriptor
> accordingly.
>
>
>>
>>> };
>>>
>>> /* 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.
>
> At the end of this patch set we will have support for 32/32 and 64/64 binder
> calls. This patch does not add compat support for 64/32 systems and will not
> work for this configuration.
>
> However until we add:
>
>> static const struct file_operations binder_fops = {
>> .owner = THIS_MODULE,
>> .poll = binder_poll,
>> .unlocked_ioctl = binder_ioctl,
>> + .compat_ioctl = binder_ioctl,
>> .mmap = binder_mmap,
>> .open = binder_open,
>> .flush = binder_flush,
>
>
> The return value for any binder ioctl from a 32bit userspace running on top
> of a 64bit kernel will be EINVAL (this happens only for 64/32 systems). Once
> we have the compat layer upstreamed we will add the above change, but until
> that point querying the binder version or any other binder iocall will fail
> (on 64/32).
>
> Let me know if you consider that changing the binder version to use __s32
> when adding the compat layer would be better.
>
If the ioctl fails on for a 32 bit process on a 64 bit kernel, you can
change the size before adding the compat_ioctl, but you need to finish
all the changes that affect a 64/64 system first. You can either split
this change in two or move the entire change to the end of your
patch-set. Can you also post your changes that add 64/32 support?
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-06-04 23:58 ` Arve Hjønnevåg
@ 2013-06-05 8:28 ` Serban Constantinescu
2013-06-18 16:18 ` Serban Constantinescu
1 sibling, 0 replies; 19+ messages in thread
From: Serban Constantinescu @ 2013-06-05 8:28 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On 05/06/13 00:58, Arve Hjønnevåg wrote:
> On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
> <Serban.Constantinescu@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.constantinescu@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.constantinescu@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",
>>>> 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 */
>>
>>
>> I will add this union and fix any uses of remote object/ file descriptor
>> accordingly.
>>
>>
>>>
>>>> };
>>>>
>>>> /* 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.
>>
>> At the end of this patch set we will have support for 32/32 and 64/64 binder
>> calls. This patch does not add compat support for 64/32 systems and will not
>> work for this configuration.
>>
>> However until we add:
>>
>>> static const struct file_operations binder_fops = {
>>> .owner = THIS_MODULE,
>>> .poll = binder_poll,
>>> .unlocked_ioctl = binder_ioctl,
>>> + .compat_ioctl = binder_ioctl,
>>> .mmap = binder_mmap,
>>> .open = binder_open,
>>> .flush = binder_flush,
>>
>>
>> The return value for any binder ioctl from a 32bit userspace running on top
>> of a 64bit kernel will be EINVAL (this happens only for 64/32 systems). Once
>> we have the compat layer upstreamed we will add the above change, but until
>> that point querying the binder version or any other binder iocall will fail
>> (on 64/32).
>>
>> Let me know if you consider that changing the binder version to use __s32
>> when adding the compat layer would be better.
>>
>
> If the ioctl fails on for a 32 bit process on a 64 bit kernel, you can
> change the size before adding the compat_ioctl, but you need to finish
> all the changes that affect a 64/64 system first. You can either split
> this change in two or move the entire change to the end of your
> patch-set. Can you also post your changes that add 64/32 support?
Thanks again for the feedback - I will split the binder version change
into its own patch.
I will rework my binder compat changes based on a 3.10 kernel and resend
them.
Regards,
Serban
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer
2013-06-04 23:58 ` Arve Hjønnevåg
2013-06-05 8:28 ` Serban Constantinescu
@ 2013-06-18 16:18 ` Serban Constantinescu
1 sibling, 0 replies; 19+ messages in thread
From: Serban Constantinescu @ 2013-06-18 16:18 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
Hi all,
Sorry for the late reply on this patch set - I had to re-prioritise some
of the other tasks I am currently working on. v5 of this patch set
should be out the door soon.
On 05/06/13 00:58, Arve Hjønnevåg wrote:
> On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
> <Serban.Constantinescu@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.constantinescu@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.constantinescu@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",
>>>> 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 */
>>
>>
>> I will add this union and fix any uses of remote object/ file descriptor
>> accordingly.
>>
The cleaner change would be to have an __u32 handle that is used in both
cases (fd/ uint handle). Adding __s32 fd to the union would cause either
inconsistency with other function definitions or a big set of changes.
See for example binder_transaction_buffer_release(), case BINDER_TYPE_FD
- task_close_fd(), __close_fd().
I will change the struct flat_binder_object to use an __u32 handle for
v5. Let me know if you have other thoughts on this.
Thanks,
Serban
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
2013-05-22 10:12 ` [PATCH v4 1/6] staging: android: binder: modify struct binder_write_read to use size_t Serban Constantinescu
2013-05-22 10:12 ` [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer Serban Constantinescu
@ 2013-05-22 10:12 ` Serban Constantinescu
2013-05-31 23:17 ` Arve Hjønnevåg
2013-05-22 10:12 ` [PATCH v4 4/6] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration Serban Constantinescu
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:12 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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.constantinescu@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)
#define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
-#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
+#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
#define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
#define BINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
#define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
2013-05-22 10:12 ` [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration Serban Constantinescu
@ 2013-05-31 23:17 ` Arve Hjønnevåg
2013-06-03 15:02 ` Greg KH
0 siblings, 1 reply; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-05-31 23:17 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
<serban.constantinescu@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.constantinescu@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)
> #define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
> -#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
> +#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
> #define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
> #define BINDER_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@android.com>
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
2013-05-31 23:17 ` Arve Hjønnevåg
@ 2013-06-03 15:02 ` Greg KH
2013-06-03 21:44 ` Arve Hjønnevåg
0 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2013-06-03 15:02 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: Serban Constantinescu, LKML, Android Kernel Team, John Stultz,
Dave Butcher
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.constantinescu@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.constantinescu@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)
> > #define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
> > -#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
> > +#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
> > #define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
> > #define BINDER_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@android.com>
What about patches 1 and 2 in this series?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
2013-06-03 15:02 ` Greg KH
@ 2013-06-03 21:44 ` Arve Hjønnevåg
2013-06-03 22:39 ` Greg KH
0 siblings, 1 reply; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-06-03 21:44 UTC (permalink / raw)
To: Greg KH
Cc: Serban Constantinescu, LKML, Android Kernel Team, John Stultz,
Dave Butcher
On Mon, Jun 3, 2013 at 8:02 AM, Greg KH <gregkh@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.constantinescu@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.constantinescu@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)
>> > #define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
>> > -#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
>> > +#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
>> > #define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
>> > #define BINDER_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@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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
2013-06-03 21:44 ` Arve Hjønnevåg
@ 2013-06-03 22:39 ` Greg KH
0 siblings, 0 replies; 19+ messages in thread
From: Greg KH @ 2013-06-03 22:39 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: Serban Constantinescu, LKML, Android Kernel Team, John Stultz,
Dave Butcher
On Mon, Jun 03, 2013 at 02:44:25PM -0700, Arve Hjønnevåg wrote:
> On Mon, Jun 3, 2013 at 8:02 AM, Greg KH <gregkh@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.constantinescu@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.constantinescu@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)
> >> > #define BINDER_SET_IDLE_TIMEOUT _IOW('b', 3, __s64)
> >> > -#define BINDER_SET_MAX_THREADS _IOW('b', 5, size_t)
> >> > +#define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
> >> > #define BINDER_SET_IDLE_PRIORITY _IOW('b', 6, __s32)
> >> > #define BINDER_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@android.com>
> >
> > What about patches 1 and 2 in this series?
> >
>
> I apparently only responded privately to those. I just resent them.
Thanks for that. I'll wait for the series to be fixed up and resent
before doing anything with them.
greg k-h
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 4/6] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
` (2 preceding siblings ...)
2013-05-22 10:12 ` [PATCH v4 3/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration Serban Constantinescu
@ 2013-05-22 10:12 ` Serban Constantinescu
2013-05-22 10:13 ` [PATCH v4 5/6] staging: android: binder: fix alignment issues Serban Constantinescu
2013-05-22 10:13 ` [PATCH v4 6/6] staging: android: binder: replace types with portable ones Serban Constantinescu
5 siblings, 0 replies; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:12 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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 <serban.constantinescu@arm.com>
Acked-by: Arve Hjønnevåg <arve@android.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 1761541..c3562c4 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -265,7 +265,7 @@ enum binder_driver_command_protocol {
* Else you have acquired a primary reference on the object.
*/
- BC_FREE_BUFFER = _IOW('c', 3, int),
+ BC_FREE_BUFFER = _IOW('c', 3, void *),
/*
* void *: ptr to transaction data received on a read
*/
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 5/6] staging: android: binder: fix alignment issues
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
` (3 preceding siblings ...)
2013-05-22 10:12 ` [PATCH v4 4/6] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration Serban Constantinescu
@ 2013-05-22 10:13 ` Serban Constantinescu
2013-05-31 23:18 ` Arve Hjønnevåg
2013-05-22 10:13 ` [PATCH v4 6/6] staging: android: binder: replace types with portable ones Serban Constantinescu
5 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:13 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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:
platform buffer(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 patch also fixes the alignment issues for the offsets buffer, where
on 64bit systems, if the data_size is not aligned to 8bytes the offset's
start address will be incorrectly calculated.
The change does not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
drivers/staging/android/binder.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index ca79084..72f5bb9 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -658,8 +658,8 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
return NULL;
}
- size = ALIGN(data_size, sizeof(void *)) +
- ALIGN(offsets_size, sizeof(void *));
+ size = ALIGN(data_size, sizeof(u32)) +
+ ALIGN(offsets_size, sizeof(u32));
if (size < data_size || size < offsets_size) {
binder_user_error("%d: got transaction with invalid size %zd-%zd\n",
@@ -807,8 +807,8 @@ static void binder_free_buf(struct binder_proc *proc,
buffer_size = binder_buffer_size(proc, buffer);
- size = ALIGN(buffer->data_size, sizeof(void *)) +
- ALIGN(buffer->offsets_size, sizeof(void *));
+ size = ALIGN(buffer->data_size, sizeof(u32)) +
+ ALIGN(buffer->offsets_size, sizeof(u32));
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%d: binder_free_buf %p size %zd buffer_size %zd\n",
@@ -1238,7 +1238,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
if (buffer->target_node)
binder_dec_node(buffer->target_node, 1, 0);
- offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
+ offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(u32)));
if (failed_at)
off_end = failed_at;
else
@@ -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;
@@ -1471,7 +1471,7 @@ static void binder_transaction(struct binder_proc *proc,
if (target_node)
binder_inc_node(target_node, 1, 0, NULL);
- offp = (size_t *)(t->buffer->data + ALIGN(tr->data_size, sizeof(void *)));
+ offp = (size_t *)(t->buffer->data + ALIGN(tr->data_size, sizeof(u32)));
if (copy_from_user(t->buffer->data, tr->data.ptr.buffer, tr->data_size)) {
binder_user_error("%d:%d got transaction with invalid data ptr\n",
@@ -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;
@@ -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));
if (put_user(cmd, (uint32_t __user *)ptr))
return -EFAULT;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 5/6] staging: android: binder: fix alignment issues
2013-05-22 10:13 ` [PATCH v4 5/6] staging: android: binder: fix alignment issues Serban Constantinescu
@ 2013-05-31 23:18 ` Arve Hjønnevåg
0 siblings, 0 replies; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-05-31 23:18 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Wed, May 22, 2013 at 3:13 AM, Serban Constantinescu
<serban.constantinescu@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:
>
> platform buffer(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 patch also fixes the alignment issues for the offsets buffer, where
> on 64bit systems, if the data_size is not aligned to 8bytes the offset's
> start address will be incorrectly calculated.
I think this refers to a problem introduced in a previous version of
this patch, not a problem with the existing code. If you want to
include this in the change description you should make that more
clear.
>
> The change does not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
> drivers/staging/android/binder.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index ca79084..72f5bb9 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -658,8 +658,8 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
> return NULL;
> }
>
> - size = ALIGN(data_size, sizeof(void *)) +
> - ALIGN(offsets_size, sizeof(void *));
> + size = ALIGN(data_size, sizeof(u32)) +
> + ALIGN(offsets_size, sizeof(u32));
I still disagree with this change. There is no reason to not keep the
kernel allocated buffers aligned to the native pointer size. The
binder_buffer struct contain several pointers that are not longer
aligned with this change.
>
> if (size < data_size || size < offsets_size) {
> binder_user_error("%d: got transaction with invalid size %zd-%zd\n",
> @@ -807,8 +807,8 @@ static void binder_free_buf(struct binder_proc *proc,
>
> buffer_size = binder_buffer_size(proc, buffer);
>
> - size = ALIGN(buffer->data_size, sizeof(void *)) +
> - ALIGN(buffer->offsets_size, sizeof(void *));
> + size = ALIGN(buffer->data_size, sizeof(u32)) +
> + ALIGN(buffer->offsets_size, sizeof(u32));
This change should also be removed.
>
> binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> "%d: binder_free_buf %p size %zd buffer_size %zd\n",
> @@ -1238,7 +1238,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
> if (buffer->target_node)
> binder_dec_node(buffer->target_node, 1, 0);
>
> - offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
> + offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(u32)));
This change should also be removed.
> if (failed_at)
> off_end = failed_at;
> else
> @@ -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;
> @@ -1471,7 +1471,7 @@ static void binder_transaction(struct binder_proc *proc,
> if (target_node)
> binder_inc_node(target_node, 1, 0, NULL);
>
> - offp = (size_t *)(t->buffer->data + ALIGN(tr->data_size, sizeof(void *)));
> + offp = (size_t *)(t->buffer->data + ALIGN(tr->data_size, sizeof(u32)));
This change should also be removed.
>
> if (copy_from_user(t->buffer->data, tr->data.ptr.buffer, tr->data_size)) {
> binder_user_error("%d:%d got transaction with invalid data ptr\n",
> @@ -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;
> @@ -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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 6/6] staging: android: binder: replace types with portable ones
2013-05-22 10:12 [PATCH v4 0/6] Android Binder IPC Fixes Serban Constantinescu
` (4 preceding siblings ...)
2013-05-22 10:13 ` [PATCH v4 5/6] staging: android: binder: fix alignment issues Serban Constantinescu
@ 2013-05-22 10:13 ` Serban Constantinescu
2013-05-31 23:18 ` Arve Hjønnevåg
5 siblings, 1 reply; 19+ messages in thread
From: Serban Constantinescu @ 2013-05-22 10:13 UTC (permalink / raw)
To: linux-kernel, gregkh, kernel-team, arve, john.stultz,
Dave.Butcher
Cc: Serban Constantinescu
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.constantinescu@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 int code; /* transaction command */
+ __u32 code; /* transaction command */
/* General information about the transaction. */
- unsigned int flags;
+ __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];
+ __u8 buf[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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 6/6] staging: android: binder: replace types with portable ones
2013-05-22 10:13 ` [PATCH v4 6/6] staging: android: binder: replace types with portable ones Serban Constantinescu
@ 2013-05-31 23:18 ` Arve Hjønnevåg
0 siblings, 0 replies; 19+ messages in thread
From: Arve Hjønnevåg @ 2013-05-31 23:18 UTC (permalink / raw)
To: Serban Constantinescu
Cc: LKML, Greg KH, Android Kernel Team, John Stultz, Dave Butcher
On Wed, May 22, 2013 at 3:13 AM, Serban Constantinescu
<serban.constantinescu@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.constantinescu@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 int code; /* transaction command */
> + __u32 code; /* transaction command */
>
> /* General information about the transaction. */
> - unsigned int flags;
> + __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];
> + __u8 buf[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@android.com>
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 19+ messages in thread