Re: [PATCH 2/4] fanotify: Add pids to events

Re: [PATCH 2/4] fanotify: Add pids to events

[Matthew Helsley]

On Wed, Dec 2, 2009 at 6:14 AM, Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> From: Andreas Gruenbacher <agruen-l3A5Bk7waGM@public.gmane.org>
>
> Pass the process identifiers of the triggering processes to fanotify
> listeners: this information is useful for event filtering and logging.
>
> Signed-off-by: Andreas Gruenbacher <agruen-l3A5Bk7waGM@public.gmane.org>
> Signed-off-by: Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>
>  fs/notify/fanotify/fanotify.c      |    5 +++--
>  fs/notify/fanotify/fanotify_user.c |    1 +
>  fs/notify/notification.c           |    3 +++
>  include/linux/fanotify.h           |    1 +
>  include/linux/fsnotify_backend.h   |    1 +
>  5 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 5b0b6b4..881067d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -10,8 +10,9 @@ static bool should_merge(struct fsnotify_event *old, struct fsnotify_event *new)
>  {
>        pr_debug("%s: old=%p new=%p\n", __func__, old, new);
>
> -       if ((old->to_tell == new->to_tell) &&
> -           (old->data_type == new->data_type)) {
> +       if (old->to_tell == new->to_tell &&
> +           old->data_type == new->data_type &&
> +           old->tgid == new->tgid) {
>                switch (old->data_type) {
>                case (FSNOTIFY_EVENT_PATH):
>                        if ((old->path.mnt == new->path.mnt) &&
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 7646111..7cfb2f6 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -108,6 +108,7 @@ static ssize_t fill_event_metadata(struct fsnotify_group *group,
>        metadata->event_len = FAN_EVENT_METADATA_LEN;
>        metadata->vers = FANOTIFY_METADATA_VERSION;
>        metadata->mask = fanotify_outgoing_mask(event->mask);
> +       metadata->pid = pid_vnr(event->tgid);

Eric, you never replied to my point about pid namespaces
(http://lkml.org/lkml/2009/7/1/2). I'm still concerned that it's a
problem for this patch. I've cc'd some pid namespace folks, listed the
problems, and some alternative solutions (where I could think of any)
below:

1. Since fanotify doesn't hold a reference to the struct pid then the
pid can become stale before the event is acted upon.
    solution a: Just ignoring this problem, like other interfaces
often do, is probably ok.
    ... ?
    solution z: Seems to require taking a reference to the pid and
giving userspace a way to drop the reference after it's done using
this value to refer to the process (yuck).

2. If the event recipient does a clone and enters a new pidns the pid
number will be incorrect without any indication.
  solution a: The events could be flushed during clone(CLONE_NEWPID)
but then userspace would miss
      them.
  solution b: Translating the pids wouldn't work because the tasks
generaly won't exist in the new
      namespace. So perhaps just write negative values in the pid
fields during clone(CLONE_NEWPID).

3. If the listening process is not in the same or an ancestor pid
namespace of the triggering process then there is no correct pid
corresponding to the event.  (Note: Always using the initial pid
namespace isn't a good [interim] solution -- then programs relying on
fanotify couldn't run in other pid namespaces).
    solution a: Again, perhaps they should be set to negative values.
    ... ?
    solution z: Disable fanotify in non-initial pid namespaces (yuck).

Cheers,
    -Matt Helsley

Re: [PATCH 2/4] fanotify: Add pids to events

[Andreas Gruenbacher]

On Friday 15 January 2010 05:41:10 Matthew Helsley wrote:
> Eric, you never replied to my point about pid namespaces
> (http://lkml.org/lkml/2009/7/1/2). I'm still concerned that it's a
> problem for this patch. I've cc'd some pid namespace folks, listed the
> problems, and some alternative solutions (where I could think of any)
> below:
> 
> 1. Since fanotify doesn't hold a reference to the struct pid then the
> pid can become stale before the event is acted upon.
>     solution a: Just ignoring this problem, like other interfaces
> often do, is probably ok.
>     ... ?
>     solution z: Seems to require taking a reference to the pid and
> giving userspace a way to drop the reference after it's done using
> this value to refer to the process (yuck).

struct fsnotify_event->tgid does hold a reference to the appropriate struct 
pid.  The reference is released when that struct fsnotify_event is freed.

> 2. If the event recipient does a clone and enters a new pidns the pid
> number will be incorrect without any indication.

No, if a process has a pid within the listener's namespace the listener will 
see this pid; otherwise, the resulting pid value is 0.

> 3. If the listening process is not in the same or an ancestor pid
> namespace of the triggering process then there is no correct pid
> corresponding to the event.

Indeed, if the listener is not in the same or an ancestor pid namespace, the 
pid in the event will end up as 0.  The event still indicates that something 
has happened to a file the listener is interested in though, it's just unclear 
who triggered the event.  I don't see a problem with that though -- do you?

Thanks,
Andreas

Re: [PATCH 2/4] fanotify: Add pids to events

[Matt Helsley]

On Fri, Jan 15, 2010 at 04:12:09PM +0100, Andreas Gruenbacher wrote:
> On Friday 15 January 2010 05:41:10 Matthew Helsley wrote:
> > Eric, you never replied to my point about pid namespaces
> > (http://lkml.org/lkml/2009/7/1/2). I'm still concerned that it's a
> > problem for this patch. I've cc'd some pid namespace folks, listed the
> > problems, and some alternative solutions (where I could think of any)
> > below:
> > 
> > 1. Since fanotify doesn't hold a reference to the struct pid then the
> > pid can become stale before the event is acted upon.
> >     solution a: Just ignoring this problem, like other interfaces
> > often do, is probably ok.
> >     ... ?
> >     solution z: Seems to require taking a reference to the pid and
> > giving userspace a way to drop the reference after it's done using
> > this value to refer to the process (yuck).
> 
> struct fsnotify_event->tgid does hold a reference to the appropriate struct 
> pid.  The reference is released when that struct fsnotify_event is freed.

OK.

> 
> > 2. If the event recipient does a clone and enters a new pidns the pid
> > number will be incorrect without any indication.
> 
> No, if a process has a pid within the listener's namespace the listener will 
> see this pid; otherwise, the resulting pid value is 0.

So the pid reference is resolved at read(), correct? If so then that's fine.
(Otherwise I'd think the values could still become stale).

> > 3. If the listening process is not in the same or an ancestor pid
> > namespace of the triggering process then there is no correct pid
> > corresponding to the event.
> 
> Indeed, if the listener is not in the same or an ancestor pid namespace, the 
> pid in the event will end up as 0.  The event still indicates that something 
> has happened to a file the listener is interested in though, it's just unclear 
> who triggered the event.  I don't see a problem with that though -- do you?

Nope. Overall, looks good to me. Thanks!

Cheers,
	-Matt Helsley

Re: [PATCH 2/4] fanotify: Add pids to events

[Andreas Gruenbacher]

On Friday 15 January 2010 10:21:46 pm Matt Helsley wrote:
> On Fri, Jan 15, 2010 at 04:12:09PM +0100, Andreas Gruenbacher wrote:
> > > 2. If the event recipient does a clone and enters a new pidns the pid
> > > number will be incorrect without any indication.
> >
> > No, if a process has a pid within the listener's namespace the listener
> > will see this pid; otherwise, the resulting pid value is 0.
> 
> So the pid reference is resolved at read(), correct? If so then that's
>  fine. (Otherwise I'd think the values could still become stale).

Yes.  Note that for non-blocking events, there is no guarantee that the 
triggering process still runs when the event is consumed though.

Andreas

Re: [PATCH 2/4] fanotify: Add pids to events

[Matt Helsley]

On Sat, Jan 16, 2010 at 11:53:45PM +0100, Andreas Gruenbacher wrote:
> On Friday 15 January 2010 10:21:46 pm Matt Helsley wrote:
> > On Fri, Jan 15, 2010 at 04:12:09PM +0100, Andreas Gruenbacher wrote:
> > > > 2. If the event recipient does a clone and enters a new pidns the pid
> > > > number will be incorrect without any indication.
> > >
> > > No, if a process has a pid within the listener's namespace the listener
> > > will see this pid; otherwise, the resulting pid value is 0.
> > 
> > So the pid reference is resolved at read(), correct? If so then that's
> >  fine. (Otherwise I'd think the values could still become stale).
> 
> Yes.  Note that for non-blocking events, there is no guarantee that the 
> triggering process still runs when the event is consumed though.

Holding the reference until the event is read ensures the pid
won't be reused in that namespace, so I think that's fine.

Cheers,
	-Matt Helsley