Re: [PATCH v4 2/6] staging: android: binder: fix binder interface for64bit compat layer

[Arve Hjønnevåg]

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?

--
Arve Hjønnevåg
--
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/