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

[Sukadev Bhattiprolu]

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/