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

[stephane eranian]

Oleg,

On Tue, Aug 18, 2009 at 1:45 PM, Oleg Nesterov<oleg@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/