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

[Nixiaoming]

On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>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.
>
Thank you for your guidance, I will correct it in a later patch.

>> to select whether to report the event creator's thread id information.
>>
>> Signed-off-by: nixiaoming <nixiaoming@xxxxxxxxxx>
.......
>> @@ -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.
Thank you for your guidance, I will correct it in a later patch.

>
>>  #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.
>

Whether it is better to change to "unsigned int flags"
Save "group->fanotify_data.flags=flags;" in "fanotify_init"
Determine whether "group->fanotify_data.flags" contains "FAN_EVENT_INFO_TID" in "fanotify_alloc_event";

At the same time, if there are other flags that need to be used later, there is no need to add new members.

By the way, whether or not "bool audit" can also be included by flags

thanks


>>                 } 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.
>