linux-user: fix preadv/pwritev offsets
PATCH v3 1/1 linux-user: fix preadv/pwritev offsets
preadv/pwritev accept low and high parts of file offset in two separate parameters. When host bitness doesn't match guest bitness these parts must be appropriately recombined.
Introduce target_to_host_low_high that does this recombination and use it in preadv/pwritev syscalls.
This fixes glibc testsuite test misc/tst-preadvwritev64.
---
Changes v2->v3:
- rename helper to target_to_host_low_high;
- handle cases TARGET_LONG_BITS * 2 > HOST_LONG_BITS and TARGET_LONG_BITS < 2 * HOST_LONG_BITS correctly.
Changes v1->v2:
- fix host high computation in TARGET_LONG_BITS > HOST_LONG_BITS case
linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)
linux-user/syscall.c +32 -2
@@ -3386,6 +3386,30 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,return ret; } +static void target_to_host_low_high (abi_ulong tlow, +abi_ulong thigh, +unsigned long *hlow, +unsigned long *hhigh) +{ +#if TARGET_LONG_BITS ==HOST_LONG_BITS + *hlow = tlow; + *hhigh = thigh; +#elif TARGET_LONG_BITS <HOST_LONG_BITS + *hlow = tlow | (unsigned long )thigh <<TARGET_LONG_BITS ; +#if TARGET_LONG_BITS *2 <=HOST_LONG_BITS + *hhigh =0 ; +#else + *hhigh = (unsigned long )thigh >> (HOST_LONG_BITS -TARGET_LONG_BITS ); +#endif +#else + *hlow = (unsigned long )tlow; + *hhigh = (unsigned long )(tlow >>HOST_LONG_BITS ); +#if TARGET_LONG_BITS <2 *HOST_LONG_BITS + *hhigh |= (unsigned long )thigh << (TARGET_LONG_BITS -HOST_LONG_BITS ); +#endif +#endif +} +static struct iovec *lock_iovec (int type,abi_ulong target_addr,abi_ulong count,int copy) {
@@ -10449,7 +10473,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, {struct iovec *vec =lock_iovec (VERIFY_WRITE , arg2, arg3,0 );if (vec !=NULL ) { - ret =get_errno (safe_preadv (arg1, vec, arg3, arg4, arg5)); +unsigned long low, high; + +target_to_host_low_high (arg4, arg5, &low, &high); + ret =get_errno (safe_preadv (arg1, vec, arg3, low, high));unlock_iovec (vec, arg2, arg3,1 ); }else { ret = -host_to_target_errno (errno);
@@ -10462,7 +10489,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, {struct iovec *vec =lock_iovec (VERIFY_READ , arg2, arg3,1 );if (vec !=NULL ) { - ret =get_errno (safe_pwritev (arg1, vec, arg3, arg4, arg5)); +unsigned long low, high; + +target_to_host_low_high (arg4, arg5, &low, &high); + ret =get_errno (safe_pwritev (arg1, vec, arg3, low, high));unlock_iovec (vec, arg2, arg3,0 ); }else { ret = -host_to_target_errno (errno);
Signature · 1 line
2.11.0
Le 05/04/2018 à 15:47, Max Filippov a écrit :
Quoted 42 lines
preadv/pwritev accept low and high parts of file offset in two separate parameters. When host bitness doesn't match guest bitness these parts must be appropriately recombined.
Introducetarget_to_host_low_highthat does this recombination and use it in preadv/pwritev syscalls.This fixes glibc testsuite test misc/tst-preadvwritev64.
Signed-off-by Max Filippov <[email protected]>---
Changesv2->v3:
- rename helper totarget_to_host_low_high;
- handle casesTARGET_LONG_BITS* 2 >HOST_LONG_BITSandTARGET_LONG_BITS< 2 *HOST_LONG_BITScorrectly.Changes
v1->v2:
- fix host high computation inTARGET_LONG_BITS>HOST_LONG_BITScaselinux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)linux-user/syscall.c +13
-0@@ -3386,6 +3386,30 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,return ret; } +static void target_to_host_low_high (abi_ulong tlow, +abi_ulong thigh, +unsigned long *hlow, +unsigned long *hhigh) +{ +#if TARGET_LONG_BITS ==HOST_LONG_BITS + *hlow = tlow; + *hhigh = thigh; +#elif TARGET_LONG_BITS <HOST_LONG_BITS + *hlow = tlow | (unsigned long )thigh <<TARGET_LONG_BITS ; +#if TARGET_LONG_BITS *2 <=HOST_LONG_BITS + *hhigh =0 ; +#else
linux-user/syscall.c static abi_long do_getsockopt(int sockfd, int level, int optname, · line 3411
Quoted 10 lines
linux-user/syscall.c +10
-0+ *hhigh = (unsigned long )thigh >> (HOST_LONG_BITS -TARGET_LONG_BITS ); +#endif +#else + *hlow = (unsigned long )tlow; + *hhigh = (unsigned long )(tlow >>HOST_LONG_BITS ); +#if TARGET_LONG_BITS <2 *HOST_LONG_BITS + *hhigh |= (unsigned long )thigh << (TARGET_LONG_BITS -HOST_LONG_BITS ); +#endif +#endif +}
Why don't you try to de-construct then re-construct the offset?
Kernel commit
601cc11d054a "Make non-compat preadv/pwritev use native register size"
is interesting.
static inline abi_llong target_pos_from_hilo(abi_ulong high,
abi_ulong low)
{
#define TARGET_HALF_LONG_BITS (TARGET_LONG_BITS / 2)
return (((abi_llong)high << TARGET_HALF_LONG_BITS) <<
TARGET_HALF_LONG_BITS) | low; }
#define HOST_HALF_LONG_BITS (HOST_LONG_BITS / 2)
abi_llong pos = target_pos_from_hilo(arg4, arg5); ret = get_errno(safe_preadv(arg1, vec, arg3,
pos,
(pos >> HOST_HALF_LONG_BITS) >> HOST_HALF_LONG_BITS));It looks simpler, but perhaps I miss something
(it's just cut&paste, I don't pretend to understand that code...)?
Richard?
Thanks,
Laurent
Quoted 1 line · Laurent Vivier
Why don't you try to de-construct then re-construct the offset?
It would require 128-bit arithmetic on 64-bit host.
Quoted 14 lines
Kernel commit
601cc11d054a"Make non-compat preadv/pwritev use native register size"
is interesting.static inline
abi_llongtarget_pos_from_hilo(abi_ulong high,
abi_ulonglow)
{
#defineTARGET_HALF_LONG_BITS(TARGET_LONG_BITS/ 2)
return (((abi_llong)high <<TARGET_HALF_LONG_BITS) <<TARGET_HALF_LONG_BITS) | low; }#define
HOST_HALF_LONG_BITS(HOST_LONG_BITS/ 2)
Quoted 7 lines
abi_llongpos =target_pos_from_hilo(arg4, arg5); ret = get_errno(safe_preadv(arg1, vec, arg3,pos, (pos >> HOST_HALF_LONG_BITS) >> HOST_HALF_LONG_BITS));It looks simpler, but perhaps I miss something
(it's just cut&paste, I don't pretend to understand that code...)?
If we decide that host unsigned long long is wide enough to represent meaningful file positions then this function may be simplified to something like
unsigned long long off = (unsigned long long)tlow |
((unsigned long long)thigh << (TARGET_LONG_BITS / 2)) << (TARGET_LONG_BITS / 2); *hlow = (unsigned long)off;
*hhigh = (unsigned long)((off >> (HOST_LONG_BITS / 2)) >> (HOST_LONG_BITS / 2));
There's also a bug that the target may specify an offset not representable on the host, that will get truncated and point to a different location in the file, possibly resulting in data corruption.
Signature · 2 lines
Thanks. -- Max
Le 05/04/2018 à 18:27, Max Filippov a écrit :
Quoted 31 lines · Laurent Vivier
Quoted 1 line
Why don't you try to de-construct then re-construct the offset?
It would require 128-bit arithmetic on 64-bit host.
Quoted 14 lines
Kernel commit
601cc11d054a"Make non-compat preadv/pwritev use native register size"
is interesting.static inline
abi_llongtarget_pos_from_hilo(abi_ulong high,
abi_ulonglow)
{
#defineTARGET_HALF_LONG_BITS(TARGET_LONG_BITS/ 2)
return (((abi_llong)high <<TARGET_HALF_LONG_BITS) <<TARGET_HALF_LONG_BITS) | low; }#define
HOST_HALF_LONG_BITS(HOST_LONG_BITS/ 2)Quoted 7 lines
abi_llongpos =target_pos_from_hilo(arg4, arg5); ret = get_errno(safe_preadv(arg1, vec, arg3,pos, (pos >> HOST_HALF_LONG_BITS) >> HOST_HALF_LONG_BITS));It looks simpler, but perhaps I miss something
(it's just cut&paste, I don't pretend to understand that code...)?If we decide that host unsigned long long is wide enough to represent meaningful file positions then this function may be simplified to something like
Quoted 8 lines
unsigned long long off = (unsigned long long)tlow |
((unsigned long long)thigh << (TARGET_LONG_BITS/ 2)) << (TARGET_LONG_BITS/ 2); *hlow = (unsigned long)off;
*hhigh = (unsigned long)((off >> (HOST_LONG_BITS/ 2)) >> (HOST_LONG_BITS/ 2));There's also a bug that the target may specify an offset not representable on the host, that will get truncated and point to a different location in the file, possibly resulting in data corruption.
I don't think it can happen, because "long long" is always 64bit, so it fits in two 32bit (long is 32bit on 32bit, 64bit on 64bit).
Thanks,
Laurent