Re: [RFC][PATCH 7/7][v4] SI_TKILL: Masquerade si_pid when crossingpid ns boundary

From: Sukadev Bhattiprolu
Date: Wed Dec 24 2008 - 16:05:49 EST


Oleg Nesterov [oleg@xxxxxxxxxx] wrote:
| On 12/24, Sukadev Bhattiprolu wrote:
| >
| > --- a/kernel/signal.c
| > +++ b/kernel/signal.c
| > @@ -2385,17 +2385,22 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
| > struct siginfo info;
| > struct task_struct *p;
| > unsigned long flags;
| > + struct pid_namespace *ns;
| >
| > error = -ESRCH;
| > info.si_signo = sig;
| > info.si_errno = 0;
| > info.si_code = SI_TKILL;
| > - info.si_pid = task_tgid_vnr(current);
| > info.si_uid = current_uid();
| >
| > rcu_read_lock();
| > p = find_task_by_vpid(pid);
| > if (p && (tgid <= 0 || task_tgid_vnr(p) == tgid)) {
| > + ns = task_active_pid_ns(p);
| > + if (ns)
| > + info.si_pid = task_tgid_nr_ns(current, ns);
| > + else
| > + info.si_pid = task_tgid_vnr(current);
|
| if ns == 0, "p" won't see the signal anyway, so all we need is

Yes, p won't see the signal, but task_tgid_nr_ns() is not safe if ns == NULL.
We should either check ns or make task_tgid_nr_ns() and friends safe with
ns == NULL ?

|
| - info.si_pid = task_tgid_vnr(current);
| + info.si_pid = task_tgid_nr_ns(current, task_active_ns(p));
|
| like we do in __do_notify().

Yes, I had a question about ns == 0 in this patch and was wondering if I
should add a check in __do_notify() too.

|
|
| But. this of course doesn't work for sys_kill(). Can't we change the helpers
| which send SI_FROMUSER() signals so that they do not fill .si_pid at all?

SI_FROMUSER() basically comes down to SI_USER and SI_TKILL (SI_QUEUE,
SI_SIGIO, SI_DETHREAD are unused ?) SI_USER has to be masqueraded in
send_signal(). That leaves us with SI_TKILL.

I was trying to have all si_pid settings done at origin and so the change
here for SI_TKILL. But yes, SI_USER (sys_kill() case) can't be done at
origin hence the special case for it in send_signal().

| Then send_signal() can do:
|
| default:
| copy_siginfo(&q->info, info);
| info.si_pid = 0;
| if (!from_ancestot_ns)
| info.si_pid = task_tgid_nr_ns(current, ...);
|
| ?


My preference was to address SI_TKILL also at origin, but am not
particular. Yes, that will work too.


|
| Yes, we use "current". But we already used it in siginfo_from_ancestor_ns().
|
| Oleg.
|
| --
| 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/
--
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/