* waitid fallout
@ 2004-09-15 3:39 Richard Henderson
2004-09-15 4:37 ` Stephen Rothwell
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Richard Henderson @ 2004-09-15 3:39 UTC (permalink / raw)
To: roland, linux-arch
Is an *entire* struct rusage required in siginfo_t for sigchld?
It's much bigger than the 24 (!) words of padding we had reserved
for expansion in siginfo_t.
Which means that the size of siginfo_t changed, which means that
any existing "sigwaitinfo ({ SIGCHLD }, &info)" will now corrupt
user memory. Perhaps we think no one does this, but we should
at least be aware of the ABI change.
Alpha is not yet using a VDSO for the signal frame unwind info, so
I noticed that all of my signal cancelation tests are now failing,
because the layout of the signal frame changed. Perhaps I'll take
this opportunity to update, but it does require a rather coordinated
kernel + libc upgrade in order to keep things working smoothly...
r~
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: waitid fallout 2004-09-15 3:39 waitid fallout Richard Henderson @ 2004-09-15 4:37 ` Stephen Rothwell 2004-09-15 5:34 ` Roland McGrath 2004-09-15 4:49 ` Richard Henderson ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Stephen Rothwell @ 2004-09-15 4:37 UTC (permalink / raw) To: Richard Henderson; +Cc: roland, linux-arch [-- Attachment #1: Type: text/plain, Size: 1169 bytes --] On Tue, 14 Sep 2004 20:39:03 -0700 Richard Henderson <rth@twiddle.net> wrote: > > Is an *entire* struct rusage required in siginfo_t for sigchld? > > It's much bigger than the 24 (!) words of padding we had reserved > for expansion in siginfo_t. On 64bit archs, at least. This means that we break the ABI ... > Which means that the size of siginfo_t changed, which means that > any existing "sigwaitinfo ({ SIGCHLD }, &info)" will now corrupt > user memory. Perhaps we think no one does this, but we should > at least be aware of the ABI change. > > Alpha is not yet using a VDSO for the signal frame unwind info, so > I noticed that all of my signal cancelation tests are now failing, > because the layout of the signal frame changed. Perhaps I'll take > this opportunity to update, but it does require a rather coordinated > kernel + libc upgrade in order to keep things working smoothly... We cannot do this even with coordination, as some of the 64bit archs ABIs have been Standardised in the LSB. Unless, of course, we revise the ABI Specs as well ... -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-15 4:37 ` Stephen Rothwell @ 2004-09-15 5:34 ` Roland McGrath 2004-09-15 6:19 ` Richard Henderson 2004-09-18 13:03 ` Arnd Bergmann 0 siblings, 2 replies; 10+ messages in thread From: Roland McGrath @ 2004-09-15 5:34 UTC (permalink / raw) To: Stephen Rothwell; +Cc: Richard Henderson, linux-arch > On Tue, 14 Sep 2004 20:39:03 -0700 Richard Henderson <rth@twiddle.net> wrote: > > > > Is an *entire* struct rusage required in siginfo_t for sigchld? > > > > It's much bigger than the 24 (!) words of padding we had reserved > > for expansion in siginfo_t. Ah, I only ever looked at the sizes on i386, where there was plenty of room. As you can see in my posts of the waitid patches, the si_rusage addition is not required by POSIX. I chose to add it so that the waitid call would subsume all functionality of wait4 et al and suffice as the sole call in the wait family that a new architecture should be obliged to provide in its system call table. If we were to take it back out, then it would make more sense to have the system call be like SGI's waitsys that takes the waitid args plus a struct rusage *. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-15 5:34 ` Roland McGrath @ 2004-09-15 6:19 ` Richard Henderson 2004-09-18 13:03 ` Arnd Bergmann 1 sibling, 0 replies; 10+ messages in thread From: Richard Henderson @ 2004-09-15 6:19 UTC (permalink / raw) To: Roland McGrath; +Cc: Stephen Rothwell, linux-arch On Tue, Sep 14, 2004 at 10:34:40PM -0700, Roland McGrath wrote: > Ah, I only ever looked at the sizes on i386, where there was plenty of > room. As you can see in my posts of the waitid patches, the si_rusage > addition is not required by POSIX. I eventually found them (I heart google). This idea makes a lot of sense, particularly if rusage had fit everywhere. > If we were to take it back out, then it would make more sense to have > the system call be like SGI's waitsys that takes the waitid args plus > a struct rusage *. This solution would make me happy. r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-15 5:34 ` Roland McGrath 2004-09-15 6:19 ` Richard Henderson @ 2004-09-18 13:03 ` Arnd Bergmann 2004-09-20 21:26 ` Roland McGrath 2004-09-20 21:27 ` Roland McGrath 1 sibling, 2 replies; 10+ messages in thread From: Arnd Bergmann @ 2004-09-18 13:03 UTC (permalink / raw) To: Roland McGrath; +Cc: Stephen Rothwell, Richard Henderson, linux-arch [-- Attachment #1: Type: text/plain, Size: 595 bytes --] On Mittwoch, 15. September 2004 07:34, Roland McGrath wrote: > If we were to take it back out, then it > would make more sense to have the system call be like SGI's waitsys that > takes the waitid args plus a struct rusage *. I just noticed it's changed in BK now. However, the respective changes in the compat_sys_waitid() code and include/linux/syscalls.h are still missing. Also, why did you put the compat code in arch/* and not in kernel/compat.c? Converting siginfo_t should probably remain architecture specific, but the rest should be the same everywhere. Arnd <>< [-- Attachment #2: signature --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-18 13:03 ` Arnd Bergmann @ 2004-09-20 21:26 ` Roland McGrath 2004-09-20 21:27 ` Roland McGrath 1 sibling, 0 replies; 10+ messages in thread From: Roland McGrath @ 2004-09-20 21:26 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton, Arnd Bergmann Cc: Linux Kernel Mailing List, Stephen Rothwell, Richard Henderson, linux-arch > I just noticed it's changed in BK now. However, the respective > changes in the compat_sys_waitid() code and include/linux/syscalls.h > are still missing. Sorry for that oversight. This patch updates the x86-64's compat code to handle the new argument to waitid. Index: linux-2.6/include/linux/syscalls.h =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/include/linux/syscalls.h,v retrieving revision 1.12 diff -B -b -p -u -r1.12 syscalls.h --- linux-2.6/include/linux/syscalls.h 31 Aug 2004 17:35:25 -0000 1.12 +++ linux-2.6/include/linux/syscalls.h 20 Sep 2004 19:55:29 -0000 @@ -163,7 +163,8 @@ asmlinkage void sys_exit_group(int error asmlinkage long sys_wait4(pid_t pid, unsigned int __user *stat_addr, int options, struct rusage __user *ru); asmlinkage long sys_waitid(int which, pid_t pid, - struct siginfo __user *infop, int options); + struct siginfo __user *infop, + int options, struct rusage __user *ru); asmlinkage long sys_waitpid(pid_t pid, unsigned int __user *stat_addr, int options); asmlinkage long sys_set_tid_address(int __user *tidptr); asmlinkage long sys_futex(u32 __user *uaddr, int op, int val, Index: linux-2.6/arch/x86_64/ia32/sys_ia32.c =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/arch/x86_64/ia32/sys_ia32.c,v retrieving revision 1.69 diff -B -b -p -u -r1.69 sys_ia32.c --- linux-2.6/arch/x86_64/ia32/sys_ia32.c 31 Aug 2004 17:35:25 -0000 1.69 +++ linux-2.6/arch/x86_64/ia32/sys_ia32.c 20 Sep 2004 20:01:06 -0000 @@ -1152,19 +1152,26 @@ asmlinkage long sys32_clone(unsigned int } asmlinkage long sys32_waitid(int which, compat_pid_t pid, - siginfo_t32 __user *uinfo, int options) + siginfo_t32 __user *uinfo, int options, + struct compat_rusage __user *uru) { siginfo_t info; + struct rusage ru; long ret; mm_segment_t old_fs = get_fs(); info.si_signo = 0; set_fs (KERNEL_DS); - ret = sys_waitid(which, pid, (siginfo_t __user *) &info, options); + ret = sys_waitid(which, pid, (siginfo_t __user *) &info, options, + uru ? &ru : NULL); set_fs (old_fs); if (ret < 0 || info.si_signo == 0) return ret; + + if (uru && (ret = put_compat_rusage(&ru, uru))) + return ret; + BUG_ON(info.si_code & __SI_MASK); info.si_code |= __SI_CHLD; return ia32_copy_siginfo_to_user(uinfo, &info); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-18 13:03 ` Arnd Bergmann 2004-09-20 21:26 ` Roland McGrath @ 2004-09-20 21:27 ` Roland McGrath 1 sibling, 0 replies; 10+ messages in thread From: Roland McGrath @ 2004-09-20 21:27 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Stephen Rothwell, Richard Henderson, linux-arch > Also, why did you put the compat code in arch/* and not in > kernel/compat.c? Converting siginfo_t should probably remain > architecture specific, but the rest should be the same everywhere. There is no existing shared compat code handling siginfo_t at all. I followed the model of existing interfaces that use it like sys32_rt_sigtimedwait. I'm not sure that all arch's provide the 32-bit siginfo_t type or call it the same thing. I haven't seen other such cases where the generic kernel/compat.c code refers to types defined in arch-specific code. Thanks, Roland ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-15 3:39 waitid fallout Richard Henderson 2004-09-15 4:37 ` Stephen Rothwell @ 2004-09-15 4:49 ` Richard Henderson 2004-09-15 6:23 ` Andi Kleen 2004-09-16 1:30 ` [PATCH] back out siginfo_t.si_rusage from waitid changes Roland McGrath 3 siblings, 0 replies; 10+ messages in thread From: Richard Henderson @ 2004-09-15 4:49 UTC (permalink / raw) To: roland, linux-arch On Tue, Sep 14, 2004 at 08:39:03PM -0700, Richard Henderson wrote: > Is an *entire* struct rusage required in siginfo_t for sigchld? FWIW, I don't see anything in http://www.opengroup.org/onlinepubs/009695399/functions/waitid.html http://www.opengroup.org/onlinepubs/009695399/basedefs/signal.h.html that mentions rusage at all. From whence does this feature arise? r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: waitid fallout 2004-09-15 3:39 waitid fallout Richard Henderson 2004-09-15 4:37 ` Stephen Rothwell 2004-09-15 4:49 ` Richard Henderson @ 2004-09-15 6:23 ` Andi Kleen 2004-09-16 1:30 ` [PATCH] back out siginfo_t.si_rusage from waitid changes Roland McGrath 3 siblings, 0 replies; 10+ messages in thread From: Andi Kleen @ 2004-09-15 6:23 UTC (permalink / raw) To: Richard Henderson; +Cc: roland, linux-arch On Tue, Sep 14, 2004 at 08:39:03PM -0700, Richard Henderson wrote: > Is an *entire* struct rusage required in siginfo_t for sigchld? > > It's much bigger than the 24 (!) words of padding we had reserved > for expansion in siginfo_t. > > Which means that the size of siginfo_t changed, which means that > any existing "sigwaitinfo ({ SIGCHLD }, &info)" will now corrupt > user memory. Perhaps we think no one does this, but we should > at least be aware of the ABI change. Oops. I think this broke x86-64 too. I don't want to change the signal ABI at all there and would prefer if this change was reverted. Thanks, -Andi ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] back out siginfo_t.si_rusage from waitid changes 2004-09-15 3:39 waitid fallout Richard Henderson ` (2 preceding siblings ...) 2004-09-15 6:23 ` Andi Kleen @ 2004-09-16 1:30 ` Roland McGrath 3 siblings, 0 replies; 10+ messages in thread From: Roland McGrath @ 2004-09-16 1:30 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton; +Cc: Linux Kernel Mailing List, linux-arch As I explained in the waitid patches, I added the si_rusage field to siginfo_t with the idea of having the siginfo_t waitid fills in contain all the information that wait4 or any such call could ever tell you. Nowhere in POSIX nor anywhere else specifies this field in siginfo_t. When Ulrich and I hashed out the system call interface we wanted, we looked at siginfo_t and decided there was plenty of space to throw in si_rusage. Well, it turns out we didn't check the 64-bit platforms. There struct rusage is ridiculously large (lots of longs for things that are never in a million years going to hit 2^32), and my changes bumped up the size of siginfo_t. Changing that size is more trouble than it's worth. This patch reverts the changes to the siginfo_t structure types, and no longer provides the rusage details in SIGCHLD signal data. Instead, I added a fifth argument to the waitid system call to fill in rusage. waitid is the name of the POSIX function with four arguments. It might make sense to rename the system call `waitsys' to follow SGI's system call with the same arguments, or `wait5' in the mindless tradition. But, feh. I just added the argument to sys_waitid, rather than worrying about changing the name in all the tables (and choosing a new stupid name). Thanks, Roland Signed-off-by: Roland McGrath <roland@redhat.com> Index: linux-2.6/arch/sparc64/kernel/signal32.c =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/arch/sparc64/kernel/signal32.c,v retrieving revision 1.42 diff -B -b -p -u -r1.42 signal32.c --- linux-2.6/arch/sparc64/kernel/signal32.c 2 Sep 2004 08:19:11 -0000 1.42 +++ linux-2.6/arch/sparc64/kernel/signal32.c 15 Sep 2004 18:38:55 -0000 @@ -122,7 +122,6 @@ struct siginfo32 { int _status; /* exit code */ compat_clock_t _utime; compat_clock_t _stime; - struct compat_rusage _rusage; } _sigchld; /* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGEMT */ @@ -188,8 +187,6 @@ int copy_siginfo_to_user32(struct siginf err |= __put_user(from->si_utime, &to->si_utime); err |= __put_user(from->si_stime, &to->si_stime); err |= __put_user(from->si_status, &to->si_status); - err |= put_compat_rusage(&from->si_rusage, - &to->si_rusage); default: err |= __put_user(from->si_pid, &to->si_pid); err |= __put_user(from->si_uid, &to->si_uid); Index: linux-2.6/arch/x86_64/ia32/ia32_signal.c =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/arch/x86_64/ia32/ia32_signal.c,v retrieving revision 1.32 diff -B -b -p -u -r1.32 ia32_signal.c --- linux-2.6/arch/x86_64/ia32/ia32_signal.c 11 Sep 2004 06:20:19 -0000 1.32 +++ linux-2.6/arch/x86_64/ia32/ia32_signal.c 15 Sep 2004 08:46:29 -0000 @@ -74,8 +74,6 @@ int ia32_copy_siginfo_to_user(siginfo_t3 err |= __put_user(from->si_utime, &to->si_utime); err |= __put_user(from->si_stime, &to->si_stime); err |= __put_user(from->si_status, &to->si_status); - err |= put_compat_rusage(&from->si_rusage, - &to->si_rusage); default: case __SI_KILL >> 16: err |= __put_user(from->si_uid, &to->si_uid); Index: linux-2.6/include/asm-generic/siginfo.h =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/include/asm-generic/siginfo.h,v retrieving revision 1.17 diff -B -b -p -u -r1.17 siginfo.h --- linux-2.6/include/asm-generic/siginfo.h 31 Aug 2004 17:35:25 -0000 1.17 +++ linux-2.6/include/asm-generic/siginfo.h 15 Sep 2004 08:26:58 -0000 @@ -75,7 +75,6 @@ typedef struct siginfo { int _status; /* exit code */ clock_t _utime; clock_t _stime; - struct rusage _rusage; } _sigchld; /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */ @@ -107,7 +106,6 @@ typedef struct siginfo { #define si_status _sifields._sigchld._status #define si_utime _sifields._sigchld._utime #define si_stime _sifields._sigchld._stime -#define si_rusage _sifields._sigchld._rusage #define si_value _sifields._rt._sigval #define si_int _sifields._rt._sigval.sival_int #define si_ptr _sifields._rt._sigval.sival_ptr Index: linux-2.6/include/asm-ia64/siginfo.h =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/include/asm-ia64/siginfo.h,v retrieving revision 1.18 diff -B -b -p -u -r1.18 siginfo.h --- linux-2.6/include/asm-ia64/siginfo.h 31 Aug 2004 17:35:25 -0000 1.18 +++ linux-2.6/include/asm-ia64/siginfo.h 15 Sep 2004 18:38:30 -0000 @@ -56,7 +56,6 @@ typedef struct siginfo { int _status; /* exit code */ clock_t _utime; clock_t _stime; - struct rusage _rusage; } _sigchld; /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */ Index: linux-2.6/include/asm-x86_64/ia32.h =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/include/asm-x86_64/ia32.h,v retrieving revision 1.20 diff -B -b -p -u -r1.20 ia32.h --- linux-2.6/include/asm-x86_64/ia32.h 31 Aug 2004 17:35:25 -0000 1.20 +++ linux-2.6/include/asm-x86_64/ia32.h 15 Sep 2004 18:38:24 -0000 @@ -115,7 +115,6 @@ typedef struct siginfo32 { int _status; /* exit code */ compat_clock_t _utime; compat_clock_t _stime; - struct compat_rusage _rusage; } _sigchld; /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */ Index: linux-2.6/kernel/exit.c =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/kernel/exit.c,v retrieving revision 1.152 diff -B -b -p -u -r1.152 exit.c --- linux-2.6/kernel/exit.c 8 Sep 2004 14:48:58 -0000 1.152 +++ linux-2.6/kernel/exit.c 15 Sep 2004 08:25:50 -0000 @@ -949,9 +949,10 @@ static int eligible_child(pid_t pid, int static int wait_noreap_copyout(task_t *p, pid_t pid, uid_t uid, int why, int status, - struct siginfo __user *infop) + struct siginfo __user *infop, + struct rusage __user *rusagep) { - int retval = getrusage(p, RUSAGE_BOTH, &infop->si_rusage); + int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0; put_task_struct(p); if (!retval) retval = put_user(SIGCHLD, &infop->si_signo); @@ -1003,7 +1004,8 @@ static int wait_task_zombie(task_t *p, i why = (exit_code & 0x80) ? CLD_DUMPED : CLD_KILLED; status = exit_code & 0x7f; } - return wait_noreap_copyout(p, pid, uid, why, status, infop); + return wait_noreap_copyout(p, pid, uid, why, + status, infop, ru); } /* @@ -1161,7 +1163,7 @@ static int wait_task_stopped(task_t *p, goto bail_ref; return wait_noreap_copyout(p, pid, uid, why, (exit_code << 8) | 0x7f, - infop); + infop, ru); } write_lock_irq(&tasklist_lock); @@ -1304,7 +1306,7 @@ check_continued: read_unlock(&tasklist_lock); retval = wait_noreap_copyout(p, pid, uid, CLD_CONTINUED, - SIGCONT, infop); + SIGCONT, infop, ru); BUG_ON(retval == 0); goto end; } @@ -1371,7 +1373,8 @@ end: } asmlinkage long sys_waitid(int which, pid_t pid, - struct siginfo __user *infop, int options) + struct siginfo __user *infop, int options, + struct rusage __user *ru) { if (options & ~(WNOHANG|WNOWAIT|WEXITED|WSTOPPED|WCONTINUED)) return -EINVAL; @@ -1395,7 +1398,7 @@ asmlinkage long sys_waitid(int which, pi return -EINVAL; } - return do_wait(pid, options, infop, NULL, &infop->si_rusage); + return do_wait(pid, options, infop, NULL, ru); } asmlinkage long sys_wait4(pid_t pid, unsigned int __user *stat_addr, Index: linux-2.6/kernel/signal.c =================================================================== RCS file: /home/roland/redhat/bkcvs/linux-2.5/kernel/signal.c,v retrieving revision 1.138 diff -B -b -p -u -r1.138 signal.c --- linux-2.6/kernel/signal.c 8 Sep 2004 14:48:58 -0000 1.138 +++ linux-2.6/kernel/signal.c 15 Sep 2004 08:26:18 -0000 @@ -1500,7 +1500,6 @@ void do_notify_parent(struct task_struct /* FIXME: find out whether or not this is supposed to be c*time. */ info.si_utime = tsk->utime + tsk->signal->utime; info.si_stime = tsk->stime + tsk->signal->stime; - k_getrusage(tsk, RUSAGE_BOTH, &info.si_rusage); info.si_status = tsk->exit_code & 0x7f; if (tsk->exit_code & 0x80) @@ -1558,7 +1557,6 @@ do_notify_parent_cldstop(struct task_str /* FIXME: find out whether or not this is supposed to be c*time. */ info.si_utime = tsk->utime; info.si_stime = tsk->stime; - k_getrusage(tsk, RUSAGE_BOTH, &info.si_rusage); info.si_code = why; switch (why) { @@ -2170,8 +2168,6 @@ int copy_siginfo_to_user(siginfo_t __use err |= __put_user(from->si_status, &to->si_status); err |= __put_user(from->si_utime, &to->si_utime); err |= __put_user(from->si_stime, &to->si_stime); - err |= __copy_to_user(&to->si_rusage, &from->si_rusage, - sizeof(to->si_rusage)); break; case __SI_RT: /* This is not generated by the kernel as of now. */ case __SI_MESGQ: /* But this is */ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2004-09-20 21:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-09-15 3:39 waitid fallout Richard Henderson 2004-09-15 4:37 ` Stephen Rothwell 2004-09-15 5:34 ` Roland McGrath 2004-09-15 6:19 ` Richard Henderson 2004-09-18 13:03 ` Arnd Bergmann 2004-09-20 21:26 ` Roland McGrath 2004-09-20 21:27 ` Roland McGrath 2004-09-15 4:49 ` Richard Henderson 2004-09-15 6:23 ` Andi Kleen 2004-09-16 1:30 ` [PATCH] back out siginfo_t.si_rusage from waitid changes Roland McGrath
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.