Re: F_SETOWN_TID: F_SETOWN was thread-specific for a while

[stephane eranian <eranian@googlemail.com>]

Oleg,

On Tue, Aug 18, 2009 at 1:45 PM, Oleg Nesterov<oleg@redhat.com> wrote:
> On 08/18, stephane eranian wrote:
>> > 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.
>
The reason I refer to SA_SIGINFO is simply because of the excerpt from the
sigaction man page:

       If  SA_SIGINFO  is specified in sa_flags, then sa_sigaction (instead of
       sa_handler) specifies the signal-handling function  for  signum.   This
       function receives the signal number as its first argument, a pointer to
       a siginfo_t as its second argument and a pointer to a ucontext_t  (cast
       to void *) as its third argument.

In other words, I use this to emphasize the fact that to get a siginfo
struct, you need to pass SA_SIGINFO and use sa_sigaction instead of
sa_handler. That's all I am saying.


> 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?
>
No, I think we are in agreement. To get meaningful siginfo use SA_SIGINFO.

> 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 });
>               ...
>       }
>
I don't think you can check si_code without first checking which
signal it is in si_signo. The values for si_code overlap between
the different struct in siginfo->_sifields.

>> 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.
>
I agree with you. Given the existing documentation in the man page
of fcntl() and the various code examples. I think it is possible for
developers to figure out how to get the si_fd in the handler. This
problem is not specific to perf_counters nor perfmon. Any SIGIO-based
program may be interested in getting si_fd, therefore I am assuming the
solution is well understood at this point.