quoted

I still don't understand what's racy about this. Won't the event reader
get a valid pidfd?

I guess this depends, right?

As the logic/implementation currently stands in this specific patch series,
pidfd_create() will _NOT_ return a valid pidfd unless the struct pid still
holds reference to a task of type PIDTYPE_TGID. This is thanks to the extra
pid_hash_task() check that I thought was appropriate to incorporate into
pidfd_create() seeing as though:

 - With the pidfd_create() declaration now being added to linux/pid.h, we
   effectively are giving the implicit OK for it to be called from other
   kernel subsystems, and hence why the caller should be subject to the
   same restrictions/verifications imposed by the API specification
   i.e. "Currently, the process identified by @pid must be a thread-group
   leader...". Not enforcing the pid_has_task() check in pidfd_create()
   effectively says that the pidfd creation can be done for any struct pid
   types i.e. PIDTYPE_PID, PIDTYPE_PGID, etc. This leads to assumptions
   being made by the callers, which effectively then could lead to
   undefined/unexpected behavior.

There definitely can be cases whereby the underlying task(s) associated
with a struct pid have been freed as a result of process being killed
early. As in, the process is killed before the pid_has_task() check is
performed from within pidfd_create() when called from fanotify. This is
precisely the race that I'm referring to here, and in such cases as the
code currently stands, the event listener will _NOT_ receive a valid pidfd.

quoted

Can't the event reader verify that the pidfd points to a dead process?

This was the idea, as in, the burden of checking whether a process has been
killed before the event listener receives the event should be on the event
listener. However, we're trying to come up with a good way to effectively
communicate that the above race I've attempted to articulate has actually
occurred. As in, do we:

a) Drop the pid_has_task() check in pidfd_create() so that a pidfd can be
   returned for all passed struct pids? That way, even if the above race is
   experienced the caller will still receive a pidfd and the event listener
   can do whatever it needs to with it.

b) Before calling into pidfd_create(), perform an explicit pid_has_task()
   check for PIDTYPE_TGID and if that returns false, then set FAN_NOPIDFD
   and save ourselves from calling into pidfd_create() all together. The
   event listener in this case doesn't have to perform the signal check to
   determine whether the process has already been killed.

c) Scrap calling into pidfd_create() all together and write a simple
   fanotify wrapper that contains the pidfd creation logic we need.

quoted

I don't mind returning FAN_NOPIDFD for convenience, but user
will have to check the pidfd that it got anyway, because process
can die at any time between reading the event and acting on the
pidfd.

Well sort of, as it depends on the approach that we decide to go ahead with
to report such early process termination cases. If we return FAN_NOPIDFD,
which signifies that the process died before a pidfd could be created, then
there's no point for the listener to step into checking the pidfd because
the pidfd already == FAN_NOPIDFD. If we simply return a pidfd regardless of
the early termination of the process, then sure the event listener will
need to check each pidfd that is supplied.

I don't see any problem with the fact that the listener would have to check the
reported pidfd. I don't see how or why that should be avoided.
If we already know there is no process, we can be nice and return NOPIDFD,
because that doesn't add any complexity.

Not to mention that if pid_vnr() returns 0 (process is outside of
pidns of caller)
then I think you MUST either report FAN_NOPIDFD or no pid info record,
because getting 0 event->pid  with a valid pidfd is weird IMO and could be
considered as a security breach.

quoted

quoted

because we perform the pidfd creation during the notification queue
processing and not in the event allocation stages (for reasons that Jan has
already covered here [1]). So, tl;dr there is the case where the fanotify
calls pidfd_create() and the check for pid_has_task() fails because the
struct pid that we're hanging onto within an event no longer contains a
task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Maybe I'm going down a rabbit hole and overthinking this whole thing,
IDK... :(

That is the feeling I get as well.
Suggestion: write the man page - that will make it clear to yourself
and to code reviewers if the API is sane and if it is going to end up
being confusing to end users.

Thanks,
Amir.