Re: [perfmon2] F_SETOWN_TID: F_SETOWN was thread-specific for a while
Status: Beta
Brought to you by:
seranian
From: Oleg N. <ol...@re...> - 2009-08-18 11:50:21
|
On 08/18, stephane eranian wrote: > > On Mon, Aug 17, 2009 at 7:16 PM, Oleg Nesterov<ol...@re...> wrote: > > Sorry for late reply... > > > > On 08/10, stephane eranian wrote: > >> > >> On Mon, Aug 10, 2009 at 7:03 PM, Oleg Nesterov<ol...@re...> wrote: > >> > > >> > Not sure if it is safe to change the historical behaviour. > >> > > >> Don't need to change it. > > > > Good, > > > >> But for SIGIO, if you see SA_SIGINFO, then pass the si_fd. > > > > But this means we do change the behaviour ;) Confused. > > > I meant do not remove F_SETSIG and its side-effect on si_fd. Ah, now I see what you meant. > > In any case. We should not look at SA_SIGINFO at all. If sys_sigaction() was > > called without SA_SIGINFO, then it doesn'matter if we send SEND_SIG_PRIV or > > siginfo_t with the correct si_fd/etc. > > > What's the official role of SA_SIGINFO? Pass a siginfo struct? > > Does POSIX describe the rules governing the content of si_fd? > Or is si_fd a Linux-ony extension in which case it goes with F_SETSIG. Not sure I understand your concern... OK. You suggest to pass siginfo_t with .si_fd/etc when we detect SA_SIGINFO. But, in that case we can _always_ pass siginfo_t, regardless of SA_SIGINFO. If the task has a signal handler and sigaction() was called without SA_SIGINFO, then the handler must not look into *info (the second arg of sigaction->sa_sigaction). And in fact, __setup_rt_frame() doesn't even copy the info to the user-space: if (ka->sa.sa_flags & SA_SIGINFO) { if (copy_siginfo_to_user(&frame->info, info)) return -EFAULT; } OK? Or I missed something? > > And again, this is even documented. The change is trivial but user-space > > visible, it may confuse the (stupid) app which uses SIGIO + SA_SIGINFO > > without F_SETSIG. > > > That would be an app that uses SIGINFO and fiddles with si_fd which has no > defined content. What kind of app would that be? The stupid app. But it is always unsafe to make the user-visible changes without good reasons. Even when we fix the bug (and the current code is not buggy) sometimes we have "this patch breaks my app or test-case!" reports. If nothing else, we can break the test-case which simply does void sigio_handler(int sig, siginfo_t *info, void *u) { assert(info->si_code == 0 && info->si_code = 0x80); } Once again: this is _documented_ ! And we can't set .si_fd = fd whithout changing .si_code, this will break copy_siginfo_to_user(). Or. Suppose that some app does: void io_handler(int sig, siginfo_t *info, void *u) { if ((info->si_code & __SI_MASK) != SI_POLL) { // RT signal failed! sig MUST be == SIGIO recover(); } else { do_something(info->si_fd); } } int main(void) { sigaction(SIGRTMIN, { SA_SIGINFO, io_handler }); sigaction(SIGIO, { SA_SIGINFO, io_handler }); ... } This is correct. But if we change the current behaviour, this app won't be able to detect the overflow. > It would seem natural that in the siginfo passed to the handler of SIGIO, the > file descriptor be passed by default. That is all I am trying to say here. Completely agreed! I was always puzzled by send_sigio_to_task(). I was never able to understand why it looks so strange. So, I think it should be static void send_sigio_to_task(struct task_struct *p, struct fown_struct *fown, int fd, int reason) { siginfo_t si; /* * F_SETSIG can change ->signum lockless in parallel, make * sure we read it once and use the same value throughout. */ int signum = ACCESS_ONCE(fown->signum) ?: SIGIO; if (!sigio_perm(p, fown, signum)) return; si.si_signo = signum; si.si_errno = 0; si.si_code = reason; si.si_fd = fd; /* Make sure we are called with one of the POLL_* reasons, otherwise we could leak kernel stack into userspace. */ BUG_ON((reason & __SI_MASK) != __SI_POLL); if (reason - POLL_IN >= NSIGPOLL) si.si_band = ~0L; else si.si_band = band_table[reason - POLL_IN]; /* Failure to queue an rt signal must be reported as SIGIO */ if (!group_send_sig_info(signum, &si, p)) group_send_sig_info(SIGIO, SEND_SIG_PRIV, p); } (except it should be on top of fcntl-add-f_etown_ex.patch). This way, at least we don't break the "detect RT signal failed" above. What do you think? But let me repeat: I just can't convince myself we have a good reason to change the strange, but carefully documented behaviour. In case you agree with the code above, I can send the patch. But only if I have a "good" changelog from you + your Signed-of-by in advance ;) Otherwise, please feel free to send this/similar change yourself. Oleg. |