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

[Fabrice Bellard]

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.

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).

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;
                    }
            }
        }

Personally I prefer the Linux API style, but Paul's lock/unlock is also
perfectly logical. The advantage of keeping the Paul's API is that you
can minimize the number of changes.

Another point is that if you use the Linux API style, it is not needed
to change the API as you want to do. The only useful addition I see is
to add an automatic tswap in get/put/__get/__put user as it is done now.

Moreover, it may be questionnable to do the page right check in
access_ok(). The Linux kernel does not do it at that point : access_ok()
only verifies that the address is in the user address space.

> [...]

Fabrice.