Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752214Ab3FDX6G (ORCPT ); Tue, 4 Jun 2013 19:58:06 -0400 Received: from mail-pb0-f47.google.com ([209.85.160.47]:52258 "EHLO mail-pb0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751343Ab3FDX6E convert rfc822-to-8bit (ORCPT ); Tue, 4 Jun 2013 19:58:04 -0400 MIME-Version: 1.0 In-Reply-To: <51ADAB5F.70902@arm.com> References: <1369217581-16285-1-git-send-email-serban.constantinescu@arm.com> <1369217581-16285-3-git-send-email-serban.constantinescu@arm.com> <51ADAB5F.70902@arm.com> Date: Tue, 4 Jun 2013 16:58:02 -0700 Message-ID: Subject: Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?= To: Serban Constantinescu Cc: LKML , Greg KH , Android Kernel Team , John Stultz , Dave Butcher Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10702 Lines: 246 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", >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/