linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] staging: android: ashmem: add 32bit compat support
@ 2013-02-01 16:07 Serban Constantinescu
  2013-02-01 16:08 ` [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface Serban Constantinescu
  2013-02-01 16:08 ` [PATCH 2/2] staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel Serban Constantinescu
  0 siblings, 2 replies; 10+ messages in thread
From: Serban Constantinescu @ 2013-02-01 16:07 UTC (permalink / raw)
  To: linux-kernel, gregkh, kernel-team, arve, john.stultz, Dave.Butcher
  Cc: Serban Constantinescu

Hi all,

This set of patches will add support for existing 32bit Android ashmem
syscalls on 64bit platforms. They also fix the ashmem pin/unpin interface
without affecting the 32bit ABI but imposing a new, correct one, for any
future 64bit Android userspace.

They have been successfully tested on 32bit platforms(VExpress 4xA9) and
64bit platforms(RTSMv8) running 32bit Android userspace and 64bit Linux +
ashmem unit tests.

Best Regards,
Serban Constantinescu
PDSW Engineer ARM Ltd.


Serban Constantinescu (2):
  staging: android: ashmem: fix ashmem pin/unpin interface
  staging: android: ashmem: Add support for 32bit ashmem calls in a
    64bit kernel

 drivers/staging/android/ashmem.c |   65 ++++++++++++++++++++++++++++++--------
 drivers/staging/android/ashmem.h |   19 +++++++++--
 2 files changed, 68 insertions(+), 16 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface
  2013-02-01 16:07 [PATCH 0/2] staging: android: ashmem: add 32bit compat support Serban Constantinescu
@ 2013-02-01 16:08 ` Serban Constantinescu
  2013-02-01 16:18   ` Greg KH
  2013-02-01 16:08 ` [PATCH 2/2] staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel Serban Constantinescu
  1 sibling, 1 reply; 10+ messages in thread
From: Serban Constantinescu @ 2013-02-01 16:08 UTC (permalink / raw)
  To: linux-kernel, gregkh, kernel-team, arve, john.stultz, Dave.Butcher
  Cc: Serban Constantinescu

The values exchanged between kernel and userspace through struct
ashmem_pin should be of type size_t. This change won't affect the
existing interface but will stand as the basis of 64bit compat layer.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/ashmem.c |    2 +-
 drivers/staging/android/ashmem.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 72064fc..aa52646 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -596,7 +596,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 	if (unlikely((pin.offset | pin.len) & ~PAGE_MASK))
 		return -EINVAL;
 
-	if (unlikely(((__u32) -1) - pin.offset < pin.len))
+	if (unlikely(((size_t) -1) - pin.offset < pin.len))
 		return -EINVAL;
 
 	if (unlikely(PAGE_ALIGN(asma->size) < pin.offset + pin.len))
diff --git a/drivers/staging/android/ashmem.h b/drivers/staging/android/ashmem.h
index 1976b10..c9b2eba 100644
--- a/drivers/staging/android/ashmem.h
+++ b/drivers/staging/android/ashmem.h
@@ -28,8 +28,8 @@
 #define ASHMEM_IS_PINNED	1
 
 struct ashmem_pin {
-	__u32 offset;	/* offset into region, in bytes, page-aligned */
-	__u32 len;	/* length forward from offset, in bytes, page-aligned */
+	size_t offset;	/* offset into region, in bytes, page-aligned */
+	size_t len;	/* length forward from offset, in bytes, page-aligned */
 };
 
 #define __ASHMEMIOC		0x77
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel
  2013-02-01 16:07 [PATCH 0/2] staging: android: ashmem: add 32bit compat support Serban Constantinescu
  2013-02-01 16:08 ` [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface Serban Constantinescu
@ 2013-02-01 16:08 ` Serban Constantinescu
  1 sibling, 0 replies; 10+ messages in thread
From: Serban Constantinescu @ 2013-02-01 16:08 UTC (permalink / raw)
  To: linux-kernel, gregkh, kernel-team, arve, john.stultz, Dave.Butcher
  Cc: Serban Constantinescu

Android's shared memory subsystem, Ashmem, does not support calls from a
32bit userspace in a 64 bit kernel. This patch adds support for syscalls
coming from a 32bit userspace in a 64bit kernel.

The patch has been successfully tested on ARMv8 AEM(64bit
platform model) and Versatile Express A9(32bit platform).

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/ashmem.c |   65 ++++++++++++++++++++++++++++++--------
 drivers/staging/android/ashmem.h |   15 +++++++++
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index aa52646..75b08a9 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -577,33 +577,29 @@ static int ashmem_get_pin_status(struct ashmem_area *asma, size_t pgstart,
 }
 
 static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
-			    void __user *p)
+			    struct ashmem_pin *pin)
 {
-	struct ashmem_pin pin;
 	size_t pgstart, pgend;
 	int ret = -EINVAL;
 
 	if (unlikely(!asma->file))
 		return -EINVAL;
 
-	if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
-		return -EFAULT;
-
 	/* per custom, you can pass zero for len to mean "everything onward" */
-	if (!pin.len)
-		pin.len = PAGE_ALIGN(asma->size) - pin.offset;
+	if (!pin->len)
+		pin->len = PAGE_ALIGN(asma->size) - pin->offset;
 
-	if (unlikely((pin.offset | pin.len) & ~PAGE_MASK))
+	if (unlikely((pin->offset | pin->len) & ~PAGE_MASK))
 		return -EINVAL;
 
-	if (unlikely(((size_t) -1) - pin.offset < pin.len))
+	if (unlikely(((size_t) -1) - pin->offset < pin->len))
 		return -EINVAL;
 
-	if (unlikely(PAGE_ALIGN(asma->size) < pin.offset + pin.len))
+	if (unlikely(PAGE_ALIGN(asma->size) < pin->offset + pin->len))
 		return -EINVAL;
 
-	pgstart = pin.offset / PAGE_SIZE;
-	pgend = pgstart + (pin.len / PAGE_SIZE) - 1;
+	pgstart = pin->offset / PAGE_SIZE;
+	pgend = pgstart + (pin->len / PAGE_SIZE) - 1;
 
 	mutex_lock(&ashmem_mutex);
 
@@ -627,6 +623,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct ashmem_area *asma = file->private_data;
+	struct ashmem_pin pin;
 	long ret = -ENOTTY;
 
 	switch (cmd) {
@@ -655,7 +652,9 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case ASHMEM_PIN:
 	case ASHMEM_UNPIN:
 	case ASHMEM_GET_PIN_STATUS:
-		ret = ashmem_pin_unpin(asma, cmd, (void __user *) arg);
+		if (unlikely(copy_from_user(&pin, (void __user *)arg, sizeof(pin))))
+			return -EFAULT;
+		ret = ashmem_pin_unpin(asma, cmd, &pin);
 		break;
 	case ASHMEM_PURGE_ALL_CACHES:
 		ret = -EPERM;
@@ -674,6 +673,42 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return ret;
 }
 
+/* support of 32bit userspace on 64bit platforms */
+#ifdef CONFIG_COMPAT
+static long compat_ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct compat_ashmem_pin c_pin;
+	struct ashmem_pin pin;
+
+	switch (cmd) {
+	case COMPAT_ASHMEM_SET_SIZE:
+		cmd = ASHMEM_SET_SIZE;
+		break;
+	case COMPAT_ASHMEM_SET_PROT_MASK:
+		cmd = ASHMEM_SET_PROT_MASK;
+		break;
+	case COMPAT_ASHMEM_PIN:
+	case COMPAT_ASHMEM_UNPIN:
+	case ASHMEM_GET_PIN_STATUS:
+		if (unlikely(copy_from_user(&c_pin, (void __user *)arg, sizeof(c_pin))))
+			return -EFAULT;
+		pin.offset = (size_t)c_pin.offset;
+		pin.len = (size_t)c_pin.len;
+		switch (cmd) {
+		case COMPAT_ASHMEM_PIN:
+			cmd = ASHMEM_PIN;
+			break;
+		case COMPAT_ASHMEM_UNPIN:
+			cmd = ASHMEM_UNPIN;
+			break;
+		}
+		return ashmem_pin_unpin(file->private_data, cmd, &pin);
+	}
+
+	return ashmem_ioctl(file, cmd, arg);
+}
+#endif /* CONFIG_COMPAT */
+
 static const struct file_operations ashmem_fops = {
 	.owner = THIS_MODULE,
 	.open = ashmem_open,
@@ -682,7 +717,9 @@ static const struct file_operations ashmem_fops = {
 	.llseek = ashmem_llseek,
 	.mmap = ashmem_mmap,
 	.unlocked_ioctl = ashmem_ioctl,
-	.compat_ioctl = ashmem_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_ashmem_ioctl,
+#endif
 };
 
 static struct miscdevice ashmem_misc = {
diff --git a/drivers/staging/android/ashmem.h b/drivers/staging/android/ashmem.h
index c9b2eba..ff7328f 100644
--- a/drivers/staging/android/ashmem.h
+++ b/drivers/staging/android/ashmem.h
@@ -32,6 +32,13 @@ struct ashmem_pin {
 	size_t len;	/* length forward from offset, in bytes, page-aligned */
 };
 
+#ifdef CONFIG_COMPAT
+struct compat_ashmem_pin {
+	compat_size_t offset;	/* offset into region, in bytes, page-aligned */
+	compat_size_t len;	/* length forward from offset, in bytes, page-aligned */
+};
+#endif
+
 #define __ASHMEMIOC		0x77
 
 #define ASHMEM_SET_NAME		_IOW(__ASHMEMIOC, 1, char[ASHMEM_NAME_LEN])
@@ -45,4 +52,12 @@ struct ashmem_pin {
 #define ASHMEM_GET_PIN_STATUS	_IO(__ASHMEMIOC, 9)
 #define ASHMEM_PURGE_ALL_CACHES	_IO(__ASHMEMIOC, 10)
 
+/* support of 32bit userspace on 64bit platforms */
+#ifdef CONFIG_COMPAT
+#define COMPAT_ASHMEM_SET_SIZE		_IOW(__ASHMEMIOC, 3, compat_size_t)
+#define COMPAT_ASHMEM_SET_PROT_MASK	_IOW(__ASHMEMIOC, 5, unsigned int)
+#define COMPAT_ASHMEM_PIN		_IOW(__ASHMEMIOC, 7, struct compat_ashmem_pin)
+#define COMPAT_ASHMEM_UNPIN		_IOW(__ASHMEMIOC, 8, struct compat_ashmem_pin)
+#endif
+
 #endif	/* _LINUX_ASHMEM_H */
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface
  2013-02-01 16:08 ` [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface Serban Constantinescu
@ 2013-02-01 16:18   ` Greg KH
  2013-02-01 16:55     ` Serban Constantinescu
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2013-02-01 16:18 UTC (permalink / raw)
  To: Serban Constantinescu
  Cc: linux-kernel, kernel-team, arve, john.stultz, Dave.Butcher

On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
> The values exchanged between kernel and userspace through struct
> ashmem_pin should be of type size_t. This change won't affect the
> existing interface but will stand as the basis of 64bit compat layer.

How do you define size_t with a 64bit kernel and a 32bit userspace
properly?  Doesn't this change open up a bunch of problems?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface
  2013-02-01 16:18   ` Greg KH
@ 2013-02-01 16:55     ` Serban Constantinescu
  2013-02-04  1:41       ` Greg KH
  0 siblings, 1 reply; 10+ messages in thread
From: Serban Constantinescu @ 2013-02-01 16:55 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-kernel, kernel-team, arve, john.stultz, Dave Butcher

Hi Greg,

On 01/02/13 16:18, Greg KH wrote:
> On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
>> The values exchanged between kernel and userspace through struct
>> ashmem_pin should be of type size_t. This change won't affect the
>> existing interface but will stand as the basis of 64bit compat layer.
>
> How do you define size_t with a 64bit kernel and a 32bit userspace
> properly?  Doesn't this change open up a bunch of problems?

The current ashmem pin/unpin kernel interface uses __u32 to specify the 
memory region and length in bytes. However these values  should be of 
type size_t so that they are able to represent the whole range of 
possible values when compiled for a 64bit platform.

Android API uses ashmem driver through libcutils, from where I attach 
the following snippet:

<aosp>/system/core/libcutils/ashmem-dev.c

   75 int ashmem_pin_region(int fd, size_t offset, size_t len)
   76 {
   77         struct ashmem_pin pin = { offset, len };
   78         return ioctl(fd, ASHMEM_PIN, &pin);
   79 }

The kernel changes inline with the userspace usage and do not affect 
existing 32bit Android (we have exported the new kernel header, rebuilt 
and tested the interface with success).

However this change will affect any 64bit userspace using the current 
faulty interface, but there is none that we know of.

Thanks,
Serban Constantinescu
`




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface
  2013-02-01 16:55     ` Serban Constantinescu
@ 2013-02-04  1:41       ` Greg KH
  2013-02-04 10:54         ` Serban Constantinescu
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2013-02-04  1:41 UTC (permalink / raw)
  To: Serban Constantinescu
  Cc: linux-kernel, kernel-team, arve, john.stultz, Dave Butcher

On Fri, Feb 01, 2013 at 04:55:01PM +0000, Serban Constantinescu wrote:
> Hi Greg,
> 
> On 01/02/13 16:18, Greg KH wrote:
> >On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
> >>The values exchanged between kernel and userspace through struct
> >>ashmem_pin should be of type size_t. This change won't affect the
> >>existing interface but will stand as the basis of 64bit compat layer.
> >
> >How do you define size_t with a 64bit kernel and a 32bit userspace
> >properly?  Doesn't this change open up a bunch of problems?
> 
> The current ashmem pin/unpin kernel interface uses __u32 to specify
> the memory region and length in bytes. However these values  should
> be of type size_t so that they are able to represent the whole range
> of possible values when compiled for a 64bit platform.

Yes, the issue is, what size is size_t on the system if you have a 32bit
userspace and a 64bit kernel?  :)

That's why we have specific types for when we cross the user/kernel
boundry.  Why not use them instead here so that you know it will work
properly in the future?

> Android API uses ashmem driver through libcutils, from where I
> attach the following snippet:
> 
> <aosp>/system/core/libcutils/ashmem-dev.c
> 
>   75 int ashmem_pin_region(int fd, size_t offset, size_t len)
>   76 {
>   77         struct ashmem_pin pin = { offset, len };
>   78         return ioctl(fd, ASHMEM_PIN, &pin);
>   79 }

Again, the 32/64 bit issue is to blame.

> The kernel changes inline with the userspace usage and do not affect
> existing 32bit Android (we have exported the new kernel header,
> rebuilt and tested the interface with success).
> 
> However this change will affect any 64bit userspace using the
> current faulty interface, but there is none that we know of.

I'd like the Android developers to give some feedback on this, before
I'll do anything.  I still think you need to change this to use the
proper kernel types.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface
  2013-02-04  1:41       ` Greg KH
@ 2013-02-04 10:54         ` Serban Constantinescu
  0 siblings, 0 replies; 10+ messages in thread
From: Serban Constantinescu @ 2013-02-04 10:54 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-kernel, kernel-team, arve, john.stultz, Dave Butcher

On 04/02/13 01:41, Greg KH wrote:
> On Fri, Feb 01, 2013 at 04:55:01PM +0000, Serban Constantinescu wrote:
>> Hi Greg,
>>
>> On 01/02/13 16:18, Greg KH wrote:
>>> On Fri, Feb 01, 2013 at 04:08:00PM +0000, Serban Constantinescu wrote:
>>>> The values exchanged between kernel and userspace through struct
>>>> ashmem_pin should be of type size_t. This change won't affect the
>>>> existing interface but will stand as the basis of 64bit compat layer.
>>>
>>> How do you define size_t with a 64bit kernel and a 32bit userspace
>>> properly?  Doesn't this change open up a bunch of problems?
>>
>> The current ashmem pin/unpin kernel interface uses __u32 to specify
>> the memory region and length in bytes. However these values  should
>> be of type size_t so that they are able to represent the whole range
>> of possible values when compiled for a 64bit platform.
>
> Yes, the issue is, what size is size_t on the system if you have a 32bit
> userspace and a 64bit kernel?  :)
>
> That's why we have specific types for when we cross the user/kernel
> boundry.  Why not use them instead here so that you know it will work
> properly in the future?

The patch set was developed using size_t to avoid the use of a #ifdef
statement that would split the use for 64bit platforms using u64 and
32bit using u32. On a 64/32 system you will have a 32bit size_t in the
user space and a 64bit size_t in the kernel. However in the kernel entry 
- compat_ashmem_ioctl we explicitly cast from compat_size_t (32bit - 
same as the userspace) to size_t (64bit used by the kernel).

 > 695                 pin.offset = (size_t)c_pin.offset;
 > 696                 pin.len = (size_t)c_pin.len;

Same logic was applied for existing size_t ioctls:

 > 46 #define ASHMEM_SET_SIZE         _IOW(__ASHMEMIOC, 3, size_t)

where on a 64/32 system you are using:

 > 57 #define COMPAT_ASHMEM_SET_SIZE          _IOW(__ASHMEMIOC, 3, 
compat_size_t)

This kernel header is not intended to be exported for current Android
userspace (even though we have tested this with success on one of the
latest Android revisions).

>> Android API uses ashmem driver through libcutils, from where I
>> attach the following snippet:
>>
>> <aosp>/system/core/libcutils/ashmem-dev.c
>>
>>    75 int ashmem_pin_region(int fd, size_t offset, size_t len)
>>    76 {
>>    77         struct ashmem_pin pin = { offset, len };
>>    78         return ioctl(fd, ASHMEM_PIN, &pin);
>>    79 }
>
> Again, the 32/64 bit issue is to blame.
>
>> The kernel changes inline with the userspace usage and do not affect
>> existing 32bit Android (we have exported the new kernel header,
>> rebuilt and tested the interface with success).
>>
>> However this change will affect any 64bit userspace using the
>> current faulty interface, but there is none that we know of.
>
> I'd like the Android developers to give some feedback on this, before
> I'll do anything.  I still think you need to change this to use the
> proper kernel types.

If you or the Android developers consider the use of #ifdef u64/u32 is 
better I will rework the patch accordingly.


Thanks,
Serban Constantinescu


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel
  2012-12-04 11:45   ` Dan Carpenter
@ 2012-12-05 14:25     ` Serban Constantinescu
  0 siblings, 0 replies; 10+ messages in thread
From: Serban Constantinescu @ 2012-12-05 14:25 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: gregkh, arve, devel, linux-kernel, john.stultz, ccross,
	zach.pfeffer, Dave Butcher, Catalin Marinas, gregkh, arve

On 04/12/12 11:45, Dan Carpenter wrote:
> I don't understand this, and I'm going to embarrass myself by
> displaying my ignorance for all to see.  Why is this code so
> different from all the other 32 bit compat code that we have in the
> kernel?
>
> On Tue, Dec 04, 2012 at 10:44:14AM +0000, Serban Constantinescu wrote:
>> -static int set_name(struct ashmem_area *asma, void __user *name)
>> +static int set_name(struct ashmem_area *asma, userptr32_t name)
>
> The user passes in a value which is a 32 pointer.  ashmem_ioctl()
> accepts it as "unsigned long arg".  We pass it to set_name() which
> truncates away the high zeros so now its a u32 (userptr32_t).  We
> then cast it to (unsigned long) and then we cast it to a void
> pointer.
>
> What's the point?  Why not just take"unsigned long arg" and cast it
> to a pointer directly?
>
>>      if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
>> -                                name, ASHMEM_NAME_LEN)))
>> +                                (void *)(unsigned long)name, ASHMEM_NAME_LEN)))
>>              ret = -EFAULT;
>
> This will introduce a Sparse complaint.  It should be:
>                               (void __user *)(unsigned long)name.

Thanks for taking your time and reviewing this patch set. I have put
together a new version of this patch (ashmem) and I will resend it to
LKML as soon as I finish testing on both 32 and 64 bit platforms.

>
> But actually we shouldn't need to do this casting.  Any casting
> which we need to do should be done in one place instead of pushed
> out to every function.
>
>> +    switch (_IOC_NR(cmd)) {
>> +    case _IOC_NR(ASHMEM_SET_NAME):
>> +            if (_IOC_SIZE(cmd) != sizeof(char[ASHMEM_NAME_LEN]))
>> +                    pr_err("ashmem: ASHMEM_SET_NAME transaction size differs\n");
>
> Don't merge debug code into the kernel.  It just means people can
> spam /var/log/messages.

I see what you mean. I won't include those in the new patch set.

>
> regards,
> dan carpenter
>
>

Kind regards,
Serban Constantinescu
`

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel
  2012-12-04 10:44 ` [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls " Serban Constantinescu
@ 2012-12-04 11:45   ` Dan Carpenter
  2012-12-05 14:25     ` Serban Constantinescu
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2012-12-04 11:45 UTC (permalink / raw)
  To: Serban Constantinescu
  Cc: gregkh, arve, devel, linux-kernel, john.stultz, ccross,
	zach.pfeffer, Dave.Butcher

I don't understand this, and I'm going to embarrass myself by
displaying my ignorance for all to see.  Why is this code so
different from all the other 32 bit compat code that we have in the
kernel?

On Tue, Dec 04, 2012 at 10:44:14AM +0000, Serban Constantinescu wrote:
> -static int set_name(struct ashmem_area *asma, void __user *name)
> +static int set_name(struct ashmem_area *asma, userptr32_t name)

The user passes in a value which is a 32 pointer.  ashmem_ioctl()
accepts it as "unsigned long arg".  We pass it to set_name() which
truncates away the high zeros so now its a u32 (userptr32_t).  We
then cast it to (unsigned long) and then we cast it to a void
pointer.

What's the point?  Why not just take"unsigned long arg" and cast it
to a pointer directly?

>  	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
> -				    name, ASHMEM_NAME_LEN)))
> +				    (void *)(unsigned long)name, ASHMEM_NAME_LEN)))
>  		ret = -EFAULT;

This will introduce a Sparse complaint.  It should be:
				(void __user *)(unsigned long)name.

But actually we shouldn't need to do this casting.  Any casting
which we need to do should be done in one place instead of pushed
out to every function.

> +	switch (_IOC_NR(cmd)) {
> +	case _IOC_NR(ASHMEM_SET_NAME):
> +		if (_IOC_SIZE(cmd) != sizeof(char[ASHMEM_NAME_LEN]))
> +			pr_err("ashmem: ASHMEM_SET_NAME transaction size differs\n");

Don't merge debug code into the kernel.  It just means people can
spam /var/log/messages.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel
  2012-12-04 10:44 [PATCH 0/2] Android: Add support for a 32bit Android file system " Serban Constantinescu
@ 2012-12-04 10:44 ` Serban Constantinescu
  2012-12-04 11:45   ` Dan Carpenter
  0 siblings, 1 reply; 10+ messages in thread
From: Serban Constantinescu @ 2012-12-04 10:44 UTC (permalink / raw)
  To: gregkh, arve, devel, linux-kernel, john.stultz, ccross,
	zach.pfeffer, Dave.Butcher
  Cc: Serban Constantinescu

Android's shared memory subsystem, Ashmem, does not support calls from a
32-bit userspace in a 64 bit kernel. This patch adds support for syscalls
coming from a 32-bit userspace in a 64-bit kernel.

Most of the changes were applied to types that change sizes between
32 and 64 bit world. This will also fix some of the issues around
checking the size of an incoming transaction package in the ioctl
switch. Since  the transaction's ioctl number are generated using
_IOC(dir,type,nr,size), a different userspace size will generate
a different ioctl number, thus switching by _IOC_NR is a better
solution.

The patch has been successfully tested on ARMv8 AEM and Versatile
Express V2P-CA9.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 drivers/staging/android/ashmem.c |   60 +++++++++++++++++++++++++-------------
 drivers/staging/android/ashmem.h |    6 ++--
 2 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 634b9ae..5e3e687 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -411,7 +411,7 @@ out:
 	return ret;
 }
 
-static int set_name(struct ashmem_area *asma, void __user *name)
+static int set_name(struct ashmem_area *asma, userptr32_t name)
 {
 	int ret = 0;
 
@@ -424,7 +424,7 @@ static int set_name(struct ashmem_area *asma, void __user *name)
 	}
 
 	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
-				    name, ASHMEM_NAME_LEN)))
+				    (void *)(unsigned long)name, ASHMEM_NAME_LEN)))
 		ret = -EFAULT;
 	asma->name[ASHMEM_FULL_NAME_LEN-1] = '\0';
 
@@ -434,7 +434,7 @@ out:
 	return ret;
 }
 
-static int get_name(struct ashmem_area *asma, void __user *name)
+static int get_name(struct ashmem_area *asma, userptr32_t name)
 {
 	int ret = 0;
 
@@ -447,11 +447,11 @@ static int get_name(struct ashmem_area *asma, void __user *name)
 		 * prevents us from revealing one user's stack to another.
 		 */
 		len = strlen(asma->name + ASHMEM_NAME_PREFIX_LEN) + 1;
-		if (unlikely(copy_to_user(name,
+		if (unlikely(copy_to_user((void *)(unsigned long)name,
 				asma->name + ASHMEM_NAME_PREFIX_LEN, len)))
 			ret = -EFAULT;
 	} else {
-		if (unlikely(copy_to_user(name, ASHMEM_NAME_DEF,
+		if (unlikely(copy_to_user((void *)(unsigned long)name, ASHMEM_NAME_DEF,
 					  sizeof(ASHMEM_NAME_DEF))))
 			ret = -EFAULT;
 	}
@@ -586,7 +586,7 @@ static int ashmem_get_pin_status(struct ashmem_area *asma, size_t pgstart,
 }
 
 static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
-			    void __user *p)
+			    userptr32_t p)
 {
 	struct ashmem_pin pin;
 	size_t pgstart, pgend;
@@ -595,7 +595,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 	if (unlikely(!asma->file))
 		return -EINVAL;
 
-	if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
+	if (unlikely(copy_from_user(&pin, (void *)(unsigned long)p, sizeof(pin))))
 		return -EFAULT;
 
 	/* per custom, you can pass zero for len to mean "everything onward" */
@@ -638,35 +638,50 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct ashmem_area *asma = file->private_data;
 	long ret = -ENOTTY;
 
-	switch (cmd) {
-	case ASHMEM_SET_NAME:
-		ret = set_name(asma, (void __user *) arg);
+	/*
+	 * since  the transaction's IOCTL number are generated using
+	 * _IOC(dir,type,nr,size), a different userspace size will not
+	 * fall through
+	 */
+	switch (_IOC_NR(cmd)) {
+	case _IOC_NR(ASHMEM_SET_NAME):
+		if (_IOC_SIZE(cmd) != sizeof(char[ASHMEM_NAME_LEN]))
+			pr_err("ashmem: ASHMEM_SET_NAME transaction size differs\n");
+		ret = set_name(asma, arg);
 		break;
-	case ASHMEM_GET_NAME:
-		ret = get_name(asma, (void __user *) arg);
+	case _IOC_NR(ASHMEM_GET_NAME):
+		if (_IOC_SIZE(cmd) != sizeof(char[ASHMEM_NAME_LEN]))
+			pr_err("ashmem: ASHMEM_GET_NAME transaction size differs\n");
+		ret = get_name(asma, arg);
 		break;
-	case ASHMEM_SET_SIZE:
+	case _IOC_NR(ASHMEM_SET_SIZE):
+		if (_IOC_SIZE(cmd) != sizeof(uint32_t))
+			pr_err("ashmem: ASHMEM_SET_SIZE transaction size differs\n");
 		ret = -EINVAL;
 		if (!asma->file) {
 			ret = 0;
 			asma->size = (size_t) arg;
 		}
 		break;
-	case ASHMEM_GET_SIZE:
+	case _IOC_NR(ASHMEM_GET_SIZE):
 		ret = asma->size;
 		break;
-	case ASHMEM_SET_PROT_MASK:
+	case _IOC_NR(ASHMEM_SET_PROT_MASK):
+		if (_IOC_SIZE(cmd) != sizeof(uint32_t))
+			pr_err("ashmem: ASHMEM_SET_PROT_MASK transaction size differs\n");
 		ret = set_prot_mask(asma, arg);
 		break;
-	case ASHMEM_GET_PROT_MASK:
+	case _IOC_NR(ASHMEM_GET_PROT_MASK):
 		ret = asma->prot_mask;
 		break;
-	case ASHMEM_PIN:
-	case ASHMEM_UNPIN:
-	case ASHMEM_GET_PIN_STATUS:
-		ret = ashmem_pin_unpin(asma, cmd, (void __user *) arg);
+	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");
+		ret = ashmem_pin_unpin(asma, cmd, arg);
 		break;
-	case ASHMEM_PURGE_ALL_CACHES:
+	case _IOC_NR(ASHMEM_PURGE_ALL_CACHES):
 		ret = -EPERM;
 		if (capable(CAP_SYS_ADMIN)) {
 			struct shrink_control sc = {
@@ -678,6 +693,9 @@ static long ashmem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			ashmem_shrink(&ashmem_shrinker, &sc);
 		}
 		break;
+	default:
+		pr_err("ashmem: IOCTL No. not found\n");
+		break;
 	}
 
 	return ret;
diff --git a/drivers/staging/android/ashmem.h b/drivers/staging/android/ashmem.h
index 1976b10..4ecdc73 100644
--- a/drivers/staging/android/ashmem.h
+++ b/drivers/staging/android/ashmem.h
@@ -27,6 +27,8 @@
 #define ASHMEM_IS_UNPINNED	0
 #define ASHMEM_IS_PINNED	1
 
+typedef uint32_t userptr32_t;
+
 struct ashmem_pin {
 	__u32 offset;	/* offset into region, in bytes, page-aligned */
 	__u32 len;	/* length forward from offset, in bytes, page-aligned */
@@ -36,9 +38,9 @@ struct ashmem_pin {
 
 #define ASHMEM_SET_NAME		_IOW(__ASHMEMIOC, 1, char[ASHMEM_NAME_LEN])
 #define ASHMEM_GET_NAME		_IOR(__ASHMEMIOC, 2, char[ASHMEM_NAME_LEN])
-#define ASHMEM_SET_SIZE		_IOW(__ASHMEMIOC, 3, size_t)
+#define ASHMEM_SET_SIZE		_IOW(__ASHMEMIOC, 3, unsigned int)
 #define ASHMEM_GET_SIZE		_IO(__ASHMEMIOC, 4)
-#define ASHMEM_SET_PROT_MASK	_IOW(__ASHMEMIOC, 5, unsigned long)
+#define ASHMEM_SET_PROT_MASK	_IOW(__ASHMEMIOC, 5, unsigned int)
 #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)
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-02-04 10:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-01 16:07 [PATCH 0/2] staging: android: ashmem: add 32bit compat support Serban Constantinescu
2013-02-01 16:08 ` [PATCH 1/2] staging: android: ashmem: fix ashmem pin/unpin interface Serban Constantinescu
2013-02-01 16:18   ` Greg KH
2013-02-01 16:55     ` Serban Constantinescu
2013-02-04  1:41       ` Greg KH
2013-02-04 10:54         ` Serban Constantinescu
2013-02-01 16:08 ` [PATCH 2/2] staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel Serban Constantinescu
  -- strict thread matches above, loose matches on Subject: below --
2012-12-04 10:44 [PATCH 0/2] Android: Add support for a 32bit Android file system " Serban Constantinescu
2012-12-04 10:44 ` [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls " Serban Constantinescu
2012-12-04 11:45   ` Dan Carpenter
2012-12-05 14:25     ` Serban Constantinescu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).