[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090731205246.GA3457@redhat.com>
Date: Fri, 31 Jul 2009 22:52:46 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc: eranian@...il.com, Ingo Molnar <mingo@...e.hu>,
LKML <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Robert Richter <robert.richter@....com>,
Paul Mackerras <paulus@...ba.org>,
Andi Kleen <andi@...stfloor.org>,
Maynard Johnson <mpjohn@...ibm.com>,
Carl Love <cel@...ibm.com>,
Corey J Ashford <cjashfor@...ibm.com>,
Philip Mucci <mucci@...s.utk.edu>,
Dan Terpstra <terpstra@...s.utk.edu>,
perfmon2-devel <perfmon2-devel@...ts.sourceforge.net>,
Michael Kerrisk <mtk.manpages@...glemail.com>,
roland <roland@...hat.com>
Subject: Re: [RFC][PATCH] fcntl: F_[SG]ETOWN_TID
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists