Re: [PATCH v4 2/6] staging: android: binder: fix binder interfacefor 64bit compat layer
From: Serban Constantinescu
Date: Wed Jun 05 2013 - 04:29:14 EST
On 05/06/13 00:58, Arve Hjønnevåg wrote:
On Tue, Jun 4, 2013 at 1:54 AM, Serban Constantinescu
<Serban.Constantinescu@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/