All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@ubuntu.com>
To: Jan Kara <jack@suse.cz>
Cc: Matthew Bobrowski <repnop@google.com>,
	amir73il@gmail.com, linux-fsdevel@vger.kernel.org,
	linux-api@vger.kernel.org
Subject: Re: [PATCH 0/5] Add pidfd support to the fanotify API
Date: Tue, 25 May 2021 12:31:33 +0200	[thread overview]
Message-ID: <20210525103133.uctijrnffehlvjr3@wittgenstein> (raw)
In-Reply-To: <20210524084746.GB32705@quack2.suse.cz>

On Mon, May 24, 2021 at 10:47:46AM +0200, Jan Kara wrote:
> On Sat 22-05-21 09:32:36, Matthew Bobrowski wrote:
> > On Fri, May 21, 2021 at 12:40:56PM +0200, Jan Kara wrote:
> > > On Fri 21-05-21 20:15:35, Matthew Bobrowski wrote:
> > > > On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
> > > > There's one thing that I'd like to mention, and it's something in
> > > > regards to the overall approach we've taken that I'm not particularly
> > > > happy about and I'd like to hear all your thoughts. Basically, with
> > > > this approach the pidfd creation is done only once an event has been
> > > > queued and the notification worker wakes up and picks up the event
> > > > from the queue processes it. There's a subtle latency introduced when
> > > > taking such an approach which at times leads to pidfd creation
> > > > failures. As in, by the time pidfd_create() is called the struct pid
> > > > has already been reaped, which then results in FAN_NOPIDFD being
> > > > returned in the pidfd info record.
> > > > 
> > > > Having said that, I'm wondering what the thoughts are on doing pidfd
> > > > creation earlier on i.e. in the event allocation stages? This way, the
> > > > struct pid is pinned earlier on and rather than FAN_NOPIDFD being
> > > > returned in the pidfd info record because the struct pid has been
> > > > already reaped, userspace application will atleast receive a valid
> > > > pidfd which can be used to check whether the process still exists or
> > > > not. I think it'll just set the expectation better from an API
> > > > perspective.
> > > 
> > > Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can
> > > be sure the original process doesn't exist anymore. So is it useful to
> > > still receive pidfd of the dead process?
> > 
> > Well, you're absolutely right. However, FWIW I was approaching this
> > from two different angles:
> > 
> > 1) I wanted to keep the pattern in which the listener checks for the
> >    existence/recycling of the process consistent. As in, the listener
> >    would receive the pidfd, then send the pidfd a signal via
> >    pidfd_send_signal() and check for -ESRCH which clearly indicates
> >    that the target process has terminated.
> > 
> > 2) I didn't want to mask failed pidfd creation because of early
> >    process termination and other possible failures behind a single
> >    FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the
> >    listener can take clear corrective branches as what's to be done
> >    next if a race is to have been detected, whereas simply returning
> >    FAN_NOPIDFD at this stage can mean multiple things.
> > 
> > Now that I've written the above and keeping in mind that we'd like to
> > refrain from doing anything in the event allocation stages, perhaps we
> > could introduce a different error code for detecting early process
> > termination while attempting to construct the info record. WDYT?
> 
> Sure, I wouldn't like to overengineer it but having one special fd value for
> "process doesn't exist anymore" and another for general "creating pidfd
> failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

?

Christian

  reply	other threads:[~2021-05-25 10:33 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20  2:09 [PATCH 0/5] Add pidfd support to the fanotify API Matthew Bobrowski
2021-05-20  2:10 ` [PATCH 1/5] kernel/pid.c: remove static qualifier from pidfd_create() Matthew Bobrowski
2021-05-20  2:10 ` [PATCH 2/5] kernel/pid.c: implement checks on parameters passed to pidfd_create() Matthew Bobrowski
2021-05-20  2:11 ` [PATCH 3/5] fanotify_user.c: minor cosmetic adjustments to fid labels Matthew Bobrowski
2021-05-20  2:11 ` [PATCH 4/5] fanotify/fanotify_user.c: introduce a generic info record copying function Matthew Bobrowski
2021-05-20 13:59   ` Amir Goldstein
2021-05-21  9:26     ` Matthew Bobrowski
2021-05-20  2:11 ` [PATCH 5/5] fanotify: Add pidfd info record support to the fanotify API Matthew Bobrowski
2021-05-20  8:17   ` Christian Brauner
2021-05-20 13:43     ` Amir Goldstein
2021-05-21  9:21       ` Matthew Bobrowski
2021-05-21  9:41         ` Amir Goldstein
2021-05-21 10:24           ` Jan Kara
2021-05-21 11:10             ` Amir Goldstein
2021-05-21 13:19               ` Jan Kara
2021-05-21 13:52                 ` Amir Goldstein
2021-05-21 15:14                   ` Jan Kara
2021-05-22  0:41                     ` Matthew Bobrowski
2021-05-22  9:01                       ` Amir Goldstein
2021-05-20 13:55 ` [PATCH 0/5] Add pidfd " Jan Kara
2021-05-21 10:15   ` Matthew Bobrowski
2021-05-21 10:40     ` Jan Kara
2021-05-21 23:32       ` Matthew Bobrowski
2021-05-24  8:47         ` Jan Kara
2021-05-25 10:31           ` Christian Brauner [this message]
2021-05-25 23:20             ` Matthew Bobrowski
2021-05-26 18:05               ` Christian Brauner
2021-05-26 22:54                 ` Matthew Bobrowski
2021-06-01 11:03                 ` Matthew Bobrowski
2021-06-01 11:46                   ` Christian Brauner
2021-06-02  6:30                     ` Matthew Bobrowski
2021-06-02  7:18                       ` Amir Goldstein
2021-06-02  8:48                         ` Christian Brauner
2021-06-02 10:56                           ` Matthew Bobrowski
2021-06-02 12:46                             ` Christian Brauner
2021-06-02 10:43                         ` Matthew Bobrowski
2021-06-02 12:18                           ` Amir Goldstein
2021-06-03  1:24                             ` Matthew Bobrowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210525103133.uctijrnffehlvjr3@wittgenstein \
    --to=christian.brauner@ubuntu.com \
    --cc=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=repnop@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.