Messages in this thread | | | Subject | Re: [PATCH 3/4] Consolidate sys32_select | From | Martin Josefsson <> | Date | Wed, 26 May 2004 00:02:13 +0200 |
| |
On Fri, 2004-04-16 at 18:05, Arnd Bergmann wrote:
Hi Arnd.
> sys32_select has seven mostly but not exactly identical versions, > so consolidate them as compat_sys_select. Based on the ppc64 > implementation, which most closely resembles sys_select. > One bug that was not caught by LTP has been fixed since the > first version of this patch. > > tested x86_64, ia64 and s390.
This breaks sparc64. rsync -> select() -> compat_sys_select() with a user pointer that has bit (1<<32) set. I came to this conclusion after having added lots of debug printk's. Here's the debug printk:
compat_sys_select: before verify_area 00000001efff6468
The address printed is the address of the tvp argument. The only weird thing with it is the (1<<32) bit, otherwise it looks like the other pointers (this doesn't happen for all select()'s but I have a testcase with rsync that reproduces it every time in exactly the same way)
When this happens __get_user() generates a pagefault and we somehow get stuck in a pagefault loop, we livelock. (verify_area() is a noop on sparc64)
How it used to look in arch/sparc64/kernel/sys_sparc32.c
> -asmlinkage int sys32_select(int n, u32 *inp, u32 *outp, u32 *exp, u32 tvp_x) > -{ > - fd_set_bits fds; > - struct compat_timeval *tvp = (struct compat_timeval *)AA(tvp_x); [snip] > - if ((ret = verify_area(VERIFY_READ, tvp, sizeof(*tvp))) > - || (ret = __get_user(sec, &tvp->tv_sec)) > - || (ret = __get_user(usec, &tvp->tv_usec))) > - goto out_nofds;
What it looks like now:
asmlinkage long compat_sys_select(int n, compat_ulong_t __user *inp, compat_ulong_t __user *outp, compat_ulong_t __user *exp, struct compat_timeval __user *tvp) { [snip] time_t sec, usec;
if ((ret = verify_area(VERIFY_READ, tvp, sizeof(*tvp))) || (ret = __get_user(sec, &tvp->tv_sec)) || (ret = __get_user(usec, &tvp->tv_usec))) goto out_nofds;
This combined with this comment from arch/sparc64/kernel/sys_sparc32.c
/* Things to consider: the low-level assembly stub does srl x, 0, x for first four arguments, so if you have pointer to something in the first four arguments, just declare it as a pointer, not u32. On the other side, arguments from 5th onwards should be declared as u32 for pointers, and need AA() around each usage.
I added some simple casting to get rid of the high-bit and now rsync works just fine and we don't livelock.
Do you have any ideas how to cleanly get this working again? Masking argument five and onwards with 0xffffffffUL ?
On a related note, a quick look through fs/compat.c indicates that we have the same problem with these compat functions as well:
compat_sys_io_getevent compat_sys_mount
-- /Martin [unhandled content-type:application/pgp-signature] | |