Thayne Harbaugh wrote:
> On Mon, 2007-11-05 at 22:42 +0100, Fabrice Bellard wrote:
>> Thayne Harbaugh wrote:
>>> On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote:
>>>> I think that using host addresses in __put_user and __get_user is not
>>>> logical. They should use target addresses as get_user and put_user. As
>>>> Paul said, It is not worth mixing get/put/copy and lock/unlock functions.
>>> Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for
>>> some discussion of get/put/copy and lock/unlock. {get,put}_user() is
>>> used for individual ints or other atomically writable types that are
>>> passed as pointers into a syscall. copy_{to,from}_user_<struct>() are
>>> used for structures that are passed to a syscall. lock/unlock() will be
>>> used internally in these because lock/unlock does address translation.
>>> lock/unlock() are still needed and are independent. __{get,put}_user()
>>> will operate internally in these functions on structure data members
>>> where lock/unlock() access_ok() have already been called.
>> I believed I was clear : once you keep lock/unlock, there is no point in
>> modifying the code to use put_user/get_user and copy[to,from]_user.
>
> without lock/unlock how do you propose that target/host address
> translation be performed? Are you suggesting a g2h() inside of
> copy_{to,from}_user()?
Yes.
Keep in mind that g2h() is only the simple case in case the target
address space is directly addressable. I don't want that the API makes
this supposition, so in the general case copy_[to,from]user are the only
method you have to exchange data with the user space.
>> So either you keep the code as it is and extend lock and unlock to
>> return an error code or you suppress all lock/unlock to use a Linux like
>> API (i.e. put_user/get_user , copy[to,from]_user, access_ok,
>> __put_user/__get_user).
>
> The error code because lock/unlock_user would then call access_ok()?
Of course.
>> So for gettimeofday, if we exclude the fact that the conversion of
>> struct timeval will be factorized, you have either:
>>
>> {
>> struct timeval tv;
>> ret = get_errno(gettimeofday(&tv, NULL));
>> if (!is_error(ret)) {
>> struct target_timeval *target_tv;
>>
>> lock_user_struct(target_tv, arg1, 0);
>> target_tv->tv_sec = tswapl(tv->tv_sec);
>> target_tv->tv_usec = tswapl(tv->tv_usec);
>> if (unlock_user_struct(target_tv, arg1, 1)) {
>> ret = -TARGET_EFAULT;
>> goto fail;
>> }
>> }
>> }
>>
>> Or
>>
>> {
>> struct timeval tv;
>> ret = get_errno(gettimeofday(&tv, NULL));
>> if (!is_error(ret)) {
>> struct target_timeval target_tv;
>>
>> target_tv.tv_sec = tswapl(tv->tv_sec);
>> target_tv.tv_usec = tswapl(tv->tv_usec);
>> if (copy_to_user(arg1, &target_tv, sizeof(target_tv)) {
>> ret = -TARGET_EFAULT;
>> goto fail;
>> }
>> }
>> }
>
> I don't see where the second one is handling target/host address
> translation.
copy_to_user() does it.
> A problem with both of the above examples is that they use tswapl().
> Without the proper type casting tswapl() doesn't do proper sign
> extension when dealing with 64bit/32bit host/target relationships. I've
> fixed more than one location where this has resulted in bugs.
This is an unrelated problem. If tswapl is not sufficient then another
function can be added.
> [...]
Regards,
Fabrice.
