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

On Thu, Feb 08, 2024 at 04:57:31PM +0100, Oleg Nesterov wrote:

On 02/08, Eric W. Biederman wrote:

quoted

Oleg Nesterov [off-list ref] writes:

quoted

Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
pid_type to group_send_sig_info(), despite its name it should work fine
even if type = PIDTYPE_PID.

Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
on PIDFD_THREAD.

While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.

I have a question here.

Why is this based on group_send_sig_info instead of send_sig_info?

Well. send_sig_info() accepts "struct task_struct *", not "struct pid *",
it doesn't do check_kill_permission(), and it doesn't handle the possible
race with mt-exec.

quoted

In particular I am asking are the intended semantics that the signal is
sent to a single thread in a thread group and placed in the per thread
queue, or is the signal sent to the entire thread group and placed
in the thread group signal queue?

This depends on PIDFD_THREAD. If it is set then the signal goes to
the per thread queue.

quoted

Because honestly right now using group_send_sig_info when
the intended target of the signal is not the entire thread
group is very confusing when reading your change.

Agreed, so perhaps it makes sense to rename it later. See

Agreed. The function seems misnamed and incorrectly documented. It just
seems that it's never been used with PIDTYPE_PID but it's perfectly
capable of doing that. So maybe just put a patch on top renaming it to
send_sig_info_type() and remove the old comment. But I can live without
renaming it for now as well.