Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

Commit Message

Xiaoming Ni Sept. 11, 2018, 6:51 a.m. UTC

Inotify api cannot display information about users and processes.
That is, you can only know that the file event is generated, but you don't know who triggered the event, which is not conducive to fault location.
Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

Example:

Comments

Amir Goldstein Sept. 11, 2018, 3:11 p.m. UTC | #1

On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming <[e-mail]> wrote:
>
> Inotify api cannot display information about users and processes.
> That is, you can only know that the file event is generated, but you don't know who triggered the event, which is not conducive to fault location.
> Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?
>

"Is it possible?" is not the only relevant question.
I suppose your patch can sort of works, but it exposes information to
potentially unpriveleged
processes, even exposes pid values outside of the process pid namespace.

While those issues could be addressed, you can't change the format
struct inotify_event
without breaking existing applications.

I guess you are not using fanotify API, which already provides pid
information (albiet tgid),
because it lacks other functionality that you need? Which
functionality might that be?
Is it directory modification events?
If so than you might be interested in my effort to add support for
those events to fanotify:
https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch

Your support, should you choose to offer it, could be in the form of
testing patches
and/or just by putting forward your use case as an example for the
need of an extended
fanotify API.

Thanks,
Amir.

Xiaoming Ni Sept. 13, 2018, 11:25 a.m. UTC | #2

On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein <[e-mail]> wrote:
>On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming <[e-mail]> wrote:
>>
>> Inotify api cannot display information about users and processes.
>> That is, you can only know that the file event is generated, but you don't know who triggered the event, which is not conducive to fault location.
>> Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?
>>
>
>"Is it possible?" is not the only relevant question.
>I suppose your patch can sort of works, but it exposes information to
>potentially unpriveleged
>processes, even exposes pid values outside of the process pid namespace.
>
>While those issues could be addressed, you can't change the format
>struct inotify_event
>without breaking existing applications.
>
In order to improve the fault location capability, can we make incompatible interface changes?

>I guess you are not using fanotify API, which already provides pid
>information (albiet tgid),
>because it lacks other functionality that you need? Which
>functionality might that be?
>Is it directory modification events?
>If so than you might be interested in my effort to add support for
>those events to fanotify:
>https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>
The fanotify API does not support monitoring file deletion events
The fanotify API supports tgid display,
but for multi-threaded programs,
it still cannot accurately identify which thread triggered the event.
Can I modify tgid to pid?
- event->tgid = get_pid(task_tgid(current));
+ event->tgid = get_pid(task_pid(current));

>Your support, should you choose to offer it, could be in the form of
>testing patches
>and/or just by putting forward your use case as an example for the
>need of an extended
>fanotify API.
>
>Thanks,
>Amir.
>

Thanks

Amir Goldstein Sept. 13, 2018, 12:31 p.m. UTC | #3

On Thu, Sep 13, 2018 at 2:25 PM Nixiaoming <[e-mail]> wrote:
>
> On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein <[e-mail]> wrote:
> >On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming <[e-mail]> wrote:
> >>
> >> Inotify api cannot display information about users and processes.
> >> That is, you can only know that the file event is generated, but you don't know who triggered the event, which is not conducive to fault location.
> >> Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?
> >>
> >
> >"Is it possible?" is not the only relevant question.
> >I suppose your patch can sort of works, but it exposes information to
> >potentially unpriveleged
> >processes, even exposes pid values outside of the process pid namespace.
> >
> >While those issues could be addressed, you can't change the format
> >struct inotify_event
> >without breaking existing applications.
> >
> In order to improve the fault location capability, can we make incompatible interface changes?

Not unless application/sysadmin/distro opts-in for the incompatible change.

>
> >I guess you are not using fanotify API, which already provides pid
> >information (albiet tgid),
> >because it lacks other functionality that you need? Which
> >functionality might that be?
> >Is it directory modification events?
> >If so than you might be interested in my effort to add support for
> >those events to fanotify:
> >https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
> >
> The fanotify API does not support monitoring file deletion events

Yes, I am working toward that goal.

> The fanotify API supports tgid display,
> but for multi-threaded programs,
> it still cannot accurately identify which thread triggered the event.
> Can I modify tgid to pid?
> - event->tgid = get_pid(task_tgid(current));
> + event->tgid = get_pid(task_pid(current));
>

So if you would like to change that you need to add a new flag to
fanotify_init (e.g. FAN_EVENT_INFO_TID)
new applications that would opt-in for the flag will get task_pid()
while existing application will keep getting task_tgid()
new applications will get -EINVAL when passing FAN_EVENT_INFO_TID
to fanotify_init() on an old kernel and they could then fall back to getting
tgid in events and be aware of that fact.

Thanks,
Amir.

Xiaoming Ni Sept. 15, 2018, 1:13 p.m. UTC | #4

on Thu, Sep 13, 2018 at 8:32 PM Amir Goldstein <[e-mail]>wrote:
>On Thu, Sep 13, 2018 at 2:25 PM Nixiaoming <[e-mail]> wrote:
>>
>> On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein <[e-mail]> wrote:
>> >On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming <[e-mail]> wrote:
>> >>
>> >> Inotify api cannot display information about users and processes.
>> >> That is, you can only know that the file event is generated, but you don't know who triggered the event, which is not conducive to fault location.
>> >> Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?
>> >>
>> >
>> >"Is it possible?" is not the only relevant question.
>> >I suppose your patch can sort of works, but it exposes information to
>> >potentially unpriveleged
>> >processes, even exposes pid values outside of the process pid namespace.
>> >
>> >While those issues could be addressed, you can't change the format
>> >struct inotify_event
>> >without breaking existing applications.
>> >
>> In order to improve the fault location capability, can we make incompatible interface changes?
>
>Not unless application/sysadmin/distro opts-in for the incompatible change.
>
>>
>> >I guess you are not using fanotify API, which already provides pid
>> >information (albiet tgid),
>> >because it lacks other functionality that you need? Which
>> >functionality might that be?
>> >Is it directory modification events?
>> >If so than you might be interested in my effort to add support for
>> >those events to fanotify:
>> >https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>> >
>> The fanotify API does not support monitoring file deletion events
>
>Yes, I am working toward that goal.
>
>> The fanotify API supports tgid display,
>> but for multi-threaded programs,
>> it still cannot accurately identify which thread triggered the event.
>> Can I modify tgid to pid?
>> - event->tgid = get_pid(task_tgid(current));
>> + event->tgid = get_pid(task_pid(current));
>>
>
>So if you would like to change that you need to add a new flag to
>fanotify_init (e.g. FAN_EVENT_INFO_TID)
>new applications that would opt-in for the flag will get task_pid()
>while existing application will keep getting task_tgid()
>new applications will get -EINVAL when passing FAN_EVENT_INFO_TID
>to fanotify_init() on an old kernel and they could then fall back to getting
>tgid in events and be aware of that fact.
>
>Thanks,
>Amir.
>

Thank you for your guidance
I am modifying the code test locally,
The kernel mode selects whether to display the thread id information 
according to the FAN_EVENT_INFO_TID tag when fanotify_alloc_event is called 
the user mode test program adds FAN_EVENT_INFO_TID to the fanotify_markd parameter mask,  
now, it can accurately display which thread triggered the event.

I will send the patch later, please help me to review it

thanks

Patch

diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index 7e4578d..be91844 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -7,6 +7,8 @@  struct inotify_event_info {
        struct fsnotify_event fse;
        int wd;
        u32 sync_cookie;
+ int pid;
+ char comm[TASK_COMM_LEN];
        int name_len;
        char name[];
 };
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index f4184b4..f7ad298 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -117,6 +117,8 @@  int inotify_handle_event(struct fsnotify_group *group,
        fsnotify_init_event(fsn_event, inode, mask);
        event->wd = i_mark->wd;
        event->sync_cookie = cookie;
+ event->pid = current->pid;
+ strncpy(event->comm, current->comm, TASK_COMM_LEN);
        event->name_len = len;
        if (len)
                strcpy(event->name, file_name);