Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Fabrice Bellard]

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.