[PATCH] staging: android: binder: move to the "real" part of the kernel

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+***@public.gmane.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.

Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+***@public.gmane.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


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%)

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 1a693d3f9d51..569ff7886dc3 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -182,4 +182,6 @@ source "drivers/ras/Kconfig"

source "drivers/thunderbolt/Kconfig"

+source "drivers/android/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index ebee55537a05..60d19820a4d4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -161,3 +161,4 @@ obj-$(CONFIG_POWERCAP) += powercap/
obj-$(CONFIG_MCB) += mcb/
obj-$(CONFIG_RAS) += ras/
obj-$(CONFIG_THUNDERBOLT) += thunderbolt/
+obj-$(CONFIG_ANDROID) += android/
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
new file mode 100644
index 000000000000..bdfc6c6f4f5a
--- /dev/null
+++ b/drivers/android/Kconfig
@@ -0,0 +1,37 @@
+menu "Android"
+
+config ANDROID
+ bool "Android Drivers"
+ ---help---
+ Enable support for various drivers needed on the Android platform
+
+if ANDROID
+
+config ANDROID_BINDER_IPC
+ bool "Android Binder IPC Driver"
+ depends on MMU
+ default n
+ ---help---
+ Binder is used in Android for both communication between processes,
+ and remote method invocation.
+
+ This means one Android process can call a method/routine in another
+ Android process, using Binder to identify, invoke and pass arguments
+ between said processes.
+
+config ANDROID_BINDER_IPC_32BIT
+ bool
+ depends on !64BIT && ANDROID_BINDER_IPC
+ default y
+ ---help---
+ The Binder API has been changed to support both 32 and 64bit
+ applications in a mixed environment.
+
+ Enable this to support an old 32-bit Android user-space (v4.4 and
+ earlier).
+
+ Note that enabling this will break newer Android user-space.
+
+endif # if ANDROID
+
+endmenu
diff --git a/drivers/android/Makefile b/drivers/android/Makefile
new file mode 100644
index 000000000000..3b7e4b072c58
--- /dev/null
+++ b/drivers/android/Makefile
@@ -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.c
similarity index 100%
rename from drivers/staging/android/binder.c
rename to drivers/android/binder.c
diff --git a/drivers/staging/android/binder.h b/drivers/android/binder.h
similarity index 95%
rename from drivers/staging/android/binder.h
rename to drivers/android/binder.h
index eb0834656dfe..5dc6a66b0665 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/android/binder.h
@@ -24,7 +24,7 @@
#define BINDER_IPC_32BIT 1
#endif

-#include "uapi/binder.h"
+#include <uapi/linux/android/binder.h>

#endif /* _LINUX_BINDER_H */

diff --git a/drivers/staging/android/binder_trace.h b/drivers/android/binder_trace.h
similarity index 100%
rename from drivers/staging/android/binder_trace.h
rename to drivers/android/binder_trace.h
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 7a0e28852965..7e012f37792b 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -1,37 +1,7 @@
menu "Android"

-config ANDROID
- bool "Android Drivers"
- ---help---
- Enable support for various drivers needed on the Android platform
-
if ANDROID

-config ANDROID_BINDER_IPC
- bool "Android Binder IPC Driver"
- depends on MMU
- default n
- ---help---
- Binder is used in Android for both communication between processes,
- and remote method invocation.
-
- This means one Android process can call a method/routine in another
- Android process, using Binder to identify, invoke and pass arguments
- between said processes.
-
-config ANDROID_BINDER_IPC_32BIT
- bool
- depends on !64BIT && ANDROID_BINDER_IPC
- default y
- ---help---
- The Binder API has been changed to support both 32 and 64bit
- applications in a mixed environment.
-
- Enable this to support an old 32-bit Android user-space (v4.4 and
- earlier).
-
- Note that enabling this will break newer Android user-space.
-
config ASHMEM
bool "Enable the Anonymous Shared Memory Subsystem"
default n
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 517ad5ffa429..479b2b86f8c8 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -2,7 +2,6 @@ ccflags-y += -I$(src) # needed for trace events

obj-y += ion/

-obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o
obj-$(CONFIG_ASHMEM) += ashmem.o
obj-$(CONFIG_ANDROID_LOGGER) += logger.o
obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 70e150ebc6c9..1bbaf6457861 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -1,4 +1,5 @@
# UAPI Header export list
+header-y += android/
header-y += byteorder/
header-y += can/
header-y += caif/
diff --git a/include/uapi/linux/android/Kbuild b/include/uapi/linux/android/Kbuild
new file mode 100644
index 000000000000..ca011eec252a
--- /dev/null
+++ b/include/uapi/linux/android/Kbuild
@@ -0,0 +1,2 @@
+# UAPI Header export list
+header-y += binder.h
diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.h
similarity index 100%
rename from drivers/staging/android/uapi/binder.h
rename to include/uapi/linux/android/binder.h

[Michael Kerrisk (man-pages)]

On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
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

Post by Greg Kroah-Hartman
---
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%)
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 1a693d3f9d51..569ff7886dc3 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -182,4 +182,6 @@ source "drivers/ras/Kconfig"
source "drivers/thunderbolt/Kconfig"
+source "drivers/android/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index ebee55537a05..60d19820a4d4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -161,3 +161,4 @@ obj-$(CONFIG_POWERCAP) += powercap/
obj-$(CONFIG_MCB) += mcb/
obj-$(CONFIG_RAS) += ras/
obj-$(CONFIG_THUNDERBOLT) += thunderbolt/
+obj-$(CONFIG_ANDROID) += android/
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
new file mode 100644
index 000000000000..bdfc6c6f4f5a
--- /dev/null
+++ b/drivers/android/Kconfig
@@ -0,0 +1,37 @@
+menu "Android"
+
+config ANDROID
+ bool "Android Drivers"
+ ---help---
+ Enable support for various drivers needed on the Android platform
+
+if ANDROID
+
+config ANDROID_BINDER_IPC
+ bool "Android Binder IPC Driver"
+ depends on MMU
+ default n
+ ---help---
+ Binder is used in Android for both communication between processes,
+ and remote method invocation.
+
+ This means one Android process can call a method/routine in another
+ Android process, using Binder to identify, invoke and pass arguments
+ between said processes.
+
+config ANDROID_BINDER_IPC_32BIT
+ bool
+ depends on !64BIT && ANDROID_BINDER_IPC
+ default y
+ ---help---
+ The Binder API has been changed to support both 32 and 64bit
+ applications in a mixed environment.
+
+ Enable this to support an old 32-bit Android user-space (v4.4 and
+ earlier).
+
+ Note that enabling this will break newer Android user-space.
+
+endif # if ANDROID
+
+endmenu
diff --git a/drivers/android/Makefile b/drivers/android/Makefile
new file mode 100644
index 000000000000..3b7e4b072c58
--- /dev/null
+++ b/drivers/android/Makefile
@@ -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.c
similarity index 100%
rename from drivers/staging/android/binder.c
rename to drivers/android/binder.c
diff --git a/drivers/staging/android/binder.h b/drivers/android/binder.h
similarity index 95%
rename from drivers/staging/android/binder.h
rename to drivers/android/binder.h
index eb0834656dfe..5dc6a66b0665 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/android/binder.h
@@ -24,7 +24,7 @@
#define BINDER_IPC_32BIT 1
#endif
-#include "uapi/binder.h"
+#include <uapi/linux/android/binder.h>
#endif /* _LINUX_BINDER_H */
diff --git a/drivers/staging/android/binder_trace.h b/drivers/android/binder_trace.h
similarity index 100%
rename from drivers/staging/android/binder_trace.h
rename to drivers/android/binder_trace.h
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 7a0e28852965..7e012f37792b 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -1,37 +1,7 @@
menu "Android"
-config ANDROID
- bool "Android Drivers"
- ---help---
- Enable support for various drivers needed on the Android platform
-
if ANDROID
-config ANDROID_BINDER_IPC
- bool "Android Binder IPC Driver"
- depends on MMU
- default n
- ---help---
- Binder is used in Android for both communication between processes,
- and remote method invocation.
-
- This means one Android process can call a method/routine in another
- Android process, using Binder to identify, invoke and pass arguments
- between said processes.
-
-config ANDROID_BINDER_IPC_32BIT
- bool
- depends on !64BIT && ANDROID_BINDER_IPC
- default y
- ---help---
- The Binder API has been changed to support both 32 and 64bit
- applications in a mixed environment.
-
- Enable this to support an old 32-bit Android user-space (v4.4 and
- earlier).
-
- Note that enabling this will break newer Android user-space.
-
config ASHMEM
bool "Enable the Anonymous Shared Memory Subsystem"
default n
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 517ad5ffa429..479b2b86f8c8 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -2,7 +2,6 @@ ccflags-y += -I$(src) # needed for trace events
obj-y += ion/
-obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o
obj-$(CONFIG_ASHMEM) += ashmem.o
obj-$(CONFIG_ANDROID_LOGGER) += logger.o
obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 70e150ebc6c9..1bbaf6457861 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -1,4 +1,5 @@
# UAPI Header export list
+header-y += android/
header-y += byteorder/
header-y += can/
header-y += caif/
diff --git a/include/uapi/linux/android/Kbuild b/include/uapi/linux/android/Kbuild
new file mode 100644
index 000000000000..ca011eec252a
--- /dev/null
+++ b/include/uapi/linux/android/Kbuild
@@ -0,0 +1,2 @@
+# UAPI Header export list
+header-y += binder.h
diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.h
similarity index 100%
rename from drivers/staging/android/uapi/binder.h
rename to include/uapi/linux/android/binder.h
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Greg Kroah-Hartman]

Post by Michael Kerrisk (man-pages)
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
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

[Dan Carpenter]

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

[John Stultz]

On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
The Android binder code has been "stable" for many years now. No matter

Well, ignoring the ABI break that landed in the last year. :)

Post by Greg Kroah-Hartman
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.
---
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

[Greg Kroah-Hartman]

Post by John Stultz
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
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" :)

Post by John Stultz

Post by Greg Kroah-Hartman
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

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.

Post by John Stultz
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.

Post by John Stultz
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.

Post by John Stultz
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.

Post by John Stultz
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

[John Stultz]

On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman

Post by John Stultz
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
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" :)

Post by John Stultz

Post by Greg Kroah-Hartman
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

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.

Post by Greg Kroah-Hartman

Post by John Stultz
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.

Post by Greg Kroah-Hartman

Post by John Stultz
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?

Post by Greg Kroah-Hartman

Post by John Stultz
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.

Post by Greg Kroah-Hartman

Post by John Stultz
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

[Greg Kroah-Hartman]

Post by John Stultz
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman

Post by John Stultz
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman

Post by Greg Kroah-Hartman
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" :)

Post by John Stultz

Post by Greg Kroah-Hartman
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

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.

Post by John Stultz
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.

Post by John Stultz
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 :)

Post by John Stultz
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

[One Thousand Gnomes]

Post by Greg Kroah-Hartman
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

Post by Greg Kroah-Hartman

Post by John Stultz
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".

Post by Greg Kroah-Hartman
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

[Greg Kroah-Hartman]

Post by One Thousand Gnomes

Post by Greg Kroah-Hartman
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.

Post by One Thousand Gnomes
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".

Post by One Thousand Gnomes

Post by Greg Kroah-Hartman

Post by John Stultz
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...

Post by One Thousand Gnomes

Post by Greg Kroah-Hartman
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

[Dan Carpenter]

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

[Greg Kroah-Hartman]

Post by Dan Carpenter
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?

Post by Dan Carpenter
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

[Dan Carpenter]

Post by Greg Kroah-Hartman

Post by Dan Carpenter
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

[Christoph Hellwig]

Post by Greg Kroah-Hartman
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.

[Greg Kroah-Hartman]

Post by Christoph Hellwig

Post by Greg Kroah-Hartman
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.

Post by Christoph Hellwig
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

[Arnd Bergmann]

Post by Greg Kroah-Hartman
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.
---
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