| Message ID | 20180917141950.111841-1-nixiaoming@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fanotify reports the thread id of the event trigger | expand |
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.
On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@gmail.com> wrote: >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. > 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@huawei.com> ....... >> @@ -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. >
On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming <nixiaoming@huawei.com> wrote: > > On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@gmail.com> wrote: > >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@huawei.com> wrote: ... > >> 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 > I strongly agree. Didn't want to impose this change on you, but in fact, I already did that in my own patch set, so you can use my patches as reference: https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190 If you do this you need to separate your change into 2 patches. First make the fanotify_data.flags change collecting the pieces from both my patches. Then make your TID change using fanotify_data.flags. Thanks, Amir.
On Tue, Sep 18, 2018 3:07 PM Amir Goldstein <amir73il@gmail.com> >On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming <nixiaoming@huawei.com> wrote: >> >> On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@gmail.com> wrote: >> >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@huawei.com> wrote: >... >> >> 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 >> > >I strongly agree. Didn't want to impose this change on you, but in >fact, I already did >that in my own patch set, so you can use my patches as reference: >https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 >https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190 > >If you do this you need to separate your change into 2 patches. >First make the fanotify_data.flags change collecting the pieces from both >my patches. >Then make your TID change using fanotify_data.flags. > >Thanks, >Amir. > Should I do this: 1 git cherry-pick 5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 Git cherry-pick 0d1a226f5c18012f19ed6eabfab46f0f125ec190 Manually handle patch conflicts 2 Add a new flag "FAN_EVENT_INFO_TID" into fanotify_data.flags 3 Incorporate "bool audit" into fanotify_data.flags 4 Submit 4 patches thanks
On Tue, Sep 18, 2018 at 12:50 PM Nixiaoming <nixiaoming@huawei.com> wrote: > > On Tue, Sep 18, 2018 3:07 PM Amir Goldstein <amir73il@gmail.com> > >On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming <nixiaoming@huawei.com> wrote: > >> > >> On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@gmail.com> wrote: > >> >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@huawei.com> wrote: > >... > >> >> 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 > >> > > > >I strongly agree. Didn't want to impose this change on you, but in > >fact, I already did > >that in my own patch set, so you can use my patches as reference: > >https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 > >https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190 > > > >If you do this you need to separate your change into 2 patches. > >First make the fanotify_data.flags change collecting the pieces from both > >my patches. > >Then make your TID change using fanotify_data.flags. > > > >Thanks, > >Amir. > > > Should I do this: > 1 git cherry-pick 5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 > Git cherry-pick 0d1a226f5c18012f19ed6eabfab46f0f125ec190 > Manually handle patch conflicts > 2 Add a new flag "FAN_EVENT_INFO_TID" into fanotify_data.flags > 3 Incorporate "bool audit" into fanotify_data.flags > 4 Submit 4 patches > No there is stuff in my patches that is irrelevant. Let me prepare a clean patch for fanotify_data.flags and you can work on top of that. I'll try to post it later today. Thanks, Amir.
On Tue, Sep 18, 2018 at 3:02 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Sep 18, 2018 at 12:50 PM Nixiaoming <nixiaoming@huawei.com> wrote: > > > > On Tue, Sep 18, 2018 3:07 PM Amir Goldstein <amir73il@gmail.com> > > >On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming <nixiaoming@huawei.com> wrote: > > >> > > >> On Mon, Sep 17, 2018 11:51 PM Amir Goldstein <amir73il@gmail.com> wrote: > > >> >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming <nixiaoming@huawei.com> wrote: > > >... > > >> >> 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 > > >> > > > > > >I strongly agree. Didn't want to impose this change on you, but in > > >fact, I already did > > >that in my own patch set, so you can use my patches as reference: > > >https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 > > >https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190 > > > > > >If you do this you need to separate your change into 2 patches. > > >First make the fanotify_data.flags change collecting the pieces from both > > >my patches. > > >Then make your TID change using fanotify_data.flags. > > > > > >Thanks, > > >Amir. > > > > > Should I do this: > > 1 git cherry-pick 5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 > > Git cherry-pick 0d1a226f5c18012f19ed6eabfab46f0f125ec190 > > Manually handle patch conflicts > > 2 Add a new flag "FAN_EVENT_INFO_TID" into fanotify_data.flags > > 3 Incorporate "bool audit" into fanotify_data.flags > > 4 Submit 4 patches > > > > No there is stuff in my patches that is irrelevant. > Let me prepare a clean patch for fanotify_data.flags and you can work > on top of that. > Nixiaoming, I posted the prep patch earlier today. You can also find a slightly modified version of the patch it on this branch with example for adding a new flag (FAN_UNPRIVILEGED): https://github.com/amir73il/linux/commits/fanotify_unpriv Note that my FAN_UNPRIVILEGED adds the flag at 0x80. Not that it matters much, but it would be nice if you add your flag at 0x100 because I have more FAN_EVENT_INFO_ flags in the works, so its nice if we can group them all on byte 3 (at least for start). For your next posting please don't forget to CC linux-api@vger.kernel.org and specify the branch your patch applies to and mention the required prep patch unless you re-send it as a series. Thanks, Amir.
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)) #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; 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; } 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 */ #define FAN_EVENT_ON_CHILD 0x08000000 /* interested in child events */ /* helper events */
In order to identify which thread triggered the event in the multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init 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(-)