Patchwork [6/8] linux-user: Rewrite __get_user/__put_user with __builtin_choose_expr

login
register
mail settings
Submitter Richard Henderson
Date Jan. 5, 2013, 12:39 a.m.
Message ID <1357346373-13898-7-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/209583/
State New
Headers show

Comments

Richard Henderson - Jan. 5, 2013, 12:39 a.m.
The previous formuation with multiple assignments to __typeof(*hptr) falls
down when hptr is qualified const.  E.g. with const struct S *p, p->f is
also qualified const.

With this formulation, there's no assignment to any local variable.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/qemu.h | 63 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 30 deletions(-)
Laurent Desnogues - Jan. 23, 2013, 6:31 p.m.
Hi,

I know this was committed some time ago, but I have a
comment about this patch.

On Sat, Jan 5, 2013 at 1:39 AM, Richard Henderson <rth@twiddle.net> wrote:
> The previous formuation with multiple assignments to __typeof(*hptr) falls
> down when hptr is qualified const.  E.g. with const struct S *p, p->f is
> also qualified const.
>
> With this formulation, there's no assignment to any local variable.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/qemu.h | 63 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8a3538c..31a220a 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -287,36 +287,39 @@ static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
>                              (type == VERIFY_READ) ? PAGE_READ : (PAGE_READ | PAGE_WRITE)) == 0;
>  }
>
> -/* NOTE __get_user and __put_user use host pointers and don't check access. */
> -/* These are usually used to access struct data members once the
> - * struct has been locked - usually with lock_user_struct().
> - */
> -#define __put_user(x, hptr)\
> -({ __typeof(*hptr) pu_ = (x);\
> -    switch(sizeof(*hptr)) {\
> -    case 1: break;\
> -    case 2: pu_ = tswap16(pu_); break; \
> -    case 4: pu_ = tswap32(pu_); break; \
> -    case 8: pu_ = tswap64(pu_); break; \
> -    default: abort();\
> -    }\
> -    memcpy(hptr, &pu_, sizeof(pu_)); \
> -    0;\
> -})
> -
> -#define __get_user(x, hptr) \
> -({ __typeof(*hptr) gu_; \
> -    memcpy(&gu_, hptr, sizeof(gu_)); \
> -    switch(sizeof(*hptr)) {\
> -    case 1: break; \
> -    case 2: gu_ = tswap16(gu_); break; \
> -    case 4: gu_ = tswap32(gu_); break; \
> -    case 8: gu_ = tswap64(gu_); break; \
> -    default: abort();\
> -    }\
> -    (x) = gu_; \
> -    0;\
> -})
> +/* NOTE __get_user and __put_user use host pointers and don't check access.
> +   These are usually used to access struct data members once the struct has
> +   been locked - usually with lock_user_struct.  */
> +
> +/* Tricky points:
> +   - Use __builtin_choose_expr to avoid type promotion from ?:,
> +   - Invalid sizes result in a compile time error stemming from
> +     the fact that abort has no parameters.
> +   - It's easier to use the endian-specific unaligned load/store
> +     functions than host-endian unaligned load/store plus tswapN.  */
> +
> +#define __put_user_e(x, hptr, e)                                        \
> +  (__builtin_choose_expr(sizeof(*(hptr)) == 1, stb_p,                   \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 2, stw_##e##_p,             \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 4, stl_##e##_p,             \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 8, stq_##e##_p, abort))))   \
> +     ((hptr), (x)), 0)
> +
> +#define __get_user_e(x, hptr, e)                                        \
> +  ((x) =                                                                \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 1, ldub_p,                  \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 2, lduw_##e##_p,            \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 4, ldl_##e##_p,             \
> +   __builtin_choose_expr(sizeof(*(hptr)) == 8, ldq_##e##_p, abort))))   \
> +     (hptr), 0)

For 8- and 16-bit quantities the load is explicitly unsigned
through the use of ldub and lduw.  But for 32-bit, ldl_[bl]e_p
return an int, so if x is a 64-bit variable sign-extension will
happen.  I'm not sure this is desirable, for instance when
using get_user_u32 which makes one think the result is an
unsigned 32-bit value.  Shouldn't ldul*_p functions be added
and used in __get_user_e?

Note I found this in private code, but wonder if some public
code isn't affected by this.

Thanks,

Laurent

> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +# define __put_user(x, hptr)  __put_user_e(x, hptr, be)
> +# define __get_user(x, hptr)  __get_user_e(x, hptr, be)
> +#else
> +# define __put_user(x, hptr)  __put_user_e(x, hptr, le)
> +# define __get_user(x, hptr)  __get_user_e(x, hptr, le)
> +#endif
>
>  /* put_user()/get_user() take a guest address and check access */
>  /* These are usually used to access an atomic data type, such as an int,
> --
> 1.7.11.7
>
>
Peter Maydell - Jan. 31, 2013, 11:15 a.m.
On 23 January 2013 18:31, Laurent Desnogues <laurent.desnogues@gmail.com> wrote:
> On Sat, Jan 5, 2013 at 1:39 AM, Richard Henderson <rth@twiddle.net> wrote:
>> +#define __get_user_e(x, hptr, e)                                        \
>> +  ((x) =                                                                \
>> +   __builtin_choose_expr(sizeof(*(hptr)) == 1, ldub_p,                  \
>> +   __builtin_choose_expr(sizeof(*(hptr)) == 2, lduw_##e##_p,            \
>> +   __builtin_choose_expr(sizeof(*(hptr)) == 4, ldl_##e##_p,             \
>> +   __builtin_choose_expr(sizeof(*(hptr)) == 8, ldq_##e##_p, abort))))   \
>> +     (hptr), 0)
>
> For 8- and 16-bit quantities the load is explicitly unsigned
> through the use of ldub and lduw.  But for 32-bit, ldl_[bl]e_p
> return an int, so if x is a 64-bit variable sign-extension will
> happen.  I'm not sure this is desirable, for instance when
> using get_user_u32 which makes one think the result is an
> unsigned 32-bit value.  Shouldn't ldul*_p functions be added
> and used in __get_user_e?
>
> Note I found this in private code, but wonder if some public
> code isn't affected by this.

I just did an audit of all the uses of get_user_u32 in the codebase
and I think the only one that runs into this (ie does get_user_u32
into a variable which is 64 bits wide) is the PPC do_store_exclusive()
in linux-user/main.c. So probably this patch broke PPC64 linux-user
32 bit exclusive stores.

-- PMM

Patch

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 8a3538c..31a220a 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -287,36 +287,39 @@  static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
                             (type == VERIFY_READ) ? PAGE_READ : (PAGE_READ | PAGE_WRITE)) == 0;
 }
 
-/* NOTE __get_user and __put_user use host pointers and don't check access. */
-/* These are usually used to access struct data members once the
- * struct has been locked - usually with lock_user_struct().
- */
-#define __put_user(x, hptr)\
-({ __typeof(*hptr) pu_ = (x);\
-    switch(sizeof(*hptr)) {\
-    case 1: break;\
-    case 2: pu_ = tswap16(pu_); break; \
-    case 4: pu_ = tswap32(pu_); break; \
-    case 8: pu_ = tswap64(pu_); break; \
-    default: abort();\
-    }\
-    memcpy(hptr, &pu_, sizeof(pu_)); \
-    0;\
-})
-
-#define __get_user(x, hptr) \
-({ __typeof(*hptr) gu_; \
-    memcpy(&gu_, hptr, sizeof(gu_)); \
-    switch(sizeof(*hptr)) {\
-    case 1: break; \
-    case 2: gu_ = tswap16(gu_); break; \
-    case 4: gu_ = tswap32(gu_); break; \
-    case 8: gu_ = tswap64(gu_); break; \
-    default: abort();\
-    }\
-    (x) = gu_; \
-    0;\
-})
+/* NOTE __get_user and __put_user use host pointers and don't check access.
+   These are usually used to access struct data members once the struct has
+   been locked - usually with lock_user_struct.  */
+
+/* Tricky points:
+   - Use __builtin_choose_expr to avoid type promotion from ?:,
+   - Invalid sizes result in a compile time error stemming from
+     the fact that abort has no parameters.
+   - It's easier to use the endian-specific unaligned load/store
+     functions than host-endian unaligned load/store plus tswapN.  */
+
+#define __put_user_e(x, hptr, e)                                        \
+  (__builtin_choose_expr(sizeof(*(hptr)) == 1, stb_p,                   \
+   __builtin_choose_expr(sizeof(*(hptr)) == 2, stw_##e##_p,             \
+   __builtin_choose_expr(sizeof(*(hptr)) == 4, stl_##e##_p,             \
+   __builtin_choose_expr(sizeof(*(hptr)) == 8, stq_##e##_p, abort))))   \
+     ((hptr), (x)), 0)
+
+#define __get_user_e(x, hptr, e)                                        \
+  ((x) =                                                                \
+   __builtin_choose_expr(sizeof(*(hptr)) == 1, ldub_p,                  \
+   __builtin_choose_expr(sizeof(*(hptr)) == 2, lduw_##e##_p,            \
+   __builtin_choose_expr(sizeof(*(hptr)) == 4, ldl_##e##_p,             \
+   __builtin_choose_expr(sizeof(*(hptr)) == 8, ldq_##e##_p, abort))))   \
+     (hptr), 0)
+
+#ifdef TARGET_WORDS_BIGENDIAN
+# define __put_user(x, hptr)  __put_user_e(x, hptr, be)
+# define __get_user(x, hptr)  __get_user_e(x, hptr, be)
+#else
+# define __put_user(x, hptr)  __put_user_e(x, hptr, le)
+# define __get_user(x, hptr)  __get_user_e(x, hptr, le)
+#endif
 
 /* put_user()/get_user() take a guest address and check access */
 /* These are usually used to access an atomic data type, such as an int,