Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG_NOINFO

Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG_NOINFO

[Oleg Nesterov]

On 12/15, Sukadev Bhattiprolu wrote:
>
> Oleg Nesterov [oleg@redhat.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.

Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG_NOINFO

[Sukadev Bhattiprolu]

| > 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).

Well, to keep the patch descriptions simple, how about I split the
two changes into two patches. First one masquerades ->si_pid with
task_pid_nr_ns() and second one uses task_tgid_nr_ns(). They
do look like unrelated changes.

| 
| > 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.

I have highlighted this and the the reparent-thread behavior
changes in the patch description.

| 
| 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).

Ok. We have to change the __si_special() indirection or how about
removing it altogether and change uses of __si_special() to one of:

	{ SEND_SIG_NOINFO, SEND_SIG_PRIV, SEND_SIG_SELF, &info }

It would add a condition check in places like:

	disassociate_ctty(): kill_pgrp(old_pgrp, SIGHUP, on_exit);

but there aren't too many of those.