[Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[TJ]

I'm building on x86_64 GNU/Linux. There are *lots* of (1053) compiler
warnings of the class:

warning: cast to pointer from integer of different size

caused by the various conversions to/from host-to-target pointer and int
types.

On x86 the warnings mostly don't occur. An easy way to get an idea of
the scale of the build problems is to review the Ubuntu build logs for
x86, x86_64, powerpc, and solaris (It's a 0.9.0 build and the solaris
build fails, but the x86_64 build log is clear although the line numbers
don't match CVS).

Fixing it looks to require a variety of fixes, from simple explicit
casts in-line, to some complicated review of multiple levels of macros
to decide where best to apply a fix.

I am aiming to work through all 1053 errors to classify the reasons for
them, and then work up a solution for each class.

If you could review the details here and provide useful feedback and
ides I'd be grateful. I don't want to redo work others are already doing
or go against some implicit techniques hidden in the macro nesting in
particular.

This should be considered alongside Thayne Harbaugh's post "[RFC]
linux-user (mostly syscall.c)".

Thanks

TJ.

--- specimen details ---

The first warning is also one of the most complex to unravel. It occurs
in  linux-user/syscall.c in functions

target_to_host_cmsg()
host_to_target_cmsg()

It is caused by use of the macro TARGET_CMSG_FIRSTHDR (which is a
reworking of the linux macro CMSG_FIRSTHDR defined in
<sys/bits/socket.h>):

static inline void target_to_host_cmsg(struct msghdr *msgh,
                                       struct target_msghdr *target_msgh) 
{
   struct target_cmsghdr *target_cmsg = TARGET_CMSG_FIRSTHDR(target_msgh);

The macro is defined thus:

linux-user/syscall_defs.h

    #define TARGET_CMSG_FIRSTHDR(mhdr) \
     ((size_t) tswapl((mhdr)->msg_controllen) >= sizeof (struct  target_cmsghdr) \
     ? (struct target_cmsghdr *) tswapl((mhdr)->msg_control) : (struct target_cmsghdr *) NULL)

and the pre-processor generates:

 struct target_cmsghdr *target_cmsg = ((size_t) tswap32((target_msgh)->msg_controllen) >= sizeof (struct target_cmsghdr) ? (struct target_cmsghdr *) tswap32((target_msgh)->msg_control) : (struct target_cmsghdr *) ((void *)0));

The issue is in the 'true' action:

 (struct target_cmsghdr *) tswap32((target_msgh)->msg_control)

and the fact that the macro wasn't designed to handle different pointer
sizes. In this case assigning a 32-bit unsigned int to a 64-bit pointer.

The variables are defined as:

linux-user/syscall_defs.h
    struct target_msghdr { ...
     abi_long     msg_control;

linux-user/qemu.h
    #ifdef TARGET_ABI32
     typedef uint32_t abi_ulong;
     typedef int32_t abi_long;
     #define TARGET_ABI_BITS 32

and the 'tswap' function as:

cpu-all.h

static inline uint32_t tswap32(uint32_t s)
{
    return s;
}

So the compiler assigns the 32-bit value returned by tswap32() to the 64-bit pointer 'target_cmsg'.

For this case it appears the 'pure' solution would be to use additional
macros along the lines of:

struct target_cmsghdr *target_cmsg = TARGET_TO_HOST_PTR( TARGET_CMSG_FIRSTHDR(target_msgh));

and define new macros along the lines of

TARGET_TO_HOST_PTR
HOST_TO_TARGET_PTR

The practical solution is to promote the 32-bit unsigned integer to a
64-bit unsigned integer (using a cast) and then let the compiler do
implicit conversion to a 64-bit pointer.

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Paul Brook]

On Saturday 03 November 2007, TJ wrote:
> I'm building on x86_64 GNU/Linux. There are *lots* of (1053) compiler
> warnings of the class:
>
> warning: cast to pointer from integer of different size

There are at due to the recent EFAULT/access_ok changes. There should be (and 
used to be) a clear separation between host and target addresses. The EFAULT 
changes have broken this. Before these ghanges it wa trivial to remap the 
target address space (e.g. place it at a high address on a 64-bit host), or 
even enabling softmmu. I'm fairly sure that wouldn't work if you tried it 
now.

> Fixing it looks to require a variety of fixes, from simple explicit
> casts in-line, to some complicated review of multiple levels of macros
> to decide where best to apply a fix.

Adding a cast is never the right thing to do. There should *always* be a clear 
distinction and explicit conversion between host pointers and target 
addresses. It is never safe to cast from one to the other.

I put quite a lot of effort into getting this separation right when I 
implemented the lock_user interfaces. It was fairly trivial to implement 
address space translation (even softmmu) using this inerface. I'm quite 
annoyed that we seem to have regresses so badly in this area. AFAICS the 
whole of syscalls.c needs re-auditing to figure out which values are host 
pointers and which are target addresses.

We should have either lock_user or {get,put}_user, not both.

Paul

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Thayne Harbaugh]

On Sat, 2007-11-03 at 18:52 +0100, Paul Brook wrote:
> On Saturday 03 November 2007, TJ wrote:
> > I'm building on x86_64 GNU/Linux. There are *lots* of (1053) compiler
> > warnings of the class:
> >
> > warning: cast to pointer from integer of different size
> 
> There are at due to the recent EFAULT/access_ok changes. There should be (and 
> used to be) a clear separation between host and target addresses. The EFAULT 
> changes have broken this. Before these ghanges it wa trivial to remap the 
> target address space (e.g. place it at a high address on a 64-bit host), or 
> even enabling softmmu. I'm fairly sure that wouldn't work if you tried it 
> now.

No, the EFAULT/access_ok() didn't cause it exactly.  The problem is that
access_ok() came from kernel code and was dummied-out to always be true.
While it was turned off it was used with arguments that weren't
appropriate.  Turning on access_ok() caused all of the incorrect usages
to suddenly show up.  As an asside, the access_ok() is usually out of
order with the lock_user() calls - access_ok() should come first.

There were also additional incorrect pointer usages that have nothing to
do with EFAULT/access_ok() but usually don't get noticed when building
on 32bit arch.

> > Fixing it looks to require a variety of fixes, from simple explicit
> > casts in-line, to some complicated review of multiple levels of macros
> > to decide where best to apply a fix.
> 
> Adding a cast is never the right thing to do. There should *always* be a clear 
> distinction and explicit conversion between host pointers and target 
> addresses. It is never safe to cast from one to the other.

Agreed.

> I put quite a lot of effort into getting this separation right when I 
> implemented the lock_user interfaces. It was fairly trivial to implement 
> address space translation (even softmmu) using this inerface. I'm quite 
> annoyed that we seem to have regresses so badly in this area. AFAICS the 
> whole of syscalls.c needs re-auditing to figure out which values are host 
> pointers and which are target addresses.

I've actually done the audit and have all the fixes queued to submit to
the list.

> We should have either lock_user or {get,put}_user, not both.

Now that's a bit of a discussion.  It's possible to push everything into
{get,put}_user() but I don't think that's quite appropriate.  Let's look
at everything that's going on:

1) page in cache with proper permissions
2) address translation

"1" is performed by access_ok() which returns true/false.  "2" is
performed by lock_user() which internally uses g2h() to perform the
address translation and returns the translated address.  lock_user() can
also do memory replication/flushing to test that the memory calls are
coded correctly even when the implementation doesn't actually perform
address translation.

access_ok() and lock_user() perform essential functions.  lock_user(),
however, isn't directly comparable to how the kernel operates and should
therefore be encapsulated inside more typical kernel functions such as
{get,put}_user(), copy_{to,from}_user() and the like.  access_ok() and
lock_user() also have overhead and should therefore be used with the
largest memory hunks possible (e.g.: they should be used with an entire
structure - not with each individual data member of the structure).
That is why __{get,put}_user() exist: for copying the individual data
members of a structure once the *entire* structure has had access
checked and the address translation is performed.

The clean-ups that I am sending will follow the following guidelines:

* abi_long/abi_ulong will be passed as function arguments at the
high-level and all direct syscall wrappers, as well as do_*() functions
will only receive those types from user space as well as will only
return abi_long types.

* type casts will be removed.

* all target/host interactions will happen through {get,put}_user() and
copy_{to,from}_user() just like in the kernel.  These will accept a
target address in an abi_ulong, do access_ok(), do lock_user() and map
the target address to a host address and then perform the get/put or
copy_to/from with the appropriate unlock_user() afterwords.

  * {get,put}_user() will be used for individual data types that are
passed.

  * copy_{to,from}_user_<struct>() will be used for structures:

static inline abi_long copy_from_user_flock(struct flock *host_fl,
                                            abi_ulong target_fl_addr)
{
    struct target_flock *target_fl;

    if (!access_ok(VERIFY_READ, target_fl_addr, sizeof(*target_fl)))
        return -TARGET_EFAULT;

    lock_user_struct(target_fl, target_fl_addr, 1);

    __get_user(host_fl->l_type, &(target_fl->l_type));
    __get_user(host_fl->l_whence, &(target_fl->l_whence));
    __get_user(host_fl->l_start, &(target_fl->l_start));
    __get_user(host_fl->l_len, &(target_fl->l_len));
    __get_user(host_fl->l_pid, &(target_fl->l_pid));

    lock_user_struct(target_fl, target_fl_addr, 0);

    return 0;
}


This effectively eliminates all {lock,unlock}_user() calls and g2h/h2g()
calls from the main syscall wrappers and emulation - the
{lock,unlock}_user() calls are localized to wrappers that marshal
between target and user.

This is the way that my source tree is and it makes things very clean.
The code is much more comparable to kernel code, both access and locking
are managed and compiler warnings are almost non-existent (I still have
a few minor clean-ups to do).  I don't think there's an appropriate way
to eliminate either {lock,unlock}_user() or {get,put}_user() and keep
comparable coding semantics to the kernel.

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Paul Brook]

> access_ok() and lock_user() perform essential functions.  lock_user(),
> however, isn't directly comparable to how the kernel operates and should
> therefore be encapsulated inside more typical kernel functions such as
> {get,put}_user(), copy_{to,from}_user() and the like.  access_ok() and
> lock_user() also have overhead and should therefore be used with the
> largest memory hunks possible (e.g.: they should be used with an entire
> structure - not with each individual data member of the structure).
> That is why __{get,put}_user() exist: for copying the individual data
> members of a structure once the *entire* structure has had access
> checked and the address translation is performed.

>   I don't think there's an appropriate way
> to eliminate either {lock,unlock}_user() or {get,put}_user() and keep
> comparable coding semantics to the kernel.

Your argument seems inconsistent to me. The kernel doesn't have lock_user at 
all, so how can using it be consistent with kernel code?

There are two different strategies for accessing user data. Either:

- Use a copying interface. i.e. get_user (for single values) or  
copy_from_user (for blocks/structures).
- Use a locking interface. i.e. lock_user.

Personally I like the locking interface as it allows a zero-copy 
implementation. However the kernel uses a copying interface, and my 
understanding is that other qemu maintainers also prefer the copying 
interface.

You need to pick one interface (get/put/copy or lock) and stick to it. If 
get_user actually just does byteswapping of values in host memory then IMHO 
it really needs to be renamed (to tswap).

Part of the problem may be that linux assumes that both kernel and userspace 
pointers can be represented by the compiler. This allows it to do address 
arithmetic and take the address of members of pointers to userspace 
structures. qemu can not do this.

qemu also has to deal with endian conversion. The kernel does not. This means 
that some data the kernel may be able to copy/pass/access unmodified needs 
special treatment in qemu.

Paul

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Thayne Harbaugh]

Here's a better explanation as to why I initially mixed lock_user() and
copy_to_user():

On Tue, 2007-11-06 at 01:05 +0000, Paul Brook wrote:
> > access_ok() and lock_user() perform essential functions.  lock_user(),
> > however, isn't directly comparable to how the kernel operates and should
> > therefore be encapsulated inside more typical kernel functions such as
> > {get,put}_user(), copy_{to,from}_user() and the like.  access_ok() and
> > lock_user() also have overhead and should therefore be used with the
> > largest memory hunks possible (e.g.: they should be used with an entire
> > structure - not with each individual data member of the structure).
> > That is why __{get,put}_user() exist: for copying the individual data
> > members of a structure once the *entire* structure has had access
> > checked and the address translation is performed.
> 
> >   I don't think there's an appropriate way
> > to eliminate either {lock,unlock}_user() or {get,put}_user() and keep
> > comparable coding semantics to the kernel.
> 
> Your argument seems inconsistent to me. The kernel doesn't have lock_user at 
> all, so how can using it be consistent with kernel code?

See your comment below about how qemu differs from the kernel.

> There are two different strategies for accessing user data. Either:
> 
> - Use a copying interface. i.e. get_user (for single values) or  
> copy_from_user (for blocks/structures).
> - Use a locking interface. i.e. lock_user.
> 
> Personally I like the locking interface as it allows a zero-copy 
> implementation. However the kernel uses a copying interface, and my 
> understanding is that other qemu maintainers also prefer the copying 
> interface.

The "zero-copy" nature of lock_user() is why I mixed the two.
lock_user() was pushed down inside of wrapper functions.  External to
the wrapper functions the code was very comparable to the kernel code.
It was the best of both worlds.

> Part of the problem may be that linux assumes that both kernel and userspace 
> pointers can be represented by the compiler. This allows it to do address 
> arithmetic and take the address of members of pointers to userspace 
> structures. qemu can not do this.

It is precisely this difference that I felt that a minor deviation was
acceptable: mixing lock_user() with copy_to_user().  My solution had the
zero-copy capability while making high-level syscall wrapper code very
comparable to kernel code.

In the end I'd just rather code it your way and move on then argue why I
think my way is better - just keep in mind that there were intelligent
decisions behind why I did it the way I did it - it wasn't haphazard
(although I did send a few half-baked patches that weren't correct to
the list - I can be a bonehead in other ways 8-)).

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Fabrice Bellard]

Paul Brook wrote:
> [...]
> Personally I like the locking interface as it allows a zero-copy 
> implementation. However the kernel uses a copying interface, and my 
> understanding is that other qemu maintainers also prefer the copying 
> interface.

At least I don't think it is critical performance wise, especially if
the page flag checks are added ! Before you added the current zero copy
interface, my plan was to use a zero copy interface just for big buffers
such as the one for read/write.

Another point is that the code from signal.c is not converted to the
zero-copy interface and it is a significant source of complexity as
there is a large amount of target specific code copied from the Linux
kernel to generate the stack frame.

Regards,

Fabrice.

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Stuart Anderson]

On Tue, 6 Nov 2007, Fabrice Bellard wrote:

> Paul Brook wrote:
>> [...]
>> Personally I like the locking interface as it allows a zero-copy
>> implementation. However the kernel uses a copying interface, and my
>> understanding is that other qemu maintainers also prefer the copying
>> interface.
>
> At least I don't think it is critical performance wise, especially if
> the page flag checks are added ! Before you added the current zero copy
> interface, my plan was to use a zero copy interface just for big buffers
> such as the one for read/write.

By the time you consider the different combinations of targets & hosts,
most of the opportunities for zero copy are eliminated anyway. Byte
ordering and structure packing amd content differences mean that we can't
do zero-copy except in the rare circumstance that the host & target
match is all of these respects. The read & write buffers would still
benefit from zero copy, but nearly everything else has to be touched
anyway.


                                 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: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Paul Brook]

> By the time you consider the different combinations of targets & hosts,
> most of the opportunities for zero copy are eliminated anyway. Byte
> ordering and structure packing amd content differences mean that we can't
> do zero-copy except in the rare circumstance that the host & target
> match is all of these respects. The read & write buffers would still
> benefit from zero copy, but nearly everything else has to be touched
> anyway.

If you're not careful you get double-copying. Once copying the struct from 
guest to host space, and then again when converting layout/endianess.

I've no idea whether this overhead is important in practice though.

Paul

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Stuart Anderson]

On Tue, 6 Nov 2007, Paul Brook wrote:

> If you're not careful you get double-copying. Once copying the struct from
> guest to host space, and then again when converting layout/endianess.

Yes, it would be easy to do that by mistake. The approach that has been
taken has been to use typed copy_*_user_<type>() routines for the structs
instead of using a seperate untyped copy_*_user() followed by a
convert_<type>() routine. The copy_*_user_<type>() routines do the copy
and convert in a single step.

This will never be as fast as an optimized buffer copy, but it will also
not be as slow as having seperate routines.



                                 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: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Fabrice Bellard]

Hi,

Regarding the user memory access, here is my suggestion which should
minimize the changes:

- Keep __put_user() and __get_user() as you did.

- Remove put_user(), get_user(), copy_from_user() and copy_to_user()

- Modify the signal.c code so that it uses __put_user, __get_user and
lock/unlock_user.

- Modify lock_user() so that it automatically does access_ok() and
returns NULL if access_ok() fails.

- Test lock_user/lock_user_struct/... return value explicitely at every
call.

- Fix page_check_range() so that it handles writes to pages containing
code by calling page_unprotect when necessary (the current code can fail
in this case !).

- Suppress no longer needed page_unprotect_range() call in syscall.c.

- Suppress or fix tput/tget macros so that they do access_ok().

Regards,

Fabrice.

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Thayne Harbaugh]

On Wed, 2007-11-07 at 20:18 +0100, Fabrice Bellard wrote:
> Hi,
> 
> Regarding the user memory access, here is my suggestion which should
> minimize the changes:

The virtue of making the minimum changes is that there are likely fewer
errors.  Other than that, it's more important to me to make the
*changes*.

> - Keep __put_user() and __get_user() as you did.

good.

> - Remove put_user(), get_user(), copy_from_user() and copy_to_user()

I'd prefer to keep put_user(), get_user(), copy_from_user() and
copy_to_user() and have them internally perform
lock_user()/unlock_user().  I like how lock_user()/unlock_user()
minimizes copying - I think that's good.  I also like keeping functions
that work in similar ways as the kernel counterparts - this is important
for maintaining functions that emulate kernel behavior so that code is
more comparable to the kernel.  This is also very near the way my
current patches are written and is the minimum work for me.

> - Modify the signal.c code so that it uses __put_user, __get_user and
> lock/unlock_user.

good.

> - Modify lock_user() so that it automatically does access_ok() and
> returns NULL if access_ok() fails.

Yes - this is good.  This is something that I missed with my first set
of patches and it's significant.

> - Test lock_user/lock_user_struct/... return value explicitely at every
> call.

yep.

> - Fix page_check_range() so that it handles writes to pages containing
> code by calling page_unprotect when necessary (the current code can fail
> in this case !).

Good.

> - Suppress no longer needed page_unprotect_range() call in syscall.c.

sure.

> - Suppress or fix tput/tget macros so that they do access_ok().

I think it's better to use get_user()/put_user() for these which would
handle assigning to different sized types and with proper sign-extension
when the target and host aren't the same sized word.

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Paul Brook]

> - Modify lock_user() so that it automatically does access_ok() and
> returns NULL if access_ok() fails.

You'll also need to augment all lock_user calls to indicate whether the buffer 
needs to be writable. Currently this information is not available until 
unlock_user is called.

Paul

Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]


On Wed, 2007-11-07 at 20:18 +0100, Fabrice Bellard wrote:

> - Fix page_check_range() so that it handles writes to pages containing
> code by calling page_unprotect when necessary (the current code can fail
> in this case !).
> 
> - Suppress no longer needed page_unprotect_range() call in syscall.c.

Something like this?

Looks like exec.c needs some attention regarding target_ulong/abi_ulong.

[-- Attachment #2: 06_efault.patch.1.4 --]
[-- Type: text/x-patch, Size: 1919 bytes --]

Index: qemu/linux-user/syscall.c
===================================================================
--- qemu.orig/linux-user/syscall.c	2007-11-12 09:56:01.000000000 -0700
+++ qemu/linux-user/syscall.c	2007-11-12 09:56:12.000000000 -0700
@@ -2745,7 +2745,6 @@
         ret = 0; /* avoid warning */
         break;
     case TARGET_NR_read:
-        page_unprotect_range(arg2, arg3);
         if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
             goto efault;
         ret = get_errno(read(arg1, p, arg3));
@@ -4538,7 +4537,6 @@
         break;
 #ifdef TARGET_NR_pread
     case TARGET_NR_pread:
-        page_unprotect_range(arg2, arg3);
         if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
             goto efault;
         ret = get_errno(pread(arg1, p, arg3, arg4));
Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c	2007-11-12 09:56:01.000000000 -0700
+++ qemu/exec.c	2007-11-12 10:00:41.000000000 -0700
@@ -1898,6 +1898,9 @@
             return -1;
         if (!(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE) )
             return -1;
+        if ((p->flags & PAGE_EXEC) && (flags & PAGE_WRITE)
+            && page_unprotect(addr, 0, NULL))
+            return -1;
     }
     return 0;
 }
@@ -1942,21 +1945,6 @@
     return 0;
 }
 
-/* call this function when system calls directly modify a memory area */
-/* ??? This should be redundant now we have lock_user.  */
-void page_unprotect_range(target_ulong data, target_ulong data_size)
-{
-    target_ulong start, end, addr;
-
-    start = data;
-    end = start + data_size;
-    start &= TARGET_PAGE_MASK;
-    end = TARGET_PAGE_ALIGN(end);
-    for(addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
-        page_unprotect(addr, 0, NULL);
-    }
-}
-
 static inline void tlb_set_dirty(CPUState *env,
                                  unsigned long addr, target_ulong vaddr)
 {