From: Greg KH <gregkh@linuxfoundation.org>
To: Colin Cross <ccross@android.com>
Cc: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"Ian Rogers" <irogers@google.com>,
"Serban Constantinescu" <serban.constantinescu@arm.com>,
lkml <linux-kernel@vger.kernel.org>,
"Arve Hjønnevåg" <arve@android.com>,
"John Stultz" <john.stultz@linaro.org>,
romlem@android.com, "David Butcher" <Dave.Butcher@arm.com>
Subject: Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
Date: Wed, 4 Dec 2013 16:02:32 -0800 [thread overview]
Message-ID: <20131205000232.GA19810@kroah.com> (raw)
In-Reply-To: <CAMbhsRQnY6+Ahs-itSeALNQ27tsWAJT8wUtXNGZE--44URp2-A@mail.gmail.com>
On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote:
> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
> >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
> >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> <snip>
> >> >>
> >> >> > And finally, is this all really needed? Why not just fix the structures
> >> >> > to be "correct", and then fix userspace to use the correct structures as
> >> >> > well, thereby not needing a compat layer at all?
> >> >>
> >> >> Some of the binder ioctls take userspace pointers. Are you suggesting
> >> >> storing those pointers in a __u64 to avoid having to have a
> >> >> compat_ioctl?
> >> >
> >> > Yes, that's the best way to solve the issue, right?
> >>
> >> It's the least code, but in exchange you lose all the type safety and
> >> warnings when copying in and out of the pointers, as well as sparse
> >> checking on the __user attribute.
> >
> > Not if you make the cast right at the beginning, when you first "touch"
> > the data, but yes, it does take some of the type saftey away, at the
> > expense of simpler code to mess up :)
> >
> >> That doesn't seem like a good tradeoff to me. In addition it requires
> >> modifying the existing heavily used 32 bit api, which means a
> >> mostly-equivalent compat layer added in libbinder to support old
> >> kernels.
> >
> > Wait, I thought that libbinder would have to be changed anyway here, to
> > handle 64bit kernels (in both 32 and 64bit userspace). Since you are
> > already changing it, why not just "do it correctly"?
>
> libbinder will need changes to support 64-bit userspace and especially
> a mixed 64-bit and 32-bit userspace, but this patch series is only
> addressing a pure 32-bit userspace on a 64-bit kernel. Support for a
> 64-bit userspace in Android is obviously going to require a future
> version of Android including, among other things, libbinder changes.
> As far as I know, those changes won't need to change the ioctl api,
> only the layout of the buffers that are passed through the ioctl api.
"only" means you can rearrange things at that point in time, as you will
have to be doing that anyway :)
> > Or does this patch series mean that no userspace code is changed? Is
> > that a "requirement" here?
>
> Since this series only addresses 32-bit userspace on 64-bit kernel
> support there are no associated userspace changes. Changing the
> 32-bit api here means that combining the KitKat branch from
> http://android.googlesource.com with a newer kernel version will not
> work.
Is that something that anyone has said would work in the past? It seems
that other parts of the Android userspace are pretty tied to specific
kernel features / versions, is this anything new if the binder code had
to change?
Anyway, the code as submitted has problems, see my response to the
second patch, so it's not ready yet anyway :(
thanks,
greg k-h
next prev parent reply other threads:[~2013-12-05 0:04 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-04 18:09 [PATCH v1 0/9] Android: Add Support for Binder Compat Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction Serban Constantinescu
2013-12-05 8:00 ` Dan Carpenter
2013-12-05 18:37 ` Serban Constantinescu
2013-12-05 8:18 ` Dan Carpenter
2013-12-05 15:31 ` Greg KH
2013-12-05 18:35 ` Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user() Serban Constantinescu
2013-12-04 23:17 ` Greg KH
2013-12-05 18:44 ` Serban Constantinescu
2013-12-05 8:36 ` Dan Carpenter
2013-12-04 18:09 ` [PATCH v1 3/9] staging: android: binder: Add cmd == CMD_NAME handling Serban Constantinescu
2013-12-05 8:40 ` Dan Carpenter
2013-12-05 18:50 ` Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 4/9] staging: android: binder: Add align_helper() macro Serban Constantinescu
2013-12-05 8:41 ` Dan Carpenter
2013-12-04 18:09 ` [PATCH v1 5/9] staging: android: binder: Add deref_helper() macro Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 6/9] staging: android: binder: Add size_helper() macro Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 7/9] staging: android: binder: Add copy_flat_binder_object() Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 8/9] staging: android: binder: Add binder compat handling to binder.h Serban Constantinescu
2013-12-04 18:09 ` [PATCH v1 9/9] staging: android: binder: Add binder compat layer Serban Constantinescu
2013-12-04 18:35 ` Greg KH
2013-12-04 20:46 ` Colin Cross
2013-12-04 21:43 ` Greg KH
2013-12-04 21:55 ` Colin Cross
2013-12-04 22:02 ` Greg KH
2013-12-04 22:22 ` Colin Cross
2013-12-05 0:02 ` Greg KH [this message]
2013-12-05 0:21 ` Colin Cross
2013-12-05 2:02 ` Arve Hjønnevåg
2013-12-05 18:31 ` Serban Constantinescu
2013-12-05 18:49 ` Greg KH
2013-12-10 3:01 ` Octavian Purdila
2013-12-11 3:21 ` Arve Hjønnevåg
2013-12-11 18:10 ` Octavian Purdila
2013-12-11 23:00 ` Arve Hjønnevåg
2013-12-12 8:45 ` Octavian Purdila
2013-12-13 5:14 ` Arve Hjønnevåg
2013-12-13 7:39 ` Octavian Purdila
2013-12-04 23:21 ` One Thousand Gnomes
2013-12-04 23:40 ` Colin Cross
2013-12-05 0:32 ` One Thousand Gnomes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131205000232.GA19810@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=Dave.Butcher@arm.com \
--cc=arve@android.com \
--cc=ccross@android.com \
--cc=devel@driverdev.osuosl.org \
--cc=irogers@google.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=romlem@android.com \
--cc=serban.constantinescu@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox