| 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
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 > >
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,
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(-)