On Tue, Feb 20, 2024 at 05:22:02PM +0100, Oleg Nesterov wrote: > On 02/20, Christian Brauner wrote: > > > > On Tue, Feb 20, 2024 at 12:00:12PM +0100, Oleg Nesterov wrote: > > > > > > Perhaps we can kill the "task_pid(current) != pid" check and just return > > > EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think > > > anobody needs pidfd_send_send_signal() to signal yourself. See below. > > > > Yeah. > > You have my ack in advance > > > > > + /* Currently unused. */ > > > > + if (info) > > > > + return -EINVAL; > > > > > > Well, to me this looks like the unnecessary restriction... And why? > > > > Because right now we aren't sure that it's used > > Yes, but... > > > and we aren't sure what use-cases are there. > > the same use-cases as for rt_sigqueueinfo() ? Specifically for pidfd_send_signal() I mean. To me it seems very unlikely that anyone would be opening a pidfd to itself and then use pidfd_send_signal() when they could entirely avoid this - race free - by simply using *sigqueueinfo(). But I may be wrong of course. > > Christian, I won't really argue but I still disagree. > > Let me first repeat once again, I do not know what people do with pidfd > and pidfd_send_signal() in particular, so I won't be surprised if this > change won't cause any regression report. Fwiw, that's fine as long as we'd fix it up. > > But at the same time, I can easily imagine the following scenario: a > userspace programmer tries to use pidfd_send_signal(info != NULL), gets > -EINVAL, decides it can't/shouldn't work, and switches to sigqueueinfo() > without any report to lkml. > > > Yes, absolutely. That was always the plan. See appended patch I put on top. > > I put you as author since you did spot this. Let me know if you don't > > want that. > > Ah. Thanks Christian. I am fine either way, whatever is more convenient > for you. > > But just in case, I won't mind at all if you simply fold this minor fix > into your PIDFD_SEND_PROCESS_GROUP patch, I certainly don't care about > the "From" tag ;) > > A really, really minor/cosmetic nit below, feel free to ignore: > > > - if ((task_pid(current) != pid) && > > + if (((task_pid(current) != pid) || type > PIDTYPE_TGID) && > > we can remove the unnecessary parens around "task_pid(current) != pid" > or add the extra parens aroung "type > PIDTYPE_TGID". > > I mean, the 1st operand of "&&" is > > (task_pid(current) != pid) || type > PIDTYPE_TGID > > and this looks a bit inconsistent to me. Ok, will do.
