Re: [Qemu-devel] [PATCH 14/18] linux-user: Use __get_user() and __put_user() to handle structs in do_fcntl()

[Peter Maydell <peter.maydell@linaro.org>]

On 7 June 2016 at 21:41, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 06/06/2016 à 20:58, Peter Maydell a écrit :
>> Use the __get_user() and __put_user() to handle reading and writing the
>> guest structures in do_ioctl(). This has two benefits:
>>  * avoids possible errors due to misaligned guest pointers
>>  * correctly sign extends signed fields (like l_start in struct flock)
>>    which might be different sizes between guest and host
>>
>> To do this we abstract out into copy_from/to_user functions. We
>> also standardize on always using host flock64 and the F_GETLK64
>> etc flock commands, as this means we always have 64 bit offsets
>> whether the host is 64-bit or 32-bit and we don't need to support
>> conversion to both host struct flock and struct flock64.
>>
>> In passing we fix errors in converting l_type from the host to
>> the target (where we were doing a byteswap of the host value
>> before trying to do the convert-bitmasks operation rather than
>> otherwise, and inexplicably shifting left by 1).
>
> I  think the ">> 1" is coming from:
>
> 43f238d Support fcntl F_GETLK64, F_SETLK64, F_SETLKW64
>
> to convert arm to x86, and should have been removed then in:
>
> 2ba7f73 alpha-linux-user: Translate fcntl l_type
>
> So yes, the ">> 1" is wrong. I don't understand how it can work.

Thanks for tracking down where it came from. I suspect it
just didn't work and nobody noticed, because:
 * there's not much use of big-on-little-endian
 * a lot of the time the bug is just going to downgrade an
   exclusive lock to a shared lock, and you won't notice if
   there isn't actually any contention on the lock...

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  linux-user/syscall.c | 280 +++++++++++++++++++++++++++++----------------------
>>  1 file changed, 157 insertions(+), 123 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 4cf67c8..f3a487e 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -4894,11 +4894,11 @@ static int target_to_host_fcntl_cmd(int cmd)
>>       case TARGET_F_SETFL:
>>              return cmd;
>>          case TARGET_F_GETLK:
>> -         return F_GETLK;
>> -     case TARGET_F_SETLK:
>> -         return F_SETLK;
>> -     case TARGET_F_SETLKW:
>> -         return F_SETLKW;
>> +            return F_GETLK64;
>> +        case TARGET_F_SETLK:
>> +            return F_SETLK64;
>> +        case TARGET_F_SETLKW:
>> +            return F_SETLKW64;
>>       case TARGET_F_GETOWN:
>>           return F_GETOWN;
>>       case TARGET_F_SETOWN:
>
> I see no reason to have this in this patch.

The idea is that we want to use only one host flock struct,
which means it must be the one which supports 64-bit offsets.
On a 32-bit host, that's the flock64 struct, which must be
used with the F_GETLK64 fcntl, not F_GETLK.
On a 64-bit host, the system headers define that F_GETLK64
and F_GETLK are identical (and that the flock64 struct is flock),
so instead of having to specialcase 64-bit hosts, we can just
say "use the F_*64 constants and struct flock64 everywhere".

If we didn't have this hunk of the patch then on a 32-bit
host the code below would go wrong, because when we did
a guest F_GETLK we'd end up passing a (host) struct flock64
to the 32-bit F_GETLK.

>>      case TARGET_F_GETLK:
>> -        if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1))
>> +        if (copy_from_user_flock(&fl64, arg)) {
>>              return -TARGET_EFAULT;
>
> why do you ignore the exact value returned by copy_from_user_flock()?
> You should return this value instead of guessing it.

Yeah, I was being lazy and not wanting to have an extra 'ret'
variable floating around. I'll fix this.

>> -        fl64.l_type =
>> -           target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1;
>
> The ">> 1" disappears...

...and it's correct that it disappears, right?

thanks
-- PMM