linux-kernel - Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081216101621.GA2784@redhat.com>
Date:	Tue, 16 Dec 2008 11:16:21 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
Cc:	roland@...hat.com, "Eric W. Biederman" <ebiederm@...ssion.com>,
	Pavel Emelyanov <xemul@...nvz.org>, bastian@...di.eu.org,
	sukadev@...ibm.com, Albert Cahalan <acahalan@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG_NOINFO

On 12/15, Sukadev Bhattiprolu wrote:
>
> Oleg Nesterov [oleg@...hat.com] wrote:
> | On 12/12, Sukadev Bhattiprolu wrote:
> | >
> | > --- a/kernel/signal.c
> | > +++ b/kernel/signal.c
> | > @@ -856,7 +856,8 @@ static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
> | >  			q->info.si_signo = sig;
> | >  			q->info.si_errno = 0;
> | >  			q->info.si_code = SI_USER;
> | > -			q->info.si_pid = task_pid_vnr(current);
> | > +			q->info.si_pid = task_tgid_nr_ns(current,
> | > +							task_active_pid_ns(t));
> |
> | I agree with the patch, but you also changed "pid" to "tgid",
> | and the changelog says nothing about this.
>
> Yes, I should have mentioned it in the changelog.
> |
> | Actually, this looks more correct to me, but did you check we
> | don't break something?
>
> Besides running some tests and LTP and posting as [rfc] not sure how
> else to check for breakage :-) Appreciate any pointers to tests for
> signals.
>
> I had seen an earlier version of this patch which mentioned that
> POSIX requires tgid rather than pid. And that looked logical to
> me

Let me repeat, I think this is more correct too, and personally
I agree with this change.

But I do not know if this change is "safe", please see below.

> and did not know about follow requirement on pdeath_signal.

Sorry for confusion, I didn't mean this will confuse the users
of ->pdeath_signal, I just do not know. But they need the
notificatation about the thread, not process.

> | For example, ->pdeath_signal. Note that we send it even if we
> | re-parent to sub-thread, not to our init. Yes, yes, yes. This
> | is imho very ugly and we should stop doing this. But we have
> | users which claim they need exactly this behaviour.
>
> Ok. so should I change it to task_pid_nr_ns() ?

I don't know. I vote for your patch as is. (but please update
the changelog).

> Would that still
> be correct for say the SIGPIPE from pipe_write() ?

Again, I don't know.

But. Let's suppose we have the user-space application which
uses .si_pid to figure out which thread hits SIGPIPE. Yes,
this is ugly because the signal is thread-specific, but this
works until this patch.

Actually, I don't understand most of (all?) users of
send_signal(SIGXXX, current, 0), SEND_SIG_PRIV looks more
logical to me. Or, _perhaps_, we need yet another SEND_SIG_SELF
which sets .si_pid = task_pid_vnr(target).

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/