drm: fix radeon setparam on 32/64 bit systems.

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9b01bd5b284bbf519b726b39f1352023cb5e9e69
Commit:     9b01bd5b284bbf519b726b39f1352023cb5e9e69
Parent:     dc7a93190c21edbf3ed23e678ad04f852b9cff28
Author:     Dave Airlie <[email protected]>
AuthorDate: Sun Jun 10 16:00:27 2007 +1000
Committer:  Dave Airlie <[email protected]>
CommitDate: Sun Jun 10 16:00:27 2007 +1000

drm: fix radeon setparam on 32/64 bit systems.

The alignment on 64-bit is different for 64-bit values.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/char/drm/radeon_ioc32.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

drivers/char/drm/radeon_ioc32.c +26 -1

index 1f1f9cc..04126c2 100644
@@ -349,6 +349,31 @@ static int compat_radeon_irq_emit(struct file *file, unsigned int cmd,
 			 DRM_IOCTL_RADEON_IRQ_EMIT, (unsigned long)request);
 }
 
+typedef struct drm_radeon_setparam32 {
+	int param;
+	u64 value;
+} __attribute__((packed)) drm_radeon_setparam32_t;
+
+static int compat_radeon_cp_setparam(struct file *file, unsigned int cmd,
+				     unsigned long arg)
+{
+	drm_radeon_setparam32_t req32;
+	drm_radeon_setparam_t __user *request;
+
+	if (copy_from_user(&req32, (void __user *) arg, sizeof(req32)))
+		return -EFAULT;
+
+	request = compat_alloc_user_space(sizeof(*request));
+	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
+	    || __put_user(req32.param, &request->param)
+	    || __put_user((void __user *)(unsigned long)req32.value,
+			  &request->value))
+		return -EFAULT;
+
+	return drm_ioctl(file->f_dentry->d_inode, file,
+			 DRM_IOCTL_RADEON_SETPARAM, (unsigned long) request);
+}
+
 drm_ioctl_compat_t *radeon_compat_ioctls[] = {
 	[DRM_RADEON_CP_INIT] = compat_radeon_cp_init,
 	[DRM_RADEON_CLEAR] = compat_radeon_cp_clear,

@@ -357,6 +382,7 @@ drm_ioctl_compat_t *radeon_compat_ioctls[] = {
 	[DRM_RADEON_VERTEX2] = compat_radeon_cp_vertex2,
 	[DRM_RADEON_CMDBUF] = compat_radeon_cp_cmdbuf,
 	[DRM_RADEON_GETPARAM] = compat_radeon_cp_getparam,
+	[DRM_RADEON_SETPARAM] = compat_radeon_cp_setparam,
 	[DRM_RADEON_ALLOC] = compat_radeon_mem_alloc,
 	[DRM_RADEON_IRQ_EMIT] = compat_radeon_irq_emit,
 };
-

To unsubscribe from this list: send the line "unsubscribe git-commits-head" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Linux Kernel Mailing List

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9b01bd5b284bbf519b726b39f1352023cb5e9e69
Commit:     9b01bd5b284bbf519b726b39f1352023cb5e9e69
Parent:     dc7a93190c21edbf3ed23e678ad04f852b9cff28
Author:     Dave Airlie <[email protected]>
AuthorDate: Sun Jun 10 16:00:27 2007 +1000
Committer:  Dave Airlie <[email protected]>
CommitDate: Sun Jun 10 16:00:27 2007 +1000

Quoted 3 lines

drm: fix radeon setparam on 32/64 bit systems.

The alignment on 64-bit is different for 64-bit values.

Only on i386. I think you just broke ppc32-on-ppc64.

Quoted 2 lines

Only on i386. I think you just broke ppc32-on-ppc64.

Indeed... I'm pretty sure the 64 bits quantity will be naturally aligned on ppc32 (and possibly others).

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Hopefully, this patch improves the situation, it introduces two new types, compat_u64 and compat_s64. These are defined on all architectures to have the same size and alignment as the 32 bit version of u64 and s64.

Signed-off-by Arnd Bergmann <[email protected]>

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Quoted 6 lines · On Friday 15 June 2007 11:31:37 Arnd Bergmann wrote:

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Why does it break them? It should just make them a little slower.

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

It's the _location_ which is wrong;

Quoted 6 lines · On Friday 15 June 2007 11:31:37 Arnd Bergmann wrote:

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Why does it break them? It should just make them a little slower.

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

That is broken on all non-x86 architectures,

Quoted 1 line

That is broken on all non-x86 architectures,

It cannot be broken, it just might be somewhat slower

Quoted 1 line

That is broken on all non-x86 architectures,

It cannot be broken, it just might be somewhat slower

Why does it break them? It should just make them a little slower.

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

Quoted 1 line · Andi Kleen

Why does it break them? It should just make them a little slower.

Not all CPUs deliver recoverable misalignment exceptions.

Quoted 2 lines

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

I thought we'd fixed all that.

Quoted 2 lines

Quoted 1 line

Why does it break them? It should just make them a little slower.

Not all CPUs deliver recoverable misalignment exceptions.

These CPUs are too broken to run Linux then.

Quoted 5 lines

Quoted 3 lines

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

I thought we'd fixed all that.

Did you audit the complete network stack?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The network code requires unaligned accesses to work anyways so if your architecture doesn't support them it is already remotely crashable.

I thought we'd fixed all that.

Did you audit the complete network stack?

For the parts used by the processors in question yes

people have done
that work so using the types without unaligned.

Quoted 1 line

For the parts used by the processors in question yes

That means? They're expected to run only a subset of the network stack? Is that expressed in Kconfig? Is it documented that the rest is dangerous?

Quoted 2 lines

people have done
that work so using the types without unaligned.

Very brave; we're talking about around half a million lines of non trivial source code here.

And debug simulators that can be made to trap such accesses, and in most cases processors which fault such an access (so you find it) but don't provide enough information to restart.

The testing isn't that hard for a given embedded system and having done work Linux does not need other changes re-breaking things.

Hopefully everybody who deploys these systems knows this. It seems like a open death trap to me, especially since the consequences are so severe: remote packet of death, could be a recall for a network conntected embedded device that doesn't easily allow firmware update. And they would rightfully blame Linux.

It would be much safer if the parts of the stack that weren't audited/tested were marked this way and check for BROKEN_UNALIGNED or similar.

Also frankly I'm surprised that whoever designed these systems didn't learn from the old M68000 who made this mistake the first time.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The network code requires unaligned accesses to work anyways so if

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

architecture doesn't support them it is already remotely crashable.

I thought we'd fixed all that.

Did you audit the complete network stack?

For the parts used by the processors in question yes

That means? They're expected to run only a subset of the network stack? Is that expressed in Kconfig? Is it documented that the rest is dangerous?

Quoted 2 lines

people have done
that work so using the types without unaligned.

Very brave; we're talking about around half a million lines of non trivial source code here.

For the architecture we use (Blackfin), it does not support unaligned accesses, and we purposely never put in the trap/fixup code - we trap, and printk("fix your source");

Quoted 5 lines

Quoted 2 lines

people have done
that work so using the types without unaligned.

Very brave; we're talking about around half a million lines of non trivial source code here.

Is there something specific that you can think of that we should be testing?

We have done alot of testing, and people have shipped alot of systems connected to a varity of networks, and have run all kinds of protocols on them.

Quoted 3 lines

For the architecture we use (Blackfin), it does not support unaligned accesses, and we purposely never put in the trap/fixup code - we trap, and printk("fix your source");

For the kernel you should fix up too in addition to the printk. Otherwise you risk a ping of death in the field with some more obscure protocol. Also the printk should be load limited.

For the parts used by the processors in question yes people have done that work so using the types without unaligned.

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Hopefully, this patch improves the situation, it introduces two new types, compat_u64 and compat_s64. These are defined on all architectures to have the same size and alignment as the 32 bit version of u64 and s64.

You're relying on compat_[us]64 being only used in structures which are already packed.

If someone uses them in a non-packed struct, they won't
decrease the alignment. I think it would be more effective to specify it as:
__attribute__((aligned(4), packed))

You're relying on compat_[us]64 being only used in structures which are already packed.  If someone uses them in a non-packed struct, they won't decrease the alignment.  I think it would be more effective to specify it as:

__attribute__((aligned(4), packed))

The other problem is that if someone defines a struct like this:

struct foo {
        short bar;
        compat_s64 baz;
} __attribute__((packed))

it'll have different definitions on x86 and ia64.

So I think we should be aiming for the ((aligned, packed)) definition and remove the __attribute__((packed)) from the struct definitions.  What do you think?

That's what I thought as well at first, since this is how the gcc documentation seems to describe it. However, recent version of gcc complain about this:

gcc-4.1 -Wall -O2 test.c -c
test.c:1: warning: 'packed' attribute ignored

I have tested versions 2.95, 3.3 and 4.1, an they all ignore do the right thing when you do not specify the packed attribute.

We might ask the gcc developers to clarify the documentation, which as of 4.1 states:

The aligned' attribute can only increase the alignment; but you can decrease it by specifying packed' as well. See below.

My understanding is that this only applies to statically allocated variables, but not to automatic stack variables and to usage of the type inside of a data structure.

Here's a program which illustrates the source of confusion:

#include <stdio.h>
#include <stddef.h>

typedef unsigned long long __attribute__((aligned(4))) compat_u64;

struct foo {
        int y;
        unsigned long long __attribute__((aligned(4))) x; };

struct bar {
        int y;
        compat_u64 x;
};

int main(void)
{
        printf("offset of foo->x is %lu\n", offsetof(struct foo, x)); printf("offset of bar->x is %lu\n", offsetof(struct bar, x)); return 0;
}

output (on ia64, and I'm told other 64-bit platforms) is:

$ ./test 
offset of foo->x is 8
offset of bar->x is 4

I'll try and come up with some wording that works for the GCC manual.

$ ./test 
offset of foo->x is 8
offset of bar->x is 4

One common problem with 32 bit system call and ioctl emulation is the different alignment rules between i386 and 64 bit machines. A number of drivers work around this by marking the compat structures as 'attribute((packed))', which is not the right solution because it breaks all the non-x86 architectures that want to use the same compat code.

Hopefully, this patch improves the situation, it introduces two new types, compat_u64 and compat_s64. These are defined on all architectures to have the same size and alignment as the 32 bit version of u64 and s64.

Signed-off-by Arnd Bergmann <[email protected]>

Hopefully, this patch improves the situation, it introduces two new types, compat_u64 and compat_s64. These are defined on all architectures to have the same size and alignment as the 32 bit version of u64 and s64.

Will GCC know that it needs to emit code to handle that (mis)alignment?

Preliminary tests show that it does load the value bytewise if we use the 'packed' attribute structure on ppc64, but doesn't if we use compat_u64. But then, I don't think it actually _needs_ to handle it n ppc64 anyway, so maybe that's not such a good test.

Quoted 1 line · David Woodhouse

Will GCC know that it needs to emit code to handle that (mis)alignment?

I've tested this with gcc-4.0.3, and it does the right thing, which is to split a 4 byte aligned 64 bit load/store into two 32 bit accesses, if you pass -mstrict-align.

Quoted 6 lines · Arnd Bergmann

Quoted 1 line · David Woodhouse

Will GCC know that it needs to emit code to handle that (mis)alignment?

I've tested this with gcc-4.0.3, and it does the right thing, which is to split a 4 byte aligned 64 bit load/store into two 32 bit accesses, if you pass -mstrict-align.

I just realized this was correct but slightly misleading. On powerpc, we don't set the 'attribute((aligned(4)))' on compat_64, so there is never a reason to handle the misalignment, even though it would work.

On x86_64, misaligned loads are always ok, so gcc never needs to care about this, even attribute((packed)) does not cause byte access here.

You're right. My question was probably not relevant -- all these 64-bit architectures cope with misaligned loads anyway. If we ever have to deal with 32-bit compat on a 64-bit architecture which can't handle misalignment, I'm just going to hide under my desk and never come out.

Quoted 3 lines

On x86_64, misaligned loads are always ok, so gcc never needs to care about this, even attribute((packed)) does not cause byte access here.

IA64 too, but it'll be handled there too -- either naturally or by fixups; it doesn't matter.