by Serban Constantinescu

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for 64bit compat layer

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
> <[email protected]> wrote:
>> On 03/06/13 22:41, Arve Hj?nnev?g wrote:
>>>
>>> On Wed, May 22, 2013 at 3:12 AM, Serban Constantinescu
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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