From: Greg Kroah-Hartman <redacted>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Signed-off-by: Greg Kroah-Hartman <redacted>
---
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
drivers/Kconfig | 2 ++
drivers/Makefile | 1 +
drivers/android/Kconfig | 37 ++++++++++++++++++++++
drivers/android/Makefile | 3 ++
drivers/{staging => }/android/binder.c | 0
drivers/{staging => }/android/binder.h | 2 +-
drivers/{staging => }/android/binder_trace.h | 0
drivers/staging/android/Kconfig | 30 ------------------
drivers/staging/android/Makefile | 1 -
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/android/Kbuild | 2 ++
.../uapi => include/uapi/linux/android}/binder.h | 0
12 files changed, 47 insertions(+), 32 deletions(-)
create mode 100644 drivers/android/Kconfig
create mode 100644 drivers/android/Makefile
rename drivers/{staging => }/android/binder.c (100%)
rename drivers/{staging => }/android/binder.h (95%)
rename drivers/{staging => }/android/binder_trace.h (100%)
create mode 100644 include/uapi/linux/android/Kbuild
rename {drivers/staging/android/uapi => include/uapi/linux/android}/binder.h (100%)
@@ -0,0 +1,3 @@+ccflags-y+=-I$(src)# needed for trace events++obj-$(CONFIG_ANDROID_BINDER_IPC)+=binder.o
diff --git a/drivers/staging/android/binder.c b/drivers/android/binder.csimilarity index 100%rename from drivers/staging/android/binder.crename to drivers/android/binder.cdiff --git a/drivers/staging/android/binder.h b/drivers/android/binder.hsimilarity index 95%rename from drivers/staging/android/binder.hrename to drivers/android/binder.hindex eb0834656dfe..5dc6a66b0665 100644--- a/drivers/staging/android/binder.h+++ b/drivers/android/binder.h
diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.hsimilarity index 100%rename from drivers/staging/android/uapi/binder.hrename to include/uapi/linux/android/binder.h
--
2.1.2
From: Michael Kerrisk (man-pages) <hidden> Date: 2014-10-16 14:18:25
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
[off-list ref] wrote:
From: Greg Kroah-Hartman <redacted>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Where does one find the canonical documentation of the user-space API?
Thanks,
Michael
quoted hunk
Signed-off-by: Greg Kroah-Hartman <redacted>
---
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
drivers/Kconfig | 2 ++
drivers/Makefile | 1 +
drivers/android/Kconfig | 37 ++++++++++++++++++++++
drivers/android/Makefile | 3 ++
drivers/{staging => }/android/binder.c | 0
drivers/{staging => }/android/binder.h | 2 +-
drivers/{staging => }/android/binder_trace.h | 0
drivers/staging/android/Kconfig | 30 ------------------
drivers/staging/android/Makefile | 1 -
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/android/Kbuild | 2 ++
.../uapi => include/uapi/linux/android}/binder.h | 0
12 files changed, 47 insertions(+), 32 deletions(-)
create mode 100644 drivers/android/Kconfig
create mode 100644 drivers/android/Makefile
rename drivers/{staging => }/android/binder.c (100%)
rename drivers/{staging => }/android/binder.h (95%)
rename drivers/{staging => }/android/binder_trace.h (100%)
create mode 100644 include/uapi/linux/android/Kbuild
rename {drivers/staging/android/uapi => include/uapi/linux/android}/binder.h (100%)
@@ -0,0 +1,3 @@+ccflags-y+=-I$(src)# needed for trace events++obj-$(CONFIG_ANDROID_BINDER_IPC)+=binder.o
diff --git a/drivers/staging/android/binder.c b/drivers/android/binder.csimilarity index 100%rename from drivers/staging/android/binder.crename to drivers/android/binder.cdiff --git a/drivers/staging/android/binder.h b/drivers/android/binder.hsimilarity index 95%rename from drivers/staging/android/binder.hrename to drivers/android/binder.hindex eb0834656dfe..5dc6a66b0665 100644--- a/drivers/staging/android/binder.h+++ b/drivers/android/binder.h
diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.hsimilarity index 100%rename from drivers/staging/android/uapi/binder.hrename to include/uapi/linux/android/binder.h
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 04:18:02PM +0200, Michael Kerrisk (man-pages) wrote:
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Where does one find the canonical documentation of the user-space API?
There really is only one "canonical" thing, and that is in the libbinder
code in the Android userspace repository. And it's not really
"documentation" so much as, "a C file that interacts with the ioctls in
the binder kernel code" :(
Think of this as just a random character driver with some funny ioctls
that will never get really documented as there is only one user of it.
sorry,
greg k-h
From: Dan Carpenter <hidden> Date: 2014-10-20 12:46:06
Having documentation would help reviewers.
Here is some documentation I found online.
http://www.phonesdevelopers.com/1793504/
But it seems like it was originally written in another language and auto
translated to English.
I don't see who is going to maintain this, if no one has time to even
document how it's supposed to work...
regards,
dan carpenter
From: Pavel Machek <hidden> Date: 2014-10-21 10:01:26
On Fri 2014-10-17 01:14:57, Greg Kroah-Hartman wrote:
On Thu, Oct 16, 2014 at 04:18:02PM +0200, Michael Kerrisk (man-pages) wrote:
quoted
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Where does one find the canonical documentation of the user-space API?
There really is only one "canonical" thing, and that is in the libbinder
code in the Android userspace repository. And it's not really
"documentation" so much as, "a C file that interacts with the ioctls in
the binder kernel code" :(
Think of this as just a random character driver with some funny ioctls
that will never get really documented as there is only one user of it.
From: John Stultz <hidden> Date: 2014-10-16 17:09:07
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
Well, ignoring the ABI break that landed in the last year. :)
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
So my main concerns/thoughts here are:
Who is going to maintain this? Has Arve agreed? Are the Android guys
comfortable with the ABI stability rules they'll now face? Currently
in the android space no one but libbinder should use the kernel
interface. Can we enforce that no one use this interface out-side of
android (To ensure its one of those "if the abi breaks and no one
notices..." edge cases)?
I'm still hopeful that a libbinder over kdbus will eventually pan out.
I still see having two core IPC mechanisms (even if the use cases are
different in style) as a negative, and I worry this might reduce
motivation to unify things at the lower level. Granted, such work can
still continue, but the incentives do change.
thanks
-john
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <redacted>
The Android binder code has been "stable" for many years now. No matter
Well, ignoring the ABI break that landed in the last year. :)
As no one noticed, it wasn't a "break" :)
quoted
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
So my main concerns/thoughts here are:
Who is going to maintain this? Has Arve agreed?
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
Are the Android guys comfortable with the ABI stability rules they'll
now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
And the Android developers said they were happy to see this code move
into the "real" part of the kernel.
Currently in the android space no one but libbinder should use the
kernel interface.
That is correct. If you do that, you deserve all of the pain and
suffering and rooted machines you will get.
Can we enforce that no one use this interface out-side of android (To
ensure its one of those "if the abi breaks and no one notices..." edge
cases)?
This is the kernel, we can not dictate "use", that's the good part of
the GPLv2 :)
Again, as no one has done this before, I strongly doubt it will happen
in the future.
I'm still hopeful that a libbinder over kdbus will eventually pan out.
I still see having two core IPC mechanisms (even if the use cases are
different in style) as a negative, and I worry this might reduce
motivation to unify things at the lower level. Granted, such work can
still continue, but the incentives do change.
Yes, things are going to change in the future, there is work happening
here, and there was a presentation at Plumbers about what is going to be
coming.
But all of the changes will be in new code. Be it kdbus, or something
else if that doesn't work out. This existing binder.c file will not be
changing at all. This existing ABI, and codebase, is something that we
have to maintain forever for those millions of devices out there in the
real world today. So as there really is nothing left to do on it, it
deserves to be in the main part of the kernel source tree.
thanks,
greg k-h
From: John Stultz <hidden> Date: 2014-10-17 03:25:36
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman
[off-list ref] wrote:
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
quoted
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <redacted>
The Android binder code has been "stable" for many years now. No matter
Well, ignoring the ABI break that landed in the last year. :)
As no one noticed, it wasn't a "break" :)
quoted
quoted
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
So my main concerns/thoughts here are:
Who is going to maintain this? Has Arve agreed?
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
Ok. The only caution I have if Arve isn't involved is that we have in
the past merged cleanup changes that introduced bugs, and it wasn't
until Arve took at look at the new kernel that these were sorted out
(see e194fd8a5d8e0a7eeed239a8534460724b62fe2d). So I think if at all
possible getting his ack on things would be good.
quoted
Are the Android guys comfortable with the ABI stability rules they'll
now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
And the Android developers said they were happy to see this code move
into the "real" part of the kernel.
Alright then.
quoted
Currently in the android space no one but libbinder should use the
kernel interface.
That is correct. If you do that, you deserve all of the pain and
suffering and rooted machines you will get.
You reference this issue quite a bit, and I have a handwavy sense of
the problem, but do you have a more detailed link to the specific
issue here?
quoted
Can we enforce that no one use this interface out-side of android (To
ensure its one of those "if the abi breaks and no one notices..." edge
cases)?
This is the kernel, we can not dictate "use", that's the good part of
the GPLv2 :)
Again, as no one has done this before, I strongly doubt it will happen
in the future.
Well, the longer something is in the kernel, the more likely someone
will depend on it.
quoted
I'm still hopeful that a libbinder over kdbus will eventually pan out.
I still see having two core IPC mechanisms (even if the use cases are
different in style) as a negative, and I worry this might reduce
motivation to unify things at the lower level. Granted, such work can
still continue, but the incentives do change.
Yes, things are going to change in the future, there is work happening
here, and there was a presentation at Plumbers about what is going to be
coming.
But all of the changes will be in new code. Be it kdbus, or something
else if that doesn't work out. This existing binder.c file will not be
changing at all. This existing ABI, and codebase, is something that we
have to maintain forever for those millions of devices out there in the
real world today. So as there really is nothing left to do on it, it
deserves to be in the main part of the kernel source tree.
Well, realistically, those millions of devices out there will almost
*never* get a kernel version bump
But yes, I'm fine with it going in. I'm just a bit surprised at how
quickly thoughts change here.
This does raise the question if the same standard could be held to
ashmem then? That's a *much* simpler and easier to maintain chunk of
code, and is just as isolated, logic wise. And while I think it would
be ideal if the unpinning feature could be done more generically, if
volatile ranges really doesn't have a future, then its maybe silly to
hold ashmem out as well?
thanks
-john
On Thu, Oct 16, 2014 at 08:25:33PM -0700, John Stultz wrote:
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
quoted
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
Well, ignoring the ABI break that landed in the last year. :)
As no one noticed, it wasn't a "break" :)
quoted
quoted
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
So my main concerns/thoughts here are:
Who is going to maintain this? Has Arve agreed?
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
Ok. The only caution I have if Arve isn't involved is that we have in
the past merged cleanup changes that introduced bugs, and it wasn't
until Arve took at look at the new kernel that these were sorted out
(see e194fd8a5d8e0a7eeed239a8534460724b62fe2d). So I think if at all
possible getting his ack on things would be good.
I'll make sure to get his Ack on anything that could possibly cause a
problem.
But yes, I'm fine with it going in. I'm just a bit surprised at how
quickly thoughts change here.
Over the past year I've realized that code in staging needs to progress
either out of the kernel due to no need/use, or into the main part of
the kernel as people rely on it. The android code is something that
people rely on, and this code is "stable", so it should be merged into
the main kernel tree.
So yes, this does seem to be a change in the public stance, but it's
something that I have been talking about with people at the zillion
conferences I go to around the world.
This does raise the question if the same standard could be held to
ashmem then?
Ah, you beat me to it, I was going to wait to talk to you in person
about this :)
That's a *much* simpler and easier to maintain chunk of
code, and is just as isolated, logic wise. And while I think it would
be ideal if the unpinning feature could be done more generically, if
volatile ranges really doesn't have a future, then its maybe silly to
hold ashmem out as well?
Yes, that was the second thing I was going to move, I'll send a patch
for that later today.
thanks,
greg k-h
From: One Thousand Gnomes <hidden> Date: 2014-10-18 21:37:05
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
Well every time in the past that Al Viro looked in its direction he broke
it so probably. Someone is going to have to clean up or fix the fact it
pokes around in the depths of the low level fd I/O code and calls stuff
like __fd_install and __alloc_fd directly, or mend it if it breaks.
I'm curious what Al Viro thinks of it
quoted
Currently in the android space no one but libbinder should use the
kernel interface.
That is correct. If you do that, you deserve all of the pain and
suffering and rooted machines you will get.
So what is the Android side model for its security. That probably also
should be described so nobody goes off and uses it for something like
systemd because "it looked neat".
But all of the changes will be in new code. Be it kdbus, or something
else if that doesn't work out. This existing binder.c file will not be
changing at all. This existing ABI, and codebase, is something that we
have to maintain forever for those millions of devices out there in the
real world today.
95% of those devices are locked down, most of them have non replaceable
batteries that will dead and irreplacable (sanely anyway) in 3-5 years.
"Forever" in the phone world is mercifully rather short.
Alan
On Sat, Oct 18, 2014 at 10:36:30PM +0100, One Thousand Gnomes wrote:
quoted
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
Well every time in the past that Al Viro looked in its direction he broke
it so probably. Someone is going to have to clean up or fix the fact it
pokes around in the depths of the low level fd I/O code and calls stuff
like __fd_install and __alloc_fd directly, or mend it if it breaks.
As it is, it is ok, but bad things happen if you allow more than one
process to open the device node. In android systems, that doesn't
happen, so all should be acceptable.
I'm curious what Al Viro thinks of it
His last comments were along the lines of "don't let anything open that
device node other than libbinder".
quoted
quoted
Currently in the android space no one but libbinder should use the
kernel interface.
That is correct. If you do that, you deserve all of the pain and
suffering and rooted machines you will get.
So what is the Android side model for its security. That probably also
should be described so nobody goes off and uses it for something like
systemd because "it looked neat".
The side model is "one owner that knows what they are doing as they have
root privileges". I don't know a way to codify that, and we all know no
one reads documentation...
quoted
But all of the changes will be in new code. Be it kdbus, or something
else if that doesn't work out. This existing binder.c file will not be
changing at all. This existing ABI, and codebase, is something that we
have to maintain forever for those millions of devices out there in the
real world today.
95% of those devices are locked down, most of them have non replaceable
batteries that will dead and irreplacable (sanely anyway) in 3-5 years.
"Forever" in the phone world is mercifully rather short.
I still see brand new devices with 2 year old Android userspace being
shipped today. With a total mis-mash of random kernel versions,
depending on what the SoC supported. If we can delete this in 2-5
years, I would be really happy.
thanks,
greg k-h
From: Pavel Machek <hidden> Date: 2014-10-21 10:36:27
On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
quoted
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <redacted>
quoted
Are the Android guys comfortable with the ABI stability rules they'll
now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
Actually, there's big difference.
If Al Viro changes core filesystem in a way that breaks
staging/binder, binder is broken, and if it can't be fixed... well it
can't be fixed.
If Al Viro changes core filesystem in a way that breaks
drivers/binder, Al's change is going to be reverted.
It is really hard to review without API documentation. Normally, API
documentation is required for stuff like this.
For example: does it add new files in /proc?
Given that it is stable, can we get rid of binder_debug() and
especially BINDER_DEBUG_ENTRY stuff?
Checkpatch warns about 98 too long lines. Some of them could be fixed
easily.
This looks scary:
trace_binder_transaction_fd(t, fp->handle,
target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
" fd %d -> %d\n",
fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
Could binder_transcation() be split to smaller functions according to
CodingStyle? 17 goto targets at the end of function are not exactly
easy to read.
ginder_thread_read/write also needs splitting.
binder_ioctl_write_read: just use direct return, no need to goto out
if it just returns.
proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
mutex_unlock(&binder_mmap_lock);
#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
while (CACHE_COLOUR((vma->vm_start ^
(uint32_t)proc->buffer))) {
Should this be (uintptr_t)?
/*pr_info("binder_mmap: %d %lx-%lx maps %p\n",
Delete the code, don't comment it out. It is on more than one place.
static void print_binder_thread(struct seq_file *m,
struct binder_thread *thread,
int print_always)
{
struct binder_transaction *t;
struct binder_work *w;
size_t start_pos = m->count;
size_t header_pos;
seq_printf(m, " thread %d: l %02x\n", thread->pid,
thread->looper);
header_pos = m->count;
t = thread->transaction_stack;
while (t) {
if (t->from == thread) {
print_binder_transaction(m,
"
outgoing transaction", t);
t = t->from_parent;
Is anyone depending on the debugfs files? Can it be deleted?
Code indentation is "interesting" in binder_thread_read(). See the "}
break;" lines. {}s should not be needed...?
I don't think this code would get merged if it was submitted for
normal inclusion in kernel. I don't think it is good idea to push it
through the back door, without documenting what it does and without
patches even going to the lists.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Are the Android guys comfortable with the ABI stability rules they'll
now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
Actually, there's big difference.
If Al Viro changes core filesystem in a way that breaks
staging/binder, binder is broken, and if it can't be fixed... well it
can't be fixed.
If Al Viro changes core filesystem in a way that breaks
drivers/binder, Al's change is going to be reverted.
One might have argued that we'd have to do that already, but the reasons
for doing that with binder in the main kernel are certainly stronger.
It is really hard to review without API documentation. Normally, API
documentation is required for stuff like this.
For example: does it add new files in /proc?
Given that it is stable, can we get rid of binder_debug() and
especially BINDER_DEBUG_ENTRY stuff?
Good point. We require documentation for every single sysfs attribute
that gets added to a driver (some escape the review, but that doesn't
change the rule), so we should not make an exception for a new procfs
file here.
Checkpatch warns about 98 too long lines. Some of them could be fixed
easily.
I don't think this should be an argument.
This looks scary:
trace_binder_transaction_fd(t, fp->handle,
target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
" fd %d -> %d\n",
fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
Could binder_transcation() be split to smaller functions according to
CodingStyle? 17 goto targets at the end of function are not exactly
easy to read.
ginder_thread_read/write also needs splitting.
Yes, in principle, but this is still a detail that would mainly serve
to simplify review. The problem is more the lack of review and
documentation of the API.
binder_ioctl_write_read: just use direct return, no need to goto out
if it just returns.
details, and a lot of people actually like the style used there.
proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
mutex_unlock(&binder_mmap_lock);
#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
while (CACHE_COLOUR((vma->vm_start ^
(uint32_t)proc->buffer))) {
Should this be (uintptr_t)?
It should probably call an architecture specific helper.
Arnd
From: Pavel Machek <hidden> Date: 2014-10-21 20:05:29
On Tue 2014-10-21 16:12:24, Arnd Bergmann wrote:
On Tuesday 21 October 2014 12:36:22 Pavel Machek wrote:
quoted
On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
quoted
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
quoted
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
[off-list ref] wrote:
quoted
From: Greg Kroah-Hartman <redacted>
quoted
quoted
Are the Android guys comfortable with the ABI stability rules they'll
now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
Actually, there's big difference.
If Al Viro changes core filesystem in a way that breaks
staging/binder, binder is broken, and if it can't be fixed... well it
can't be fixed.
If Al Viro changes core filesystem in a way that breaks
drivers/binder, Al's change is going to be reverted.
One might have argued that we'd have to do that already, but the reasons
for doing that with binder in the main kernel are certainly stronger.
quoted
It is really hard to review without API documentation. Normally, API
documentation is required for stuff like this.
For example: does it add new files in /proc?
Given that it is stable, can we get rid of binder_debug() and
especially BINDER_DEBUG_ENTRY stuff?
Good point. We require documentation for every single sysfs attribute
that gets added to a driver (some escape the review, but that doesn't
change the rule), so we should not make an exception for a new procfs
file here.
Actually, it looked like it is debugfs file. Code was messy enough
that I was not sure.
quoted
Could binder_transcation() be split to smaller functions according to
CodingStyle? 17 goto targets at the end of function are not exactly
easy to read.
ginder_thread_read/write also needs splitting.
Yes, in principle, but this is still a detail that would mainly serve
to simplify review. The problem is more the lack of review and
documentation of the API.
From: Dan Carpenter <hidden> Date: 2014-10-17 09:26:26
The code isn't very beautiful and there are lots of details wrong like
the error codes.
Al had some critical things to say about it but it looks like most of
those issues have been fixed.
regards,
dan carpenter
On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
The code isn't very beautiful and there are lots of details wrong like
the error codes.
Really, what is wrong with the existing error code usages?
Al had some critical things to say about it but it looks like most of
those issues have been fixed.
All of the ones that can be fixed, have, as far as I know.
We are stuck with some that we can't fix, which is ok as it is safe in
the Android user model.
thanks,
greg k-h
From: Dan Carpenter <hidden> Date: 2014-10-20 09:21:11
On Mon, Oct 20, 2014 at 06:05:47AM +0800, Greg Kroah-Hartman wrote:
On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
quoted
The code isn't very beautiful and there are lots of details wrong like
the error codes.
Really, what is wrong with the existing error code usages?
I guess I was mostly looking at binder_ioctl(), the rest of the code
seems better.
I feel like we went directly from "This code is never going in so there
is no need to look at it." to "This code is going in in one week so
there is no time to look at it."
How often do people rely on proc_no_lock=1 to make this work? People
are saying on the internet that you don't need acurate information so
you should disable locking as a speed up.
http://www.programdevelop.com/1821706/. It seems like a bad idea to
provide provide the "run fast and crash" option.
Why is binder_set_nice needed? Some comments would help here.
434 static void binder_set_nice(long nice)
435 {
436 long min_nice;
437
438 if (can_nice(current, nice)) {
439 set_user_nice(current, nice);
440 return;
441 }
442 min_nice = rlimit_to_nice(current->signal->rlim[RLIMIT_NICE].rlim_cur);
443 binder_debug(BINDER_DEBUG_PRIORITY_CAP,
444 "%d: nice value %ld not allowed use %ld instead\n",
445 current->pid, nice, min_nice);
446 set_user_nice(current, min_nice);
447 if (min_nice <= MAX_NICE)
448 return;
It's harmless but wierd to check if min_nice is valid after we already
called set_user_nice().
449 binder_user_error("%d RLIMIT_NICE not set\n", current->pid);
450 }
Random comment:
709 has_page_addr =
710 (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
The casting on "buffer->data" ugly and inconsistent. There should be
a function:
void *buffer_data(struct binder_buffer *buffer)
{
return buffer.data;
}
That way this becomes:
has_page_addr = (buffer_data(buffer) + buffer_size) & PAGE_MASK);
The "has_page_addr" variable name is misleading, because we're not
storing true/false. We're storing the last page address.
711 if (n == NULL) {
712 if (size + sizeof(struct binder_buffer) + 4 >= buffer_size)
713 buffer_size = size; /* no room for other buffers */
714 else
715 buffer_size = size + sizeof(struct binder_buffer);
716 }
717 end_page_addr =
718 (void *)PAGE_ALIGN((uintptr_t)buffer->data + buffer_size);
719 if (end_page_addr > has_page_addr)
720 end_page_addr = has_page_addr;
721 if (binder_update_page_range(proc, 1,
722 (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL))
723 return NULL;
The alignment here is confusing because we don't align buffer->data
below.
724
725 rb_erase(best_fit, &proc->free_buffers);
726 buffer->free = 0;
727 binder_insert_allocated_buffer(proc, buffer);
728 if (buffer_size != size) {
729 struct binder_buffer *new_buffer = (void *)buffer->data + size;
^^^^^^^^^^^^^^^^^^^^
I don't really understand when buffer->data has to be page aligned and
when not. This code has very few comments.
730
731 list_add(&new_buffer->entry, &buffer->entry);
732 new_buffer->free = 1;
733 binder_insert_free_buffer(proc, new_buffer);
734 }
Does the stop_on_user_error option work? There should be some
documentation for this stuff.
regards,
dan carpenter
On Mon, Oct 20, 2014 at 2:20 AM, Dan Carpenter [off-list ref] wrote:
On Mon, Oct 20, 2014 at 06:05:47AM +0800, Greg Kroah-Hartman wrote:
quoted
On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
quoted
The code isn't very beautiful and there are lots of details wrong like
the error codes.
Really, what is wrong with the existing error code usages?
I guess I was mostly looking at binder_ioctl(), the rest of the code
seems better.
I feel like we went directly from "This code is never going in so there
is no need to look at it." to "This code is going in in one week so
there is no time to look at it."
How often do people rely on proc_no_lock=1 to make this work? People
are saying on the internet that you don't need acurate information so
you should disable locking as a speed up.
http://www.programdevelop.com/1821706/. It seems like a bad idea to
provide provide the "run fast and crash" option.
That is not what this switch is for. It is only there to debug the
driver if it gets stuck with the lock held. I would not object to
adding a config option to remove this param by default.
Why is binder_set_nice needed? Some comments would help here.
The driver has some support for priority inheritance.
434 static void binder_set_nice(long nice)
435 {
436 long min_nice;
437
438 if (can_nice(current, nice)) {
439 set_user_nice(current, nice);
440 return;
441 }
442 min_nice = rlimit_to_nice(current->signal->rlim[RLIMIT_NICE].rlim_cur);
443 binder_debug(BINDER_DEBUG_PRIORITY_CAP,
444 "%d: nice value %ld not allowed use %ld instead\n",
445 current->pid, nice, min_nice);
446 set_user_nice(current, min_nice);
447 if (min_nice <= MAX_NICE)
448 return;
It's harmless but wierd to check if min_nice is valid after we already
called set_user_nice().
I don't remember why I did this, but I agree it is weird.
449 binder_user_error("%d RLIMIT_NICE not set\n", current->pid);
450 }
Random comment:
709 has_page_addr =
710 (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
The casting on "buffer->data" ugly and inconsistent. There should be
a function:
Other code in the kernel seems to do this the same way (although most
cast to unsigned long instead of uintptr_t). This code rounds down to
the start of of the page, and needs to cast to an integer type for
this. Adding a global kernel helper for this would be better than a
binder specific one. The existing PAGE_ALIGN function, which rounds
up, still needs casts to use with pointer types though.
void *buffer_data(struct binder_buffer *buffer)
{
return buffer.data;
}
That way this becomes:
has_page_addr = (buffer_data(buffer) + buffer_size) & PAGE_MASK);
I don't think this will compile.
The "has_page_addr" variable name is misleading, because we're not
storing true/false. We're storing the last page address.
It is the address of a page we already have mapped, not the last
address of the new allocation.
711 if (n == NULL) {
712 if (size + sizeof(struct binder_buffer) + 4 >= buffer_size)
713 buffer_size = size; /* no room for other buffers */
714 else
715 buffer_size = size + sizeof(struct binder_buffer);
716 }
717 end_page_addr =
718 (void *)PAGE_ALIGN((uintptr_t)buffer->data + buffer_size);
719 if (end_page_addr > has_page_addr)
720 end_page_addr = has_page_addr;
721 if (binder_update_page_range(proc, 1,
722 (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL))
723 return NULL;
The alignment here is confusing because we don't align buffer->data
below.
binder_update_page_range allocates and maps pages. buffer->data will
point to a range within the allocated pages.
724
725 rb_erase(best_fit, &proc->free_buffers);
726 buffer->free = 0;
727 binder_insert_allocated_buffer(proc, buffer);
728 if (buffer_size != size) {
729 struct binder_buffer *new_buffer = (void *)buffer->data + size;
^^^^^^^^^^^^^^^^^^^^
I don't really understand when buffer->data has to be page aligned and
when not. This code has very few comments.
buffer->data never needs to be page aligned. The code above rounds to
page boundaries to allocate new pages as needed.
730
731 list_add(&new_buffer->entry, &buffer->entry);
732 new_buffer->free = 1;
733 binder_insert_free_buffer(proc, new_buffer);
734 }
Does the stop_on_user_error option work? There should be some
documentation for this stuff.
From: Rom Lemarchand <hidden> Date: 2014-10-22 03:10:35
On the topic of maintainers for binder: both Arve Hjønnevåg
(arve-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org) and Riley Andrews (riandrews-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org) have
volunteered to be co-maintainers with Greg.
We would also like to make kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org the maintainer of
the whole android directory.
From: Joe Perches <joe@perches.com> Date: 2014-10-22 03:16:56
On Tue, 2014-10-21 at 20:10 -0700, Rom Lemarchand wrote:
On the topic of maintainers for binder: both Arve Hjønnevåg
(arve@android.com) and Riley Andrews (riandrews@android.com) have
volunteered to be co-maintainers with Greg.
We would also like to make kernel-team@android.com the maintainer of
the whole android directory.
It's generally better to have people as maintainers than
using nameless email addresses.
From: Christoph Hellwig <hch@infradead.org> Date: 2014-10-17 09:43:31
On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote:
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
NAK. It's complete rubbish and does things to the FD code that it
really shouldn't. Android needs to completely redo the interface, and
there's been absolutely no work towards that.
This is exactly the sort of attitude I feared about when you started the
whole staging concepts, and it sounds like a good reason to disband
staging entirely, given that it's been mostly useless except for
boasting peoples commit statistics.
On Fri, Oct 17, 2014 at 02:43:29AM -0700, Christoph Hellwig wrote:
On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote:
quoted
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
NAK. It's complete rubbish and does things to the FD code that it
really shouldn't. Android needs to completely redo the interface, and
there's been absolutely no work towards that.
There is now work to resolve the interface, it requires someone who has
the rights to push to Android userspace. But that is going to be a
"total rewrite", and until then, this code needs to be used, no matter
how much we hate this.
This is exactly the sort of attitude I feared about when you started the
whole staging concepts, and it sounds like a good reason to disband
staging entirely, given that it's been mostly useless except for
boasting peoples commit statistics.
I know you hate staging, which is fine, it's not there for you. It's
there for others, and so much good stuff has happened with it, that I'm
not going to give it up.
But this code is different. It's something like a random driver that a
distro has been carrying for 5+ years that it has to ship due to
userspace that relies on it, so the api can't be changed. We accept
drivers like that at times, and we don't drop really old and crappy
drivers either, as long as someone is using it, and it is
self-contained.
I'll step up to maintain this and handle the interactions between grumpy
kernel developers who look at the code, and the Android developers who
are stuck with this monstrosity.
thanks,
greg k-h
From: Christoph Hellwig <hch@infradead.org> Date: 2014-10-21 10:46:48
On Mon, Oct 20, 2014 at 06:04:50AM +0800, Greg Kroah-Hartman wrote:
There is now work to resolve the interface, it requires someone who has
the rights to push to Android userspace. But that is going to be a
"total rewrite", and until then, this code needs to be used, no matter
how much we hate this.
It helps to qualify why it absolutely has to, and why this is different
from other interfaces we haven't merged. Is this the last building
block to run upstream Linux on a common Android device out of the box
without needing any patches or out of tree drivers? Is there any other
good reason I might have missed.
To convince other people that merging a piece like this absolutely needs
to get merged I'd suggest you start with presenting factual argument,
and then let the broader audience weight their merrits.
I think with the known problems of the code, and the fact that the
real user ABI is a library anyway the stakes are quite high here.
So as a start please prepare a list of arguments, a detailed description
of the ABI, and post a proper patch (not a move) that suggests adding
this driver to all the relevant lists (most importantly linux-fsdevel
and linux-api) so that people with the right experience can review it.
On Thursday 16 October 2014 14:47:41 Greg Kroah-Hartman wrote:
From: Greg Kroah-Hartman <redacted>
The Android binder code has been "stable" for many years now. No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.
Signed-off-by: Greg Kroah-Hartman <redacted>
---
This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1
I'm worried about the user interface: since graduating binder out of
staging with the existing ioctl interface has never been discussed
as a real option and (I assume) everybody expected the way forward
would be to have a replacement, I don't think it ever received the
attention it should have. Specific concerns are:
- I don't think there has been an audit of which subset of the API
is actually required. IIRC, it was said initially that actual
applications don't use all the features, and that we should have
a smaller attack surface.
- Using kernel pointers in user space interfaces is an information
leak that can be used to construct an exploit. (I don't know if
this is still used that way, I think it was doing it last
time I checked).
- The driver supports two versions of the user interface (v7 and
v8), but only one of them can be selected at compile-time, and
an 'allmodconfig' kernel will only include the deprecated one
on 32-bit machines.
- The implementation is not namespace-aware and will cause
information to be shared across containers in a potentially
harmful way.
If we graduate the driver from staging, it should IMHO at least
be useful in containers to run Android user space safely.
Arnd