Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 18 Aug 2009 13:45:33 +0200 | From | Oleg Nesterov <> | Subject | Re: F_SETOWN_TID: F_SETOWN was thread-specific for a while |
| |
On 08/18, stephane eranian wrote: > > On Mon, Aug 17, 2009 at 7:16 PM, Oleg Nesterov<oleg@redhat.com> wrote: > > Sorry for late reply... > > > > On 08/10, stephane eranian wrote: > >> > >> On Mon, Aug 10, 2009 at 7:03 PM, Oleg Nesterov<oleg@redhat.com> 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.
| ![\](/images/icornerr.gif) |