Re: [Qemu-devel] [PATCH 00/13] __{get, put}_user return value cleanup

[Peter Maydell]

On 23 April 2014 14:11,  <address@hidden> wrote:
> From: Riku Voipio <address@hidden>
>
> This series is primarily motivated to have a gcc-4.9 buildfix:
>
> linux-user/syscall.c: In function ‘host_to_target_stat64’:
> linux-user/qemu.h:301:19: error: right-hand operand of comma expression has 
> no effect [-Werror=unused-value]
>       ((hptr), (x)), 0)
>
> removing the unused 0 moves the bar:
>
> linux-user/main.c: In function ‘arm_kernel_cmpxchg64_helper’:
> linux-user/qemu.h:330:15: error: void value not ignored as it ought to be
>          __ret = __put_user((x), __hptr);    \
>
> And after fixing that, we see there is a lot of reading the return
> value of __put_user and __get_user in signal.c - apparently without
> much of consistency as other functions do check and others don't...
>
> I'm not 100% sure that simply removing the checks is right way
> and signal.c is quite a mess, so push this set early for comments
> before spending more time on this approach.

So the reason for this inconsistency is that QEMU's approach
to checking that memory is accessible differs from the Linux kernel.
In Linux, the check effectively happens on each get/put access
(for free courtesy of the MMU), so the __get/__put_user accessors
can each individually succeed or fail, and the return value must
always be checked.
In QEMU, we do an access check for an entire region via
lock_user (or lock_user_struct) and catch the failure case there.
So our __get/__put_user accessors can never fail. However we
retained the return value because a lot of the code in signal.c
is pretty much copy-and-pasted from the kernel sources with
minor tweaks, and so it includes the "gather up and check
return values at each step" code.
Generally in code review or if I've been tidying up or fixing the
signal handling code for some other reason I've removed the
pointless accumulation of return values. But a bunch of our
less-maintained architectures still have it.

>   signal.c: remove __get/__put_user return value reading
>   signal.c setup_frame/x86: __put_user cleanup
>   signal.c: remove return value from copy_siginfo_to_user
>   signal.c: remove return value from setup_sigcontext
>   signal.c: remove return value from restore_sigcontext
>   RFC comment out restore_fpu_state (sparc)
>   do_sigaltstack: remove __get_user value check
>   do_sigreturn - remove __get_user checks
>   signal.c: setup_frame remove __put_user checks
>   remove __put/get error checks from ppc {save,restore}_user_regs
>   sparc64_set_context: remove __get_user checks
>   remove __get_user return check from PPC do_setcontext
>   fix gcc-4.9 compiler error on __{get,put]}_user

Given that signal.c is a mix of code for all our supported
architectures, it would be nice if these patch summary lines
were a bit more consistent:
 linux-user/signal.c: Generic change affecting all archs
 linux-user/signal.c: PPC: Change affecting only PPC

and so on.

I should probably have another go at a tidyup I was
looking at ages ago which splits linux-user/signal.c
into linux-user/$arch/signal.c...

thanks
-- PMM