Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Fri, Dec 13, 2013 at 7:14 AM, Arve Hjønnevåg wrote: > On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila > wrote: >> On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg wrote: >>> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila >>> wrote: On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: > > Assuming you are talking about a kernel compat layer that translates > the flat_binder_object structs as they pass between 32 bit and 64 bit > processes, that will not always work. The data portion of the message > sometimes contain size values that are invisible to the kernel, but > these values will be wrong if the kernel move data to make room for a > different size flat_binder_object. > Hi Arve, Yes, I was talking about translating flat_binder_objects. I understand the potential issue for the user data payload, however, since most applications will use libbinder, the only problematic case is readIntPtr/writeIntPtr, which we can deprecate and convert applications that use it to readInt64. AFAICS there is only one user in the AOSP for this API (libmedia). If you are referring to data blobs that application parses I don't think there is anything we can do, even at libbinder level. Can you give me an example of the sort of problems you see? Thanks, Tavi >>> >>> The specific problem I was told about can be found in >>> frameworks/base/core/java/android/os/Bundle.java, but there could be >>> other. The size of the bundle is stored in the parcel so the end of >>> the bundle will be wrong if the bundle contains a flat_binder_object >>> that the driver changes the size of. However, since the sending >>> process gets the parcel size from libbinder, changing libbinder to >>> always use the 64 bit version of flat_binder_object should work with >>> this code. >>> >> >> AFAICS the bundle size is stored as an int32 so there is no bit width >> issues between the sending and receiving process. >> >> When I mentioned that flat_binder_objects will be translated, I meant >> the whole transaction will be translated to preserve its user payload >> intact. Something like this: >> >> 32bit64bit >> ++++ >> | o o oo oxx| -> | oo oo| >> ++++ >> >> where o are binder objects, spaces are user data and x,y are offsets >> pointers to binder objects (they are size_t so they need translation >> as well). >> >> As long as the application does not use absolute offsets in the >> payload and as long as the data types stored in the payload are fixed >> bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of >> intptr) doing the translation in kernel should be fine. I checked >> libbinder and both assumptions seems to be true (except in a few cases >> for the later which I already mentioned) >> >> So, what am I missing? > > The bundle size is wrong in the receiving process if the driver change > the size of a flat_binder_object stored in the bundle. > > A simplified example where a flat_binder_object is a single pointer, > and a bundle only adds a size to the data stored: > If you send a bundle with an object and an int followed by an > int from a 32 bit process to a 64 bit process you would get > this transformation of the data (offsets not shown): > bundle(obj, inta), int(intb) => parcel_32(8,obj,inta,intb) => > parcel_64(8,objl,objh,inta,intb) => bundle(obj), int(inta), int(intb) > > The first bundle argument is missing an item in the receiving process, > and the second argument is instead of . > Thanks Arve, I understand the problem now. When I first looked at the code I thought that the bundle stores the number of objects instead of its size. In this case, changing libbinder (and ServiceManager) to use 64bit structures when talking with 64bit kernels seems the only reasonable approach. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila wrote: > On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg wrote: >> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila >> wrote: >>> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. >>> >>> Hi Arve, >>> >>> Yes, I was talking about translating flat_binder_objects. >>> >>> I understand the potential issue for the user data payload, however, >>> since most applications will use libbinder, the only problematic case >>> is readIntPtr/writeIntPtr, which we can deprecate and convert >>> applications that use it to readInt64. AFAICS there is only one user >>> in the AOSP for this API (libmedia). >>> >>> If you are referring to data blobs that application parses I don't >>> think there is anything we can do, even at libbinder level. >>> >>> Can you give me an example of the sort of problems you see? >>> >>> Thanks, >>> Tavi >> >> The specific problem I was told about can be found in >> frameworks/base/core/java/android/os/Bundle.java, but there could be >> other. The size of the bundle is stored in the parcel so the end of >> the bundle will be wrong if the bundle contains a flat_binder_object >> that the driver changes the size of. However, since the sending >> process gets the parcel size from libbinder, changing libbinder to >> always use the 64 bit version of flat_binder_object should work with >> this code. >> > > AFAICS the bundle size is stored as an int32 so there is no bit width > issues between the sending and receiving process. > > When I mentioned that flat_binder_objects will be translated, I meant > the whole transaction will be translated to preserve its user payload > intact. Something like this: > > 32bit64bit > ++++ > | o o oo oxx| -> | oo oo| > ++++ > > where o are binder objects, spaces are user data and x,y are offsets > pointers to binder objects (they are size_t so they need translation > as well). > > As long as the application does not use absolute offsets in the > payload and as long as the data types stored in the payload are fixed > bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of > intptr) doing the translation in kernel should be fine. I checked > libbinder and both assumptions seems to be true (except in a few cases > for the later which I already mentioned) > > So, what am I missing? The bundle size is wrong in the receiving process if the driver change the size of a flat_binder_object stored in the bundle. A simplified example where a flat_binder_object is a single pointer, and a bundle only adds a size to the data stored: If you send a bundle with an object and an int followed by an int from a 32 bit process to a 64 bit process you would get this transformation of the data (offsets not shown): bundle(obj, inta), int(intb) => parcel_32(8,obj,inta,intb) => parcel_64(8,objl,objh,inta,intb) => bundle(obj), int(inta), int(intb) The first bundle argument is missing an item in the receiving process, and the second argument is instead of . -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg wrote: > On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila > wrote: >> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: >>> >>> Assuming you are talking about a kernel compat layer that translates >>> the flat_binder_object structs as they pass between 32 bit and 64 bit >>> processes, that will not always work. The data portion of the message >>> sometimes contain size values that are invisible to the kernel, but >>> these values will be wrong if the kernel move data to make room for a >>> different size flat_binder_object. >>> >> >> Hi Arve, >> >> Yes, I was talking about translating flat_binder_objects. >> >> I understand the potential issue for the user data payload, however, >> since most applications will use libbinder, the only problematic case >> is readIntPtr/writeIntPtr, which we can deprecate and convert >> applications that use it to readInt64. AFAICS there is only one user >> in the AOSP for this API (libmedia). >> >> If you are referring to data blobs that application parses I don't >> think there is anything we can do, even at libbinder level. >> >> Can you give me an example of the sort of problems you see? >> >> Thanks, >> Tavi > > The specific problem I was told about can be found in > frameworks/base/core/java/android/os/Bundle.java, but there could be > other. The size of the bundle is stored in the parcel so the end of > the bundle will be wrong if the bundle contains a flat_binder_object > that the driver changes the size of. However, since the sending > process gets the parcel size from libbinder, changing libbinder to > always use the 64 bit version of flat_binder_object should work with > this code. > AFAICS the bundle size is stored as an int32 so there is no bit width issues between the sending and receiving process. When I mentioned that flat_binder_objects will be translated, I meant the whole transaction will be translated to preserve its user payload intact. Something like this: 32bit64bit ++++ | o o oo oxx| -> | oo oo| ++++ where o are binder objects, spaces are user data and x,y are offsets pointers to binder objects (they are size_t so they need translation as well). As long as the application does not use absolute offsets in the payload and as long as the data types stored in the payload are fixed bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of intptr) doing the translation in kernel should be fine. I checked libbinder and both assumptions seems to be true (except in a few cases for the later which I already mentioned) So, what am I missing? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila wrote: > On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: >> >> Assuming you are talking about a kernel compat layer that translates >> the flat_binder_object structs as they pass between 32 bit and 64 bit >> processes, that will not always work. The data portion of the message >> sometimes contain size values that are invisible to the kernel, but >> these values will be wrong if the kernel move data to make room for a >> different size flat_binder_object. >> > > Hi Arve, > > Yes, I was talking about translating flat_binder_objects. > > I understand the potential issue for the user data payload, however, > since most applications will use libbinder, the only problematic case > is readIntPtr/writeIntPtr, which we can deprecate and convert > applications that use it to readInt64. AFAICS there is only one user > in the AOSP for this API (libmedia). > > If you are referring to data blobs that application parses I don't > think there is anything we can do, even at libbinder level. > > Can you give me an example of the sort of problems you see? > > Thanks, > Tavi The specific problem I was told about can be found in frameworks/base/core/java/android/os/Bundle.java, but there could be other. The size of the bundle is stored in the parcel so the end of the bundle will be wrong if the bundle contains a flat_binder_object that the driver changes the size of. However, since the sending process gets the parcel size from libbinder, changing libbinder to always use the 64 bit version of flat_binder_object should work with this code. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg wrote: > > Assuming you are talking about a kernel compat layer that translates > the flat_binder_object structs as they pass between 32 bit and 64 bit > processes, that will not always work. The data portion of the message > sometimes contain size values that are invisible to the kernel, but > these values will be wrong if the kernel move data to make room for a > different size flat_binder_object. > Hi Arve, Yes, I was talking about translating flat_binder_objects. I understand the potential issue for the user data payload, however, since most applications will use libbinder, the only problematic case is readIntPtr/writeIntPtr, which we can deprecate and convert applications that use it to readInt64. AFAICS there is only one user in the AOSP for this API (libmedia). If you are referring to data blobs that application parses I don't think there is anything we can do, even at libbinder level. Can you give me an example of the sort of problems you see? Thanks, Tavi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Mon, Dec 9, 2013 at 7:01 PM, Octavian Purdila wrote: > On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg wrote: >> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 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 >> wrote: >> >> >> > 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"? >>> >> >> Yes libbinder will have to be changed to support calls between 32 bit >> and 64 bit processes, so I don't see much value in a patchset that >> only supports all 32 bit or all 64 bit processes. If user space is >> fixed to use 64 bit pointers on a 64 bit system, then much of the code >> added in this patchset becomes useless (and probably harmful as it >> appears to prevent 32 bit processes from communicating with 64 bit >> processes). >> > > Hi, > > Coincidentally, I have been working on a compat layer myself lately. > It is implemented in the binder driver with no changes in libbinder > and it includes support for mixed mode. > > Unless you think that the kernel compat layer is a dead end, I can > post the patches here for review. IMO the kernel compat layer gives > you greater flexibility because it keeps the 32bit ABI unchanged. Of > course it comes with the price of increased complexity. > > Thanks, > Tavi Assuming you are talking about a kernel compat layer that translates the flat_binder_object structs as they pass between 32 bit and 64 bit processes, that will not always work. The data portion of the message sometimes contain size values that are invisible to the kernel, but these values will be wrong if the kernel move data to make room for a different size flat_binder_object. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg wrote: > On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 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 >>> >> wrote: >>> >> >>> >> >>> >> > 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"? >> > > Yes libbinder will have to be changed to support calls between 32 bit > and 64 bit processes, so I don't see much value in a patchset that > only supports all 32 bit or all 64 bit processes. If user space is > fixed to use 64 bit pointers on a 64 bit system, then much of the code > added in this patchset becomes useless (and probably harmful as it > appears to prevent 32 bit processes from communicating with 64 bit > processes). > Hi, Coincidentally, I have been working on a compat layer myself lately. It is implemented in the binder driver with no changes in libbinder and it includes support for mixed mode. Unless you think that the kernel compat layer is a dead end, I can post the patches here for review. IMO the kernel compat layer gives you greater flexibility because it keeps the 32bit ABI unchanged. Of course it comes with the price of increased complexity. Thanks, Tavi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Thu, Dec 05, 2013 at 06:31:25PM +, Serban Constantinescu wrote: > Hi all, > > Thanks for your feedback! Sadly enough, being in a different > time-zone, is not useful. > > Sorry for the confusion related to why is this patch needed or not. I > should highlight a bit more what is the patch enabling and what > would be the different alternatives, at least from my perspective. > > *64bit kernel/ 32bit userspace* > > This patch series adds support for 32bit userspace running on 64bit > kernels. Thus by applying this patch to your kernel you will be able > to use any existing 32bit Android userspace on your 64bit platform, > running a 64bit kernel. That is pure 32bit userspace with no 64bit > support! > > This means *no modifications to the 32bit userspace*. Therefore any > applications or userspace side drivers, that uses the binder > interface at a native level will not have to be modified. These kind > of applications are not "good citizens" - the native binder API is > not exposed in the Android NDK. However I do not know how many > applications do this and if breaking the compatibility is a concernt > for 32bit userspace running on 64bit kernels. Um, I thought we were assured that the _only_ user of the kernel binder interface was libbinder. If other programs are touching this interface "directly", you have bigger problems then just a 32/64 bit issue, and that needs to be fixed. In other words, you should be totally safe in modifying libbinder as well as the kernel interface at the same time. Now if you really want to do that or not is another issue, but it should be possible (and in my opinion, the better option...) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
Hi all,
Thanks for your feedback! Sadly enough, being in a different time-zone,
is not useful.
Sorry for the confusion related to why is this patch needed or not. I
should highlight a bit more what is the patch enabling and what would be
the different alternatives, at least from my perspective.
*64bit kernel/ 32bit userspace*
This patch series adds support for 32bit userspace running on 64bit
kernels. Thus by applying this patch to your kernel you will be able to
use any existing 32bit Android userspace on your 64bit platform, running
a 64bit kernel. That is pure 32bit userspace with no 64bit support!
This means *no modifications to the 32bit userspace*. Therefore any
applications or userspace side drivers, that uses the binder interface
at a native level will not have to be modified. These kind of
applications are not "good citizens" - the native binder API is not
exposed in the Android NDK. However I do not know how many applications
do this and if breaking the compatibility is a concernt for 32bit
userspace running on 64bit kernels.
Below you will find more information on one of the alternative
solutions, the userspace compat.
*32bit kernel/ 32bit userspace*
*64bit kernel/ 64bit userspace*
My last series of binder patches, accepted into 3.12 revision of the
Linux kernel, audits the binder interface for 64bit. A kernel with these
changes applied can be used with a pure 64bit userspace (this does not
include support for 32bit applications coexisting with 64bit ones). The
userspace side needs trivial changes to libbinder.so and servicemanager,
that I will upstream ASAP to AOSP, and that work for 32/32 systems and
64/64 systems without modifying the existing 32bit interface ABI (most
of the changes are related to uint32_t != void *).
Author: Serban Constantinescu
Date: Thu Jul 4 10:54:48 2013 +0100
staging: android: binder: fix binder interface for 64bit compat layer
*64bit kernel/ 64bit coexisting with 32bit userspace
Please note that *none of the above solutions fix this yet*. To
understand why this is a special case please take a look at
how the binder driver works, seen from a high level perspective:
ServiceManager
App1 <-> App2
Thus, for two apps to exchange information between each other they will
have to communicate with the userspace governor, ServiceManager. All the
interaction between App1, App2 and ServiceManager is done through a
combination of libbinder.so (Userspace HAL) and the binder driver. Note
that there can only been one ServiceManager process, that is set during
the userspace boot process.
Now lets consider the case that Arve described earlier 32bit
Applications coexisting with 64bit ones. In this case all the commands
and interface used will have to be the same, the ones used by the 64bit
ServiceManager. For this the kernel entry point for 32bit compat will
have to be changed to:
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3893,9 +3893,13 @@ static const struct file_operations binder_fops = {
.owner = THIS_MODULE,
.poll = binder_poll,
.unlocked_ioctl = binder_ioctl,
-#ifdef CONFIG_COMPAT
+#if defined(CONFIG_COMPAT) && !defined(COEXIST_32BIT_64BIT)
.compat_ioctl = compat_binder_ioctl,
#endif
+
+#if defined(COEXIST_32BIT_64BIT)
+ .compat_ioctl = binder_ioctl,
+#endif
.mmap = binder_mmap,
.open = binder_open,
.flush = binder_flush,
thus from the perspective of a kernel that works with a 64bit userspace
where 64bit applications coexist with 32bit applications the handling
will be the same for both 32bit and 64bit processes. This will also
involve modifying libbinder.so to use 64bit structures like:
--- a/libs/binder/IPCThreadState.cpp
+++ b/libs/binder/IPCThreadState.cpp
+#if defined(COEXIST_32BIT_64BIT) && !defined(__LP64__)
+/*
+ * For running 32-bit processes on a 64-bit system, make the interaction with
the
+ * binder driver the same from this, when built for 32-bit, as for a 64-bit
process.
+ */
+struct coexist_binder_write_read {
+uint64_twrite_size; /* __LP64__ size_t: bytes to write */
+uint64_twrite_consumed; /* __LP64__ size_t: bytes consumed by driver */
+uint64_twrite_buffer; /* __LP64__ unsigned long */
+uint64_tread_size; /* __LP64__ size_t: bytes to read */
+uint64_tread_consumed; /* __LP64__ size_t: bytes consumed by driver */
+uint64_tread_buffer;/* __LP64__ unsigned long */
+};
"Good citizen" Android applications will go through these changes (since
they should only use the binder Java API), and so shouldn't encounter
any problem. Any 32bit applications that use the binder interface at a
native level will not work with this model (they should not do so!)·
This is exactly what the kernel compat "guarantees" *only* for 64/32bit
systems.
The cleaner solution from the binder kernel perspective is to hoo
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 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 >> >> wrote: >> >> >> >> >> >> > 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"? > Yes libbinder will have to be changed to support calls between 32 bit and 64 bit processes, so I don't see much value in a patchset that only supports all 32 bit or all 64 bit processes. If user space is fixed to use 64 bit pointers on a 64 bit system, then much of the code added in this patchset becomes useless (and probably harmful as it appears to prevent 32 bit processes from communicating with 64 bit processes). > Or does this patch series mean that no userspace code is changed? Is > that a "requirement" here? > I don't think we need to support old 32 bit userspace framework code on a 64 bit system. I think it is more important to not prevent mixed mode systems. -- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
> None of this (the patch series or the original code) is mine. My Sorry mistraced the attribution sequence > question was more of a general one on designing ioctls, as well as > concerns with changing the existing 32-bit api. I think in general my advice would be: If its already been screwed up - sort the structures out for the 64bit version - make the 32bit version and compat ones clean copies of what they are now - don't try and pull stunts with alignof and other such tricks because - some poor bugger will have to debug it one day (and it might be you some years after you forget how it worked) - most of our security holes show up in complex data parsing paths - figure out whether its better to do compat fixups or just have 64bit use new structures and new ioctl numbers with the 32bit ones working but with 32bit offsets regardless of 32/64bit CPU mode. and for new stuff - design for 64bit safety in advance - watch the padding rules and user alignment - consider leaving some spare zero space on the end of the structs Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 4:02 PM, Greg KH wrote: > On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote: >> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 >> >> 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 >> >> >> wrote: >> >> >> >> >> >> >> >> >> > 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? We have explicitly said that Android userspace does not require a specific kernel version. I expect to see KitKat devices running at least 3.4, 3.10, and whatever is the next long term stable version. Sometimes new kernels need userspace support, but we try to avoid that as much as possible. The last major one I can remember was removing early suspend in 3.4, but in that case I provided a compat 3.4 branch to allow using 3.4 with an older userspace. Changing the binder api is not completely unsupportable for old versions, my guess is some people would just ship with a new kernel with the binder changes reverted. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote: > On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 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 > >> >> wrote: > >> >> > >> >> > >> >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 3:21 PM, One Thousand Gnomes
wrote:
> On Wed, 4 Dec 2013 10:35:54 -0800
> Greg KH wrote:
>
>> On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
>> > +#define size_helper(x) ({ \
>> > + size_t __size; \
>> > + if (!is_compat_task()) \
>> > + __size = sizeof(x); \
>> > + else if (sizeof(x) == sizeof(struct flat_binder_object))\
>> > + __size = sizeof(struct compat_flat_binder_object); \
>> > + else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
>> > + __size = sizeof(struct compat_binder_transaction_data); \
>> > + else if (sizeof(x) == sizeof(size_t)) \
>> > + __size = sizeof(compat_size_t); \
>> > + else\
>> > +BUG(); \
>> > + __size; \
>> > + })
>>
>> Ick.
>>
>> First off, no driver should ever be able to crash the kernel, which you
>> just did.
>
> And which would appear to mean that this code hasn't actually been
> tested - at least not properly with error cases ?
>
> You talk about type safety too but your code is already full of
> "put_user(node->ptr, (void * __user *)ptr))"
None of this (the patch series or the original code) is mine. My
question was more of a general one on designing ioctls, as well as
concerns with changing the existing 32-bit api.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, 4 Dec 2013 10:35:54 -0800
Greg KH wrote:
> On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
> > +#define size_helper(x) ({ \
> > + size_t __size; \
> > + if (!is_compat_task()) \
> > + __size = sizeof(x); \
> > + else if (sizeof(x) == sizeof(struct flat_binder_object))\
> > + __size = sizeof(struct compat_flat_binder_object); \
> > + else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
> > + __size = sizeof(struct compat_binder_transaction_data); \
> > + else if (sizeof(x) == sizeof(size_t)) \
> > + __size = sizeof(compat_size_t); \
> > + else\
> > +BUG(); \
> > + __size; \
> > + })
>
> Ick.
>
> First off, no driver should ever be able to crash the kernel, which you
> just did.
And which would appear to mean that this code hasn't actually been
tested - at least not properly with error cases ?
You talk about type safety too but your code is already full of
"put_user(node->ptr, (void * __user *)ptr))"
The binder_copy_to_user thing is pretty confusingly named - it sounds
like a wrapper but is in fact a whole set of operations with two
different values and extra cookie structures and the like
Would something like
binder_put_userptr(mode->ptr, &ptr)
perhaps be a shade easier to follow as a set of changes, and less clunky ?
And 3/9 you could have done a clean up .. instead of replacing endless
repeats of
- cmd == BC_INCREFS_DONE ?
- "BC_INCREFS_DONE" :
- "BC_ACQUIRE_DONE",
+ acquire ?
+ "BC_ACQUIRE_DONE" :
+ "BC_INCREFS_DONE",
couldn't you do that bit in just one place ?
Ditto
- cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+ request ?
"BC_REQUEST_DEATH_NOTIFICATION" :
"BC_CLEAR_DEATH_NOTIFICATION",
The "_helper" stuff with type and size magic also really obfuscates the
code horribly
+static inline struct flat_binder_object *copy_flat_binder_object(void
__user *ptr) +{
+ return (struct flat_binder_object *)ptr;
+}
What were you arguing about type safety again ?
While I'm tempted to answer "and that children is what happens when you
don't take your interfaces mainstream and peer review them in the first
place" I appreciate it won't help ;-)
I think I'd rather see the structures fixed up to be correct and properly
type stable for 64bit on a 64bit box including u64 user pointers.
For 32bit then yes you probably have to do something icky like
struct binder_foo64 {
}
struct binder_foo_compat {
}
#if 32bit
#define binder_foo binder_foo_compat
#else
#define binder_foo binder_foo64
#endif
but I do think it would make the rest of the code look less like a lesson
on pointer and GNU extension obfuscation and when 32bit finally gets
buried the uglies can be removed.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH 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 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 >> >> wrote: >> >> >> >> >> >> > 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. > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: > On Wed, Dec 4, 2013 at 1:43 PM, Greg KH 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 > >> wrote: > >> > >> > >> > 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"? Or does this patch series mean that no userspace code is changed? Is that a "requirement" here? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 1:43 PM, Greg KH 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 wrote: >> >> >> > 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. 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. I would suggest fixing the 32-bit structures to use fixed sizes where appropriate (__u32 instead of unsigned long) while maintaining compatibility, and then using compat_ioctl where necessary to handle pointers. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: > On Wed, Dec 4, 2013 at 10:35 AM, Greg KH wrote: > > > > 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? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 10:35 AM, Greg KH wrote: > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
> +#define size_helper(x) ({\
> + size_t __size; \
> + if (!is_compat_task()) \
> + __size = sizeof(x); \
> + else if (sizeof(x) == sizeof(struct flat_binder_object))\
> + __size = sizeof(struct compat_flat_binder_object); \
> + else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
> + __size = sizeof(struct compat_binder_transaction_data); \
> + else if (sizeof(x) == sizeof(size_t)) \
> + __size = sizeof(compat_size_t); \
> + else\
> + BUG(); \
> + __size; \
> + })
Ick.
First off, no driver should ever be able to crash the kernel, which you
just did.
Second, almost none of those "if" lines will ever be hit, why did you
include it all?
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?
You have the chance to fix the api properly, why not take it and do it,
making all of this unnecessary.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
