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

[Thayne Harbaugh]

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()?

> 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()?

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

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.  What I'm
suggesting is the following:

static inline abi_long copy_to_user_timeval(abi_ulong target_timeval_addr,
                                            const struct timeval *tv)
{
    if (!access_ok(VERIFY_WRITE, target_tv_addr, sizeof(*target_tv)))
        return -TARGET_EFAULT;

    lock_user_struct(target_tv, target_tv_addr, 0);

    __put_user(tv->tv_sec, &target_tv->tv_sec);
    __put_user(tv->tv_usec, &target_tv->tv_usec);

    unlock_user_struct(target_tv, target_tv_addr, 1);

    return 0;
}

.
.
.
        {
            struct timeval tv;
            ret = get_errno(gettimeofday(&tv, NULL));
            if (!is_error(ret)) {
                if (copy_to_user_timeval(arg1, &tv)) {
                    ret = -TARGET_EFAULT;
                    goto fail;
                }
            }
        }

(Ignoring the factorization of the timeval struct)
copy_to_user_timeval() here properly handles target/host address
translation.  It also uses __put_user() which properly handles any sign
extension.  The difference between the main syscall code and the linux
kernel is simply this:

copy_to_user(arg1, &tv, sizeof(tv)) -> copy_to_user_timeval(arg1, &tv)
                      ^^^^^^^^^^^^                 ^^^^^^^^

Allowing that minor difference (since qemu needs to do translation
between the structure types) is reasonable.  Furthermore the access_ok()
is kept inside the copy_to_user*() function just like in the linux
kernel.  The construct I've shown above is very comparable to kernel
code.


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

Sure, there are advantages to minimal changes.  Personally I prefer the
Linux API style because that's what is being emulated.

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

Why would a tswap() need to be added?  It's already inside of
__{get,put}_user()?  I'm a bit confused since the changes I'm making
don't deviate from the intentions of the kernel code and my changes will
address address translation and type casting as are necessary for qemu.

I think that the big problem is that the current patches in my tree are
different than what Stuart originally sent and the initial three patches
that I sent I hadn't gone back to add the address translation to
{get,put}_user() (yes, it's my fault, I should have paid more
attention).  I've attached a newer patch which reflects what I'm doing
at the higher level (it still doesn't address the sparc64 code, though).

> Moreover, it may be questionnable to do the page right check in

s/right/write/; ?

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

You're right - I don't see that any of the archs use the access type
argument.

> > [...]
> 
> Fabrice.

06_efault.patch.1.3
Description: Text Data