[PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[David Miller]

Ben, I'm pretty sure you're hitting this too on powerpc.  Every time a
32-bit process tries to upload cliprects it's going to fail with
-EFAULT or similar.

Nothing in userspace checks the return value for errors, etc. :-/

The only reason I caught this is because I have a debugging check on
sparc64 that makes sure that faults on kernel accesses to 32-bit
userspace never have any of the high 32-bits set.

--------------------

drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

Only X86 32-bit uses a different alignment for "unsigned long long"
than it's 64-bit counterpart.

Therefore this compat translation is only correct, and only needed,
when either CONFIG_X86 or CONFIG_IA64.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/gpu/drm/drm_ioc32.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
index 920b72f..282d9fd 100644
--- a/drivers/gpu/drm/drm_ioc32.c
+++ b/drivers/gpu/drm/drm_ioc32.c
@@ -954,6 +954,7 @@ static int compat_drm_sg_free(struct file *file, unsigned int cmd,
 			 DRM_IOCTL_SG_FREE, (unsigned long)request);
 }
 
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
 typedef struct drm_update_draw32 {
 	drm_drawable_t handle;
 	unsigned int type;
@@ -984,6 +985,7 @@ static int compat_drm_update_draw(struct file *file, unsigned int cmd,
 			DRM_IOCTL_UPDATE_DRAW, (unsigned long)request);
 	return err;
 }
+#endif
 
 struct drm_wait_vblank_request32 {
 	enum drm_vblank_seq_type type;
@@ -1066,7 +1068,9 @@ drm_ioctl_compat_t *drm_compat_ioctls[] = {
 #endif
 	[DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC32)] = compat_drm_sg_alloc,
 	[DRM_IOCTL_NR(DRM_IOCTL_SG_FREE32)] = compat_drm_sg_free,
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
 	[DRM_IOCTL_NR(DRM_IOCTL_UPDATE_DRAW32)] = compat_drm_update_draw,
+#endif
 	[DRM_IOCTL_NR(DRM_IOCTL_WAIT_VBLANK32)] = compat_drm_wait_vblank,
 };
 
-- 
1.6.1.2.350.g88cc

Re: [PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[Benjamin Herrenschmidt]

On Wed, 2009-02-18 at 01:35 -0800, David Miller wrote:
> Ben, I'm pretty sure you're hitting this too on powerpc.  Every time a
> 32-bit process tries to upload cliprects it's going to fail with
> -EFAULT or similar.

Heh, quite possibly

Could that be related to the kernel spewing a bunch of

[drm:drm_update_drawable_info] *ERROR* Failed to copy cliprects from userspace

?

I've seen that yesterday on the Bimini, was planning to dig today until
I got stuck home with a cold...

> Nothing in userspace checks the return value for errors, etc. :-/

Surprise !

> The only reason I caught this is because I have a debugging check on
> sparc64 that makes sure that faults on kernel accesses to 32-bit
> userspace never have any of the high 32-bits set.

That's a nice trick, I might do something similar on power..

Cheers,
Ben.

> --------------------
> 
> drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.
> 
> Only X86 32-bit uses a different alignment for "unsigned long long"
> than it's 64-bit counterpart.
> 
> Therefore this compat translation is only correct, and only needed,
> when either CONFIG_X86 or CONFIG_IA64.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  drivers/gpu/drm/drm_ioc32.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index 920b72f..282d9fd 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -954,6 +954,7 @@ static int compat_drm_sg_free(struct file *file, unsigned int cmd,
>  			 DRM_IOCTL_SG_FREE, (unsigned long)request);
>  }
>  
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>  typedef struct drm_update_draw32 {
>  	drm_drawable_t handle;
>  	unsigned int type;
> @@ -984,6 +985,7 @@ static int compat_drm_update_draw(struct file *file, unsigned int cmd,
>  			DRM_IOCTL_UPDATE_DRAW, (unsigned long)request);
>  	return err;
>  }
> +#endif
>  
>  struct drm_wait_vblank_request32 {
>  	enum drm_vblank_seq_type type;
> @@ -1066,7 +1068,9 @@ drm_ioctl_compat_t *drm_compat_ioctls[] = {
>  #endif
>  	[DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC32)] = compat_drm_sg_alloc,
>  	[DRM_IOCTL_NR(DRM_IOCTL_SG_FREE32)] = compat_drm_sg_free,
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>  	[DRM_IOCTL_NR(DRM_IOCTL_UPDATE_DRAW32)] = compat_drm_update_draw,
> +#endif
>  	[DRM_IOCTL_NR(DRM_IOCTL_WAIT_VBLANK32)] = compat_drm_wait_vblank,
>  };
>  

Re: [PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 19 Feb 2009 08:59:50 +1100

> Could that be related to the kernel spewing a bunch of
> 
> [drm:drm_update_drawable_info] *ERROR* Failed to copy cliprects from userspace

Yep, that is exactly caused by this bug.

Re: [PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[Arnd Bergmann]

On Wednesday 18 February 2009, David Miller wrote:
> drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.
> 
> Only X86 32-bit uses a different alignment for "unsigned long long"
> than it's 64-bit counterpart.
> 
> Therefore this compat translation is only correct, and only needed,
> when either CONFIG_X86 or CONFIG_IA64.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

The patch is correct AFAICT, but I'd like to point out that the
problem could have been avoided (besides using a non-padded layout)
by using a compat_u64 member in the struct definition instead of
the packed attribute:

typedef struct drm_update_draw32 {
	drm_drawable_t handle;
	unsigned int type;
	unsigned int num;
	compat_u64 data;	/**< Pointer */
} drm_update_draw32_t;

I find that too often __attribute__((packed)) is used on whole
structures where some other much more gentle solution can be
applied. In fact, there are very few files (e.g.
linux/unaligned/packed_struct.h) that look like they want all
of the implied meanings (pack members, drop alignment on whole
structure, access members as unaligned).

A grep for "packed" in compat_ioctl definitions revealed the
same bug as in drm_update_draw32 to be present in
raw32_config_request, and I'm rather sure that there are more
of these.

	Arnd <><

Re: [PATCH] drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, 19 Feb 2009 15:19:01 +0100

> On Wednesday 18 February 2009, David Miller wrote:
> > drm: Only use DRM_IOCTL_UPDATE_DRAW compat wrapper for compat X86.
> > 
> > Only X86 32-bit uses a different alignment for "unsigned long long"
> > than it's 64-bit counterpart.
> > 
> > Therefore this compat translation is only correct, and only needed,
> > when either CONFIG_X86 or CONFIG_IA64.
> > 
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> The patch is correct AFAICT, but I'd like to point out that the
> problem could have been avoided (besides using a non-padded layout)
> by using a compat_u64 member in the struct definition instead of
> the packed attribute:

Indeed, David A. showed me compat_u64 et al. and I'm fine with
it being fixed that way too.  Feel free to submit a patch :)