Re: sys getdents64 needs compat wrapper ?

[Arnd Bergmann <arnd@arndb.de>]

[-- Attachment #1: Type: text/plain, Size: 5306 bytes --]

On Freitag, 18. Juni 2004 00:28, Arun Sharma wrote:
> It took us a bit longer :) But here's the promised patch. Using
> __put_user_unaligned() on ia64 may still cause unaligned faults,
> but we chose to optimize for the common case, where it's 4 byte
> aligned.   

Isn't it legal for i386 code to pass syscall arguments with
an arbitrary alignment? I guess gcc normally aligns everything
to 32 bit unless you force it not to but you might still return
error for syscalls that work fine on a native i386 system.

> + */
> +#define __get_user_unaligned(x, ptr)                                   \
> +({                                                                     \
> +       __typeof__ (*(ptr)) __x = (x);                                  \
> +       copy_from_user(&__x, (ptr), sizeof(*(ptr))) ? -EFAULT : 0;      \
> +})

This should be __copy_from_user instead of copy_from_user, right?

> +
> +
> +/* 
> + * This macro should be used instead of __put_user() when accessing
> + * values at locations that are unknown to be aligned.
> + */
> +#define __put_user_unaligned(x, ptr)                                   \
> +({                                                                     \
> +       __typeof__ (*(ptr)) __x = (x);                                  \
> +       copy_to_user((ptr), &__x, sizeof(*(ptr))) ? -EFAULT : 0;        \
> +})

same here.

> +extern long __put_user_unaligned_unknown (void);
> +
> +#define __put_user_unaligned(x, ptr)                                                           \
> +({                                                                                             \
> +       long __ret;                                                                             \
> +       switch (sizeof(*(ptr))) {                                                               \
> +               case 1: __ret = __put_user((x), (ptr)); break;                                  \
> +               case 2: __ret = (__put_user((x), (u8 __user *)(ptr)))                           \
> +                       || (__put_user((x) >> 8, ((u8 __user *)(ptr) + 1))); break;             \
> +               case 4: __ret = (__put_user((x), (u16 __user *)(ptr)))                          \
> +                       || (__put_user((x) >> 16, ((u16 __user *)(ptr) + 1))); break;           \
> +               case 8: __ret = (__put_user((x), (u32 __user *)(ptr)))                          \
> +                       || (__put_user((x) >> 32, ((u32 __user *)(ptr) + 1))); break;           \
> +               default: __ret = __put_user_unaligned_unknown();                                \
> +       }                                                                                       \
> +       __ret;                                                                                  \
> +})

Under what circumstances would you need to break down a 4 byte
alignment into 2 bytes? I can understand that you want to optimize
the unaligned-64 bit case because i386-gcc aligns them only to 32 bit
in user space, but the other special cases are bogus. Why not
just do:

#define __put_user_unaligned(x, ptr)					\
(									\
	(sizeof(*(ptr)) == 8) ?						\
		((__put_user((x), (u32 __user *)(ptr)))			\
		 | (__put_user((x) >> 32, ((u32 __user *)(ptr) + 1))))	\
		: __put_user(x,ptr)					\
)

Note also the use of | instead of || to get the correct value on error
(-EFAULT, not 1).

> diff -purN -X dontdiff linux-2.6.7-rc3-getdents/include/asm-ppc64/uaccess.h linux-2.6.7-rc3-getdents-user_unaligned/include/asm-ppc64/uaccess.h
> --- linux-2.6.7-rc3-getdents/include/asm-ppc64/uaccess.h        2004-06-15 13:41:35.000000000 +0800
> +++ linux-2.6.7-rc3-getdents-user_unaligned/include/asm-ppc64/uaccess.h 2004-06-17 14:51:42.020600447 +0800
> @@ -12,6 +12,7 @@
>  #include <linux/sched.h>
>  #include <linux/errno.h>
>  #include <asm/processor.h>
> +#include <asm-generic/uaccess.h>
>  
>  #define VERIFY_READ    0
>  #define VERIFY_WRITE   1
> diff -purN -X dontdiff linux-2.6.7-rc3-getdents/include/asm-s390/uaccess.h linux-2.6.7-rc3-getdents-user_unaligned/include/asm-s390/uaccess.h
> --- linux-2.6.7-rc3-getdents/include/asm-s390/uaccess.h 2004-06-15 13:41:39.000000000 +0800
> +++ linux-2.6.7-rc3-getdents-user_unaligned/include/asm-s390/uaccess.h  2004-06-17 14:51:42.021577010 +0800
> @@ -16,6 +16,7 @@
>   */
>  #include <linux/sched.h>
>  #include <linux/errno.h>
> +#include <asm-generic/uaccess.h>
>  
>  #define VERIFY_READ     0
>  #define VERIFY_WRITE    1

ppc64 and s390 both don't care about alignment, so simply using

#define __get_user_unaligned __get_user
#define __put_user_unaligned __put_user

would make more sense here. See include/asm-*/unaligned.h

> diff -purN -X dontdiff linux-2.6.7-rc3-getdents/include/asm-x86_64/uaccess.h linux-2.6.7-rc3-getdents-user_unaligned/include/asm-x86_64/uaccess.h
> --- linux-2.6.7-rc3-getdents/include/asm-x86_64/uaccess.h       2004-06-15 13:41:34.000000000 +0800
> +++ linux-2.6.7-rc3-getdents-user_unaligned/include/asm-x86_64/uaccess.h        2004-06-17 14:51:42.022553572 +0800
> @@ -10,6 +10,7 @@
>  #include <linux/sched.h>
>  #include <linux/prefetch.h>
>  #include <asm/page.h>
> +#include <asm-generic/uaccess.h>
>  
>  #define VERIFY_READ 0
>  #define VERIFY_WRITE 1

Same on x86_64.

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]