Re: [PATCH v2] fanotify: report thread pidfds for FAN_REPORT_TID

[Amir Goldstein]

On Fri, May 29, 2026 at 9:40 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Fri, May 29, 2026 at 09:21:55AM +0200, Amir Goldstein wrote:
> > On Fri, May 29, 2026 at 4:01 AM AnonymeMeow <anonymemeow@xxxxxxxxx> wrote:
> > >
> > > The FAN_REPORT_PIDFD and FAN_REPORT_TID flags used to be mutually
> > > exclusive because by the time the pidfd support was introduced to
> > > fanotify, pidfds could only be created for thread group leaders. Now
> > > that the pidfd API supports thread-specific pidfds via PIDFD_THREAD,
> > > this restriction can be lifted.
> > >
> > > Also drop the pid_has_task() check to allow pidfds to be reported for
> > > reaped tasks as well.
> >
> > I am not objecting to this change, but as a rule of thumb, almost
> > every time that
> > a commit message says "Also" it means that it should have been a separate
> > commit.
> >
> > This is not an exception to the rule and even worse -
> > This change PIDFD_STALE is an independent change of behavior
> > (as observed by breakage of fanotify21) that could (maybe unlikely)
> > surprise real users.
>
> I don't think so. It's the same as when we enabled PIDFD_STALE for
> AF_UNIX sockets: it strictly expanded the usability of the interface as
> you could now reliably get a pidfd for all events and get stronger
> guarantees.
>
> > We have few options:
> > 1. Make it opt-in with yet another fanotify_init flag
> > 2. Enable it implicitly together (and in the same patch) with FAN_REPORT_TID
> > 3. Enable it without opt-in (as you did) but as separate patch
> >     that will be reverted if someone complains on "real world"
> >     (i.e. not test) breakage
> >
> > IMO, the path of least resistance is option 2.
> > If users want to get the pidfd of tgid I suppose there should be no
> > problem to get it from the pidfd of the thread. Right?
> > So what FAN_REPORT_TID really means is give me a richer pidfd
> > with more information about the actor than without FAN_REPORT_TID.
> >
> > I'd like to get Jan's input on those UAPI choices.
> >
> > >
> > > Link: https://lore.kernel.org/lkml/20260528-schmuckvoll-heilen-garen-be77b4208671@brauner/
> > > Signed-off-by: AnonymeMeow <anonymemeow@xxxxxxxxx>
> > > ---
> > >
> > > On 2026-05-28 13:51 +0200, Christian Brauner wrote:
> > > > For quite a while the kernel refused to hand out pidfds for reaped
> > > > processes even if the struct pid was pinned like in this case.
> > > >
> > > > But that makes various APIs - including this one - way less powerful
> > > > than they can be. Nowadays the socket layer already hands out pidfds for
> > > > reaped processes. It also stashed the struct pid. Let's do the same
> > > > here.
> > > >
> > > > Drop the pid_has_task() change and then:
> > > >
> > > > pidfd = pidfd_prepare(event->pid, pidfd_flags | PIDFD_STALE, &pidfd_file);
> > > >
> > > > which instructs pidfs to and out a pidfd even if the task has already
> > > > been reaped. Reaped pidfds can still be queried for various types of
> > > > information that is kept around even if the task is long gone.
> > >
> > > Thanks for the review. I've updated this in v2 as you suggested.
> > >
> > > Also, the LTP fanotify21 test currently explicitly expects ESRCH or
> > > FAN_NOPIDFD for exited processes. I will update that test case accordingly.
> >
> > This sentence highlights the problematic aspect of this change.
> > How would you update the test expectation?
> > The test must be able to run on upstream as well as stable kernels.
> > Would you test for support in FAN_REPORT_TID
> > and according to the result determine the expected behavior of the
> > test cases (variants) without FAN_REPORT_TID?
> > Very ugly IMO.
> >
> > OTH, if you take the suggestion that PIDFD_STALE is implied only
> > for FAN_REPORT_TID, you can change the expectation for the
> > "terminated child" test case only for TST_VARIANT_PIDFD_THREAD.
>
> I think this is just not worth it. It brings users no value if they get
> slightly different semantics for effectively the same API. I don't see
> what the problem is with just enabling it and fixing the LTP tests.
> We've done that so many times before when APIs get extended.

Of course, fixing LTP tests is no problem.
I'm just not sure what the expectation should be for test that
runs on different stable kernels and I assume this change cannot
be backported all the way to all versions with FAN_REPORT_PIDFD
support. If the test can be made to support both its fine.

>
> Worst-case someone comes back and tells us that they were relying on
> that specific behavior then you just revert. So I would propose to add a
> separate patch on top of PIDFD_THREAD that enables PIDFD_STALE
> unconditionally. If this leads to non-test regressions revert the patch.

I can live with that, but the legacy flag behavior change  needs to be in
a patch of its own clearly stating this change.

Thanks,
Amir.