Re: [PATCH v2] fanotify reports the thread id of the event trigger

[Amir Goldstein]

On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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.