The kernel uses (drivers/staging/android/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
Generated bionic header uses (bionic/libc/kernel/common/linux/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
But frameworks/native/include/utils/ashmem.h uses:
#define ASHMEM_PIN _IO(__ASHMEMIOC, 7) #define ASHMEM_UNPIN _IO(__ASHMEMIOC, 8)
Due to the above, any userspace call ioctl(fd, ASHMEM_PIN, XXX) is invalid.
Also there is a name mismatch: kernel uses ASHMEM_GET_PIN_STATUS for ioctl and ASHMEM_IS_PINNED / ASHMEM_IS_UNPINNED for the result, but userspace uses ASHMEM_ISPINNED for ioctl and ASHMEM_NOW_PINNED / ASHMEM_NOW_UNPINNED for the result. This is pretty confusing and IMHO should be unified between kernel and userspace headers.
Suggested fix for frameworks/native/include/utils/ashmem.h is attached.
Dmitry
On 12/06/2012 01:41 AM, Dmitry Antipov wrote:
The kernel uses (drivers/staging/android/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
Generated bionic header uses (bionic/libc/kernel/common/linux/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
But frameworks/native/include/utils/ashmem.h uses:
#define ASHMEM_PIN _IO(__ASHMEMIOC, 7) #define ASHMEM_UNPIN _IO(__ASHMEMIOC, 8)
Due to the above, any userspace call ioctl(fd, ASHMEM_PIN, XXX) is invalid.
Also there is a name mismatch: kernel uses ASHMEM_GET_PIN_STATUS for ioctl and ASHMEM_IS_PINNED / ASHMEM_IS_UNPINNED for the result, but userspace uses ASHMEM_ISPINNED for ioctl and ASHMEM_NOW_PINNED / ASHMEM_NOW_UNPINNED for the result. This is pretty confusing and IMHO should be unified between kernel and userspace headers.
Suggested fix for frameworks/native/include/utils/ashmem.h is attached.
This looks right to me. Seems like that ashmem.h is just really old. You should submit this to AOSP.
One suggestion: For your own test case, you might want to copy in the kernel's (or bionic's) ashmem.h into the test, rather then including the frameworks/native..../ashmem.h header, as this will allow the test case to build with both standard GNU as well as Android environments.
thanks -john
On 12/08/2012 04:32 AM, John Stultz wrote:
This looks right to me. Seems like that ashmem.h is just really old. You should submit this to AOSP.
One suggestion: For your own test case, you might want to copy in the kernel's (or bionic's) ashmem.h into the test, rather then including the frameworks/native..../ashmem.h header, as this will allow the test case to build with both standard GNU as well as Android environments.
Thanks, I'll do #ifdef ANDROID for that.
BTW, why it's possible to mmap() more than ASHMEM_SET_SIZE? Basically it is:
open("/dev/ashmem", O_RDWR) = 3 ioctl(3, 0x40047703, 0x4000) = 0 // ASHMEM_SET_SIZE for 4 pages - OK mmap2(NULL, 20480, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = 0xb6eff000 // next mmap() 5 pages - OK???
Dmitry
On 12/10/2012 08:10 AM, Dmitry Antipov wrote:
On 12/08/2012 04:32 AM, John Stultz wrote:
This looks right to me. Seems like that ashmem.h is just really old. You should submit this to AOSP.
One suggestion: For your own test case, you might want to copy in the kernel's (or bionic's) ashmem.h into the test, rather then including the frameworks/native..../ashmem.h header, as this will allow the test case to build with both standard GNU as well as Android environments.
Thanks, I'll do #ifdef ANDROID for that.
BTW, why it's possible to mmap() more than ASHMEM_SET_SIZE? Basically it is:
open("/dev/ashmem", O_RDWR) = 3 ioctl(3, 0x40047703, 0x4000) = 0 // ASHMEM_SET_SIZE for 4 pages - OK mmap2(NULL, 20480, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = 0xb6eff000 // next mmap() 5 pages - OK???
So looking at the code, the mmap handler doesn't look at the size of the vma passed to it. But It will generate the specified _SET_SIZE sized shmem file as the file backing for the vma.
So I think if you were to access the 5th page there I suspect you'd get a SIGBUS.
And if I recall, this mirrors what happens if you mmap() 5 pages of a normal file that is really 4 pages (as the file may be extended later while mmapped via a write or ftruncate).
So I suspect this behavior is expected.
thanks -john
On 12/08/2012 04:32 AM, John Stultz wrote:
Suggested fix for frameworks/native/include/utils/ashmem.h is attached.
This looks right to me. Seems like that ashmem.h is just really old. You should submit this to AOSP.
Is it possible without downloading official Android repositories? I tried to follow http://source.android.com/source/submit-patches.html, but './repo upload' makes an attempt to send the change to repository's origin (e.g. review.android.git.linaro.org) instead of android-review.googlesource.com :-(.
Dmitry
On 12/06/2012 01:41 AM, Dmitry Antipov wrote:
The kernel uses (drivers/staging/android/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
Generated bionic header uses (bionic/libc/kernel/common/linux/ashmem.h):
#define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin)
But frameworks/native/include/utils/ashmem.h uses:
#define ASHMEM_PIN _IO(__ASHMEMIOC, 7) #define ASHMEM_UNPIN _IO(__ASHMEMIOC, 8)
Due to the above, any userspace call ioctl(fd, ASHMEM_PIN, XXX) is invalid.
Also there is a name mismatch: kernel uses ASHMEM_GET_PIN_STATUS for ioctl and ASHMEM_IS_PINNED / ASHMEM_IS_UNPINNED for the result, but userspace uses ASHMEM_ISPINNED for ioctl and ASHMEM_NOW_PINNED / ASHMEM_NOW_UNPINNED for the result. This is pretty confusing and IMHO should be unified between kernel and userspace headers.
Hey Dmitry, I was playing with your current ashmem unit test and needed the following trivial patch to get it all (seemingly) working against Linus' head kernel w/ ubuntu userland.
thanks -john
diff --git a/ashmem.h b/ashmem.h index 519fcfb..5a14837 100644 --- a/ashmem.h +++ b/ashmem.h @@ -39,7 +39,7 @@ #define ASHMEM_GET_PROT_MASK _IO(__ASHMEMIOC, 6) #define ASHMEM_PIN _IOW(__ASHMEMIOC, 7, struct ashmem_pin) #define ASHMEM_UNPIN _IOW(__ASHMEMIOC, 8, struct ashmem_pin) -#define ASHMEM_ISPINNED _IOW(__ASHMEMIOC, 9, struct ashmem_pin) +#define ASHMEM_ISPINNED _IO(__ASHMEMIOC, 9) #define ASHMEM_PURGE_ALL_CACHES _IO(__ASHMEMIOC, 10)
#endif /* _UTILS_ASHMEM_H */
On 12/19/2012 04:55 AM, John Stultz wrote:
I was playing with your current ashmem unit test and needed the following trivial patch to get it all (seemingly) working against Linus' head kernel w/ ubuntu userland.
Ashmem unit test currently targets ashmem kernel driver from:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head...
which is a bit different from k.org version (we use armdroid version for juice-aosp because 64-bit developement goes there). Note that armdroid version has bug: ashmem.c expects that _IOC_SIZE (ioctl_number) == sizeof (struct ashmem_pin) for ASHMEM_GET_PIN_STATUS, but ashmem.h encodes ASHMEM_GET_PIN_STATUS with _IO(XXX), e.g. with zero _IOC_SIZE.
Since ASHMEM_GET_PIN_STATUS ioctl (a.k.a. ASHMEM_ISPINNED in user-space Android headers) uses struct ashmem_pin as 3rd arg, I believe that the correct definition for the user space is:
#define ASHMEM_ISPINNED _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
and for the kernel, respectively:
#define ASHMEM_GET_PIN_STATUS _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
This is the way I make it work in juice-aosp:
http://git.linaro.org/gitweb?p=people/antipov/linux-aarch64.git%3Ba=commit%3...
Dmitry
On Dec 19, 2012 2:13 AM, "Dmitry Antipov" dmitry.antipov@linaro.org wrote:
On 12/19/2012 04:55 AM, John Stultz wrote:
I was playing with your current ashmem unit test and needed the
following trivial
patch to get it all (seemingly) working against Linus' head kernel w/
ubuntu userland.
Ashmem unit test currently targets ashmem kernel driver from:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head...
which is a bit different from k.org version (we use armdroid version for
juice-aosp because
64-bit developement goes there). Note that armdroid version has bug:
ashmem.c expects that
_IOC_SIZE (ioctl_number) == sizeof (struct ashmem_pin) for
ASHMEM_GET_PIN_STATUS, but ashmem.h
encodes ASHMEM_GET_PIN_STATUS with _IO(XXX), e.g. with zero _IOC_SIZE.
Since ASHMEM_GET_PIN_STATUS ioctl (a.k.a. ASHMEM_ISPINNED in user-space
Android headers)
uses struct ashmem_pin as 3rd arg, I believe that the correct definition
for the user space is:
#define ASHMEM_ISPINNED _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
and for the kernel, respectively:
#define ASHMEM_GET_PIN_STATUS _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
This is the way I make it work in juice-aosp:
http://git.linaro.org/gitweb?p=people/antipov/linux-aarch64.git%3Ba=commit%3...
So, I'll double check aosp common.bit, but I think the Google kernel and k.org are consistent. Not sure why the arm kernel would be different.
Given the confusion we should check w android devs. I suspect the issue is no one really uses the is pinned/get status interfaces, so this inconsistency has gone unnoticed.
Thanks -john
On 12/19/2012 02:12 AM, Dmitry Antipov wrote:
On 12/19/2012 04:55 AM, John Stultz wrote:
I was playing with your current ashmem unit test and needed the following trivial patch to get it all (seemingly) working against Linus' head kernel w/ ubuntu userland.
Ashmem unit test currently targets ashmem kernel driver from:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head...
which is a bit different from k.org version (we use armdroid version for juice-aosp because 64-bit developement goes there). Note that armdroid version has bug: ashmem.c expects that _IOC_SIZE (ioctl_number) == sizeof (struct ashmem_pin) for ASHMEM_GET_PIN_STATUS, but ashmem.h encodes ASHMEM_GET_PIN_STATUS with _IO(XXX), e.g. with zero _IOC_SIZE.
Since ASHMEM_GET_PIN_STATUS ioctl (a.k.a. ASHMEM_ISPINNED in user-space Android headers) uses struct ashmem_pin as 3rd arg, I believe that the correct definition for the user space is:
#define ASHMEM_ISPINNED _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
and for the kernel, respectively:
#define ASHMEM_GET_PIN_STATUS _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
This is the way I make it work in juice-aosp:
http://git.linaro.org/gitweb?p=people/antipov/linux-aarch64.git%3Ba=commit%3...
So looking at this after having coffee, and while I think you're technically right, the problem is that your change introduce an ABI break with the existing kernel interface. That's a big no-no.
The _IOW() macro just generates a number. Its useful so that the commands are unique, given the size of the structure may be different on different arches. This allows the same constant to get different ioctl command numbers for 32bit and 64bit systems, if the structure size differs.
Luckily the ashmem_pin structure is explicit in size and constant across arches, so its "safe".
So fixing this would do nothing but break the existing abi.
A similar issue was recently reverted, so I don't think we want to propose such a change. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git%3Ba=commitd...
So I'd suggest instead to make your test consistent with the kernel, rather then the other way around.
As an aside, if we were unlucky and the structure being passed was not constant between arches, the proper solution would probably be to correct the ioctl cmd definitions as you have, but also to provide a compatability definition "_GET_PIN_STATUS_OLD" or that used the same broken definition. Then wire in the _OLD command in the ioctl assuming the existing users are 32bits. This way you fix properly it for future applications, and avoid breaking old existing applications. If binder has similar problems, this approach may be more useful.
thanks -john
On 12/20/2012 03:54 AM, John Stultz wrote:
So I'd suggest instead to make your test consistent with the kernel, rather then the other way around.
This makes no sense if the kernel itself is buggy :-).
Again, the root of evil is a bug in http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head... (k.org mainline has no such bug). I've tried to fix it, but this causes ABI break. So, if we can't change ASHMEM_GET_PIN_STATUS encoding, then this code is obviously wrong: ... case _IOC_NR(ASHMEM_PIN): case _IOC_NR(ASHMEM_UNPIN): case _IOC_NR(ASHMEM_GET_PIN_STATUS): if (_IOC_SIZE(cmd) != sizeof(struct ashmem_pin)) pr_err("ashmem: ASHMEM_PIN transaction size differs\n"); ... (because it assumes that sizeof(struct ashmem_pin) is encoded into ASHMEM_GET_PIN_STATUS).
Serban, the code above was introduced at:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=commit%3Bh=43632e1f149...
Dmitry
On Dec 20, 2012 5:36 AM, "Dmitry Antipov" dmitry.antipov@linaro.org wrote:
On 12/20/2012 03:54 AM, John Stultz wrote:
So I'd suggest instead to make your test consistent with the kernel,
rather then the other way around.
This makes no sense if the kernel itself is buggy :-).
Again, the root of evil is a bug in
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head...
(k.org mainline has no such bug). I've tried to fix it, but this causes
ABI break. So, if we can't change ASHMEM_GET_PIN_STATUS
encoding, then this code is obviously wrong: ... case _IOC_NR(ASHMEM_PIN): case _IOC_NR(ASHMEM_UNPIN): case _IOC_NR(ASHMEM_GET_PIN_STATUS): if (_IOC_SIZE(cmd) != sizeof(struct ashmem_pin)) pr_err("ashmem: ASHMEM_PIN transaction size differs\n"); ... (because it assumes that sizeof(struct ashmem_pin) is encoded into
ASHMEM_GET_PIN_STATUS).
Serban, the code above was introduced at:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=commit%3Bh=43632e1f149...
Yea, that kernel change will need to be dropped. I know Serban is reworking that patch and didn't see a similar issue in the latest version.
Regardless, kudos to your test case calling this issue out! Its doing exactly what we need.
Thanks -john
linaro-android@lists.linaro.org
-
Dmitry Antipov -
John Stultz