On 07/31, Peter Zijlstra wrote:
>
> In order to direct the SIGIO signal to a particular thread of a
> multi-threaded application we cannot, like suggested by the manpage, put
> a TID into the regular fcntl(F_SETOWN) call. It will still be send to
> the whole process of which that thread is part.
>
> Since people do want to properly direct SIGIO we introduce F_SETOWN_TID,
> which functions similarly to F_SETOWN, except positive arguments are
> interpreted as TIDs and negative arguments are interpreted as PIDs.

I think this is correct. But,

> @@ -431,6 +474,7 @@ static void send_sigio_to_task(struct task_struct *p,
>                              int fd,
>                              int reason)
>  {
> +     int (*send_sig)(int, struct siginfo *, struct task_struct *);
>       /*
>        * F_SETSIG can change ->signum lockless in parallel, make
>        * sure we read it once and use the same value throughout.
> @@ -440,6 +484,8 @@ static void send_sigio_to_task(struct task_struct *p,
>       if (!sigio_perm(p, fown, signum))
>               return;
>
> +     send_sig = fown->task_only ? send_sig_info : group_send_sig_info;

this is ugly.

I do not blame your patch, I blame signal.c which has a lot of helpers
to send a signal but it is never possible to find the right one.

I think we need a new trivial helper,

        int xxx(int sig, struct siginfo *info, struct task_struct *p, bool 
group)
        {
                unsigned long flags;
                int ret = -ESRCH;

                if (lock_task_sighand(p, &flags)) {
                        ret = send_signal(sig, info, p, group);
                        unlock_task_sighand(p, &flags);
                }

                return ret;
        }

send_sigio_to_task() can just do: send_signal(..., !fown->task_only).

group_send_sig_info(), send_sig_info() should use this helper too.


Also. without the new helper, F_SETOWN does check_kill_permission()
while F_SETOWN_TID does not. This doesn't really matter, but this
looks a bit odd.

Note that send_sigio_to_task() does not need check_kill_permission().
We use either SEND_SIG_PRIV or SI_FROMKERNEL() signals, so we never
actually check permissions. Even if we did, it would be just wrong
to deny the signal here using current_cred().


Peter, may I suggest to to re-diff your patch on top of the trivial
patch I am going to send a bit later today?

Oleg.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel

Reply via email to