Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD

On Sat, Feb 10, 2024 at 01:30:33PM +0100, Oleg Nesterov wrote:

Christian,

Thanks again! the last 2 commits in vfs.pidfd look good to me.

As for this patch, I am not sure I understand your concerns, and I
have another concern, please see below.

For the moment, please forget about PIDFD_THREAD.

On 02/10, Christian Brauner wrote:

quoted

(1) kill(-1234) => kill process group with id 1234
(2) kill(0)     => kill process group of @current

which implementation wise is indicated by

__kill_pgrp_info(..., pid ? find_vpid(-pid) ? task_pgrp(current))

We're obviously not going to implement (2) as that doesn't really make a
sense for pidfd_send_signal().

Sure,

quoted

But (1) is also wrong for pidfd_send_signal(). If we'd ever implement
(1) it should be via pidfd_open(1234, PIDFD_PROCESS_GROUP).

Why do you think we need another flag for open() ?

We don't need one. We could if we wanted to was my point. But let's
ignore that for now.

To me it looks fine if we allow to send the signal to pgrp if
flags & PIDFD_SIGNAL_PROCESS_GROUP.

Yes, that's what I want too, I just wonder about the semantics.

And pidfd_send_signal() can just do

	if (PIDFD_SIGNAL_THREAD_GROUP)
		ret = __kill_pgrp_info(sig, kinfo, pid);
	else
		ret = kill_pid_info_type(...);
		
(yes, yes, this needs tasklist, just a pseudo code to simpliy)

Now lets recall about PIDFD_THREAD.

If the target task is a group leader - there is no difference.

If it is not a leader - then __kill_pgrp_info() will always return
-ESRCH, do_each_pid_task(PIDTYPE_PGID) won't find any task.

And personally I think this is all we need.

------------------------------------------------------------------------------
But if you want to make PIDFD_SIGNAL_THREAD_GROUP work even if the
target task is not a leader, then yes, we need something like

	task_pgrp(pid_task(pid, PIDTYPE_PID))

like you did in the new kill_pgrp_info() helper in this patch.

I won't argue, but do you think this makes a lot of sense?

The question is what is more useful for userspace when they do:
pidfd_send_signal(1234, PIDFD_SEND_PROCESS_GROUP)?

(1) They either mean to signal a process group that is headed by 1234.
(2) Or they want to signal a process group of which 1234 is a member or
    the leader.

From a usability perspective (1) is a lot more restrictive because it
requires @pidfd to refer to a process group leader. Whereas (2) doesn't
require userspace to hold a @pidfd to a process group leader. It is
enough to just hold a @pidfd. In other words, (2) has wider scope.

And intuitively that is what I had thought is more useful. But by that
logic PIDFD_SEND_THREAD_GROUP would have to signal to the thread-group
that @pidfd is in regardless of whether @pidfd is actually a
thread-group leader.

Which is also what you're pointing out, afaict.