lkml.org 
[lkml]   [2009]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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.



\
 
 \ /
  Last update: 2009-08-18 13:53    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site