Oleg Nesterov [[email protected]] 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 [email protected] | More majordomo info at http://vger.kernel.org/majordomo-info.html | Please read the FAQ at http://www.tux.org/lkml/ _______________________________________________ Containers mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
