Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180917141950.111841-1-nixiaoming@huawei.com>
In-Reply-To: <20180917141950.111841-1-nixiaoming@huawei.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Mon, 17 Sep 2018 18:51:18 +0300
Message-ID: <CAOQ4uxj_o9FP8EH9_=TX=Tv9WDfExykrni7OjE3992YZrJDpZg@mail.gmail.com>
Subject: Re: [PATCH v2] fanotify reports the thread id of the event trigger
To:     nixiaoming@huawei.com
Cc:     Jan Kara <jack@suse.cz>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@huawei.com> wrote:
>
> In order to identify which thread triggered the event in the
> multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init

According to code and man page this is a 'flag' not a 'tag'
Please stick to existing terminology.

> to select whether to report the event creator's thread id information.
>
> Signed-off-by: nixiaoming <nixiaoming@huawei.com>
> ---
>  fs/notify/fanotify/fanotify.c      | 5 ++++-
>  fs/notify/fanotify/fanotify_user.c | 5 +++--
>  include/linux/fsnotify_backend.h   | 1 +
>  include/uapi/linux/fanotify.h      | 1 +
>  4 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 94b5215..5a3af87 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -171,7 +171,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
>                 goto out;
>  init: __maybe_unused
>         fsnotify_init_event(&event->fse, inode, mask);
> -       event->tgid = get_pid(task_tgid(current));
> +       if (group->fanotify_data.should_report_tid)
> +               event->tgid = get_pid(task_pid(current));
> +       else
> +               event->tgid = get_pid(task_tgid(current));
>         if (path) {
>                 event->path = *path;
>                 path_get(&event->path);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 6905488..3aa425b 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>                 return -EPERM;
>
>  #ifdef CONFIG_AUDITSYSCALL
> -       if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
> +       if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT | FAN_EVENT_INFO_TID))
>  #else
> -       if (flags & ~FAN_ALL_INIT_FLAGS)
> +       if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_EVENT_INFO_TID))

Not this way. You need to add FAN_EVENT_INFO_TID to FAN_ALL_INIT_FLAGS.

>  #endif
>                 return -EINVAL;
>
> @@ -731,6 +731,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>         }
>
>         group->fanotify_data.user = user;
> +       group->fanotify_data.should_report_tid = (flags & FAN_EVENT_INFO_TID) ? true : false;

Please drop "? true : false" its not needed.

>         atomic_inc(&user->fanotify_listeners);
>         group->memcg = get_mem_cgroup_from_mm(current->mm);
>
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b8f4182..44c659f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -193,6 +193,7 @@ struct fsnotify_group {
>                         unsigned int max_marks;
>                         struct user_struct *user;
>                         bool audit;
> +                       bool should_report_tid;

For brevity I would call that report_tid, but not insisting.

>                 } fanotify_data;
>  #endif /* CONFIG_FANOTIFY */
>         };
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index 7424791..4eba41c 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -18,6 +18,7 @@
>
>  #define FAN_ONDIR              0x40000000      /* event occurred against dir */
>
> +#define FAN_EVENT_INFO_TID     0x02000000      /* reports the thread id of the event trigger */

That is not the right place nor the sensible value for this flag, but this:

#define FAN_UNLIMITED_QUEUE     0x00000010
#define FAN_UNLIMITED_MARKS     0x00000020
#define FAN_ENABLE_AUDIT        0x00000040
+#define FAN_EVENT_INFO_TID     0x00000080

Thanks,
Amir.