Re: [Qemu-devel] [PATCH] linux-user EFAULT implementation

[Paul Brook <paul@codesourcery.com>]

On Tuesday 10 July 2007, Stuart Anderson wrote:
> On Mon, 9 Jul 2007, Fabrice Bellard wrote:
> > No. Ideally you should use the same conventions as the Linux kernel and
> > assume that you cannot access the user data directly.
>
> That's what I had already started doing today.
>
> > For the time being, I would suggest to minimize the number of changes and
> > just extend lock_user()/unlock_user() as you began to do to handle
> > -EFAULT. The rest is mostly a question of cosmetics.
>
> The attached patch is my in-progress work of the complete overhaul to use
> the kernel conventions. It needs some more work to finish the
> conversion, but enough should be done to see how it is going to turn
> out. Overall, I think the converted code is easier to read, especially
> if you are familiar with reading kernel code. I also think it will end
> up being more correct both becasue of the additional time now spent on
> reviewing each syscall, as well as the kernel conventions tend to make
> you be more thorough and explicite.

>  {
>      struct target_rusage *target_rusage;
>
> -    lock_user_struct(target_rusage, target_addr, 0);
> +    if( !access_ok(VERIFY_READ,target_addr,sizeof(*target_rusage)) )
> return -1;
> +    target_rusage = (struct target_rusage *)g2h(target_addr); 

Using g2h directly is bad. g2h is an implementation detail of one particular 
memory model.

The whole point of the lock_user abstraction (or a similar copy_from_user 
abstraction) is that almost none of the code cares how "user" memory is 
accessed. One of the long-term goals of this abstraction is to allow the 
softmmu code to be used with userspace emulation. In this case a region may 
be split across multiple discontiguous host pages.

The reason I used a locking paradigm rather than a copying one is that it 
allows a zero-copy implementation in the common case. I've no strong 
objections to a copying interface, however it must be implementation 
agnostic.

Paul