[lkml]   [2009]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    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<> wrote:
    > > Sorry for late reply...
    > >
    > > On 08/10, stephane eranian wrote:
    > >>
    > >> On Mon, Aug 10, 2009 at 7:03 PM, Oleg Nesterov<> 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

    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
    } else {

    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))

    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;
    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.


     \ /
      Last update: 2009-08-18 13:53    [W:0.026 / U:86.704 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site