Re: [PATCH 0/5] Add pidfd support to the fanotify API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 02, 2021 at 03:18:03PM +0300, Amir Goldstein wrote:
> > > 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.
> >
> > > 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.
> >
> > > 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.

Yes, agree. Going ahead with returning FAN_NOPIDFD for early process
termination i.e. before fanotify calls into pidfd creation code. All other
pidfd creation errors will be reported as FAN_EPIDFD.

> 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.

Good point. I feel as though reporting FAN_NOPIDFD in such cases would be
more appropriate, rather than leaving off a pidfd info record completely.

> > > > 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.

Yeah, that's a good approach. I'll consider doing this for future API
changes.

/M



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux