[Qemu-devel] RFC: [0/11] EFAULT patch

[Qemu-devel] RFC: [0/11] EFAULT patch

[Stuart Anderson]

Following this message, are the 11 parts of the patch that implements
EFAULT detection in the linux-user mode. Hopefully, this reflects what
was discussed following the first RFC of this patch. Also, hopefully, it
is easier to digest in smaller pieces like this.

In short, the (un)lock_user*() interfaces have been replaced by access_ok
and copy_(to|from)_user() style interfaces. This code should now look
more like some of the 32_on_64 code in the Linux kernel.

As a side effect of these changes, and the more thorough testing that
went along with them, several other bugs have been fixed in areas such
as IPC and sockets.

As before, the Linux Test Project test suite was used in an armel on
x86_64 environment.

Your comments would be appreciated as I'd like to finish beating these
changes into shape so they can be accepted into the repository.


                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[J. Mayer]

On Tue, 2007-09-18 at 20:59 -0400, Stuart Anderson wrote:
> Following this message, are the 11 parts of the patch that implements
> EFAULT detection in the linux-user mode. Hopefully, this reflects what
> was discussed following the first RFC of this patch. Also, hopefully, it
> is easier to digest in smaller pieces like this.
> 
> In short, the (un)lock_user*() interfaces have been replaced by access_ok
> and copy_(to|from)_user() style interfaces. This code should now look
> more like some of the 32_on_64 code in the Linux kernel.
> 
> As a side effect of these changes, and the more thorough testing that
> went along with them, several other bugs have been fixed in areas such
> as IPC and sockets.
> 
> As before, the Linux Test Project test suite was used in an armel on
> x86_64 environment.
> 
> Your comments would be appreciated as I'd like to finish beating these
> changes into shape so they can be accepted into the repository.


The idea is great but there seem to be a problem in those patches:
you directly cast syscall arguments, which are (or should be)
target_ulong to pointers in the host environment. You should to use the
g2h / h2g macros to get the pointer in the host memory from the offset
in the target address space. Offset in the target address  space can not
be assumed to be the same size as an address in the host address space,
thus can never be casted directly to host pointer.
Then, the changes you've done, changing long arguments (which should be
target_long to be correct, you can take a look at the last patch I sent
on the list) to pointers, for example in function prototypes, are
incorrect.
This used to be handled by the lock_user functions and should be handled
in your patch too...

One thing I really dislike is multiple statements on the same line. I
know this is only cosmetics (and that coding style discussion usually
have no end), but code like:
if (xxxx) return -1;
can easily confuse any reader, imho, especially when the lines are long
then is to be avoided....

Regards.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[Stuart Anderson]

On Wed, 19 Sep 2007, J. Mayer wrote:

> The idea is great but there seem to be a problem in those patches:
> you directly cast syscall arguments, which are (or should be)
> target_ulong to pointers in the host environment. You should to use the
> g2h / h2g macros to get the pointer in the host memory from the offset
> in the target address space.

I was explicitly told to _not_ use these in this code.

> Offset in the target address  space can not
> be assumed to be the same size as an address in the host address space,

And in fact, they definitely are not the same in certain cobinations.

> thus can never be casted directly to host pointer.

At some point, we have to convert things from target_long to a host
pointer. If we can agree on what that right mechanism it to do this, I'd
be glad to use it. So far, everything I've seen involved some form of
type casting.


> This used to be handled by the lock_user functions and should be handled
> in your patch too...

It was handled by g2h() which was just a typecast hidden behind the
macro.


                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[Paul Brook]

On Wednesday 19 September 2007, Stuart Anderson wrote:
> On Wed, 19 Sep 2007, J. Mayer wrote:
> > The idea is great but there seem to be a problem in those patches:
> > you directly cast syscall arguments, which are (or should be)
> > target_ulong to pointers in the host environment. You should to use the
> > g2h / h2g macros to get the pointer in the host memory from the offset
> > in the target address space.
>
> I was explicitly told to _not_ use these in this code.

g2h and h2g assume a single linear mapping from host to guest address space. 
They are internal implementation details of the lock/copy routines, and 
should not be used elsewhere.


> > This used to be handled by the lock_user functions and should be handled
> > in your patch too...
>
> It was handled by g2h() which was just a typecast hidden behind the
> macro.

It also does linear address offsetting.

Having the macro is important. It means that the translation from guest to 
host address space happens in precisely one place. The current code has been 
fairly carefully audited to make sure everything[1] uses that one definition. 

Please don't break this property. When we do change to a differnt mapping 
system (eg. enabling softmmu) it's trivial to prove that we've made all the 
changes necessary.

Paul

[1] Except some of the mmap code, which would need rewriting anyway.

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[Stuart Anderson]

On Wed, 19 Sep 2007, J. Mayer wrote:

> Then, the changes you've done, changing long arguments (which should be
> target_long to be correct, you can take a look at the last patch I sent
> on the list) to pointers, for example in function prototypes, are
> incorrect.

I just went, and looked at the linux code again for 32 on 64 for x86_64 and
powerpc.  In both of these cases (and I suspect the others as well), the
parameters which are passed via registers are 0 extended from 32 bits to
64 bit in the syscall entry asm code. This way, once the C code is
called via the sys_call_table, everything is dealt with as 64 bits. This
actually keeps the rest of the code simpler as the rest of the kernel
doesn't have to be extending & truncating pointers everywhere else.

On x86_64 and powerpc, it appears that both user (ie target) and kernel
pointers co-exist and that the code that maps structures assume that the
__get_user()/__put_user() and copy_*_user() routines can handle any
special situation. The pointers passed into functions like
cp_compat_stat() are 64-bits for both the structure located in the
kernel, and the one located in user space.

My understanding is that we want to do as the kernel does as much as
possible. In light of this, wouldn't we want to be decreasing the use
of target_long where pointers may be involved instead of increasing it?


                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[J. Mayer]

On Wed, 2007-09-19 at 15:00 -0400, Stuart Anderson wrote:
> On Wed, 19 Sep 2007, J. Mayer wrote:
> 
> > Then, the changes you've done, changing long arguments (which should be
> > target_long to be correct, you can take a look at the last patch I sent
> > on the list) to pointers, for example in function prototypes, are
> > incorrect.
> 
> I just went, and looked at the linux code again for 32 on 64 for x86_64 and
> powerpc.  In both of these cases (and I suspect the others as well), the
> parameters which are passed via registers are 0 extended from 32 bits to
> 64 bit in the syscall entry asm code. This way, once the C code is
> called via the sys_call_table, everything is dealt with as 64 bits. This
> actually keeps the rest of the code simpler as the rest of the kernel
> doesn't have to be extending & truncating pointers everywhere else.

It's not surprising to me for PowerPC as PowerPC 32 is mostly defined as
the same architecture as PowerPC 64 with 32 bits addressing. Running in
32 bits mode on PowerPC mostly mean you have addresses masked for memory
accesses and different overflow and carry flags generations, but all
computations can still be done the same way as in 64 bits mode.
The situation may be different for x86 code running on x86_64, don't
have the knowledge to tell...

> On x86_64 and powerpc, it appears that both user (ie target) and kernel
> pointers co-exist and that the code that maps structures assume that the
> __get_user()/__put_user() and copy_*_user() routines can handle any
> special situation. The pointers passed into functions like
> cp_compat_stat() are 64-bits for both the structure located in the
> kernel, and the one located in user space.
> 
> My understanding is that we want to do as the kernel does as much as
> possible. In light of this, wouldn't we want to be decreasing the use
> of target_long where pointers may be involved instead of increasing it?

Well, we also have to take care of the reversed case: emulating 64 bits
targets on 32 bits hosts (which is exactly what I'm currently trying to
sanitize). Then, we need to keep argument size to always be >=
TARGET_LONG_BITS. That's the reason why it seems a good idea to me to
use target_long everywhere but at the point we would actually
dereference the pointer. If we want once to be able to fully emulate 64
bits targets on 32 bits hosts, we will have to deal with the fact the
target memory space may not fit in the host one, then deal with page
swapping. In those conditions, host pointers use would have to be
avoided as much as possible, imho.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[Paul Brook]

On Wednesday 19 September 2007, Stuart Anderson wrote:
> On Wed, 19 Sep 2007, J. Mayer wrote:
> > Then, the changes you've done, changing long arguments (which should be
> > target_long to be correct, you can take a look at the last patch I sent
> > on the list) to pointers, for example in function prototypes, are
> > incorrect.
>
> I just went, and looked at the linux code again for 32 on 64 for x86_64 and
> powerpc.  In both of these cases (and I suspect the others as well), the
> parameters which are passed via registers are 0 extended from 32 bits to
> 64 bit in the syscall entry asm code. This way, once the C code is
> called via the sys_call_table, everything is dealt with as 64 bits. This
> actually keeps the rest of the code simpler as the rest of the kernel
> doesn't have to be extending & truncating pointers everywhere else.
>
> On x86_64 and powerpc, it appears that both user (ie target) and kernel
> pointers co-exist and that the code that maps structures assume that the
> __get_user()/__put_user() and copy_*_user() routines can handle any
> special situation. The pointers passed into functions like
> cp_compat_stat() are 64-bits for both the structure located in the
> kernel, and the one located in user space.
>
> My understanding is that we want to do as the kernel does as much as
> possible. In light of this, wouldn't we want to be decreasing the use
> of target_long where pointers may be involved instead of increasing it?

No. We're doing more than most 32-64 syscall thunks. To a first approximation 
the syscall thunks can bindly zero extend all values. In qemu we need to know 
whether something is a pointer or a value.

Kernel and userspace addresses are not interchangeable in the kernel. Any 
place that does so is probably a bug.

Paul

Re: [Qemu-devel] RFC: [0/11] EFAULT patch

[Stuart Anderson]

On Wed, 19 Sep 2007, Paul Brook wrote:

> No. We're doing more than most 32-64 syscall thunks. To a first approximation
> the syscall thunks can bindly zero extend all values. In qemu we need to know
> whether something is a pointer or a value.

Isn't that was the code in do_syscall() does? or am I looking at something
in the wrong way?

> Kernel and userspace addresses are not interchangeable in the kernel. Any
> place that does so is probably a bug.

I said co-exist, not interchangeable. My point was that the 32-on-64
code didn't do any explicit mapping of pointers passed to it other than
the normal API.

I'm having trouble determining how you would like for things to be. Could
you maybe provide a small sample of how all of this should work, and then
I can probably see what I'm not quite getting.


                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149