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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6a12d2f31001142041j7f917b07l1e1a728790175321@mail.gmail.com>
Date:	Thu, 14 Jan 2010 20:41:10 -0800
From:	Matthew Helsley <matt.helsley@...il.com>
To:	Eric Paris <eparis@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	viro@...iv.linux.org.uk, hch@...radead.org, agruen@...e.de,
	containers@...ts.linux-foundation.org, matthltc@...ibm.com,
	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH 2/4] fanotify: Add pids to events

On Wed, Dec 2, 2009 at 6:14 AM, Eric Paris <eparis@...hat.com> wrote:
> From: Andreas Gruenbacher <agruen@...e.de>
>
> 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@...e.de>
> Signed-off-by: Eric Paris <eparis@...hat.com>
> ---
>
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/