| Message ID | 20181202113826.32133-16-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
On Sun 02-12-18 13:38:26, Amir Goldstein wrote: > dirent modification events (create/delete/move) do not carry the > child entry name/inode information. Instead, we report FAN_ONDIR > for mkdir/rmdir so user can differentiate them from creat/unlink. > > For backward compatibility and consistency, do not report FAN_ONDIR > to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > to user in FAN_REPORT_FID mode for all event types. > > Unlike legacy fanotify events (open/access/close), dirent events > for subdir entries (mkdir/rmdir) will be reported regardless if > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > be reported if the user asked for it. > > Cc: <linux-api@vger.kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Some comments below. > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 89c19db4d45f..1aa23cefae5d 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -112,6 +112,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > int data_type) > { > __u32 marks_mask = 0, marks_ignored_mask = 0; > + __u32 test_mask, user_mask = FANOTIFY_EVENT_TYPES; > const struct path *path = data; > struct fsnotify_mark *mark; > int type; > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > marks_ignored_mask |= mark->ignored_mask; > } > > + test_mask = event_mask & marks_mask & ~marks_ignored_mask; > + > + /* > + * dirent modification events (create/delete/move) do not carry the > + * child entry name/inode information. Instead, we report FAN_ONDIR > + * for mkdir/rmdir so user can differentiate them from creat/unlink. > + * > + * For backward compatibility and consistency, do not report FAN_ONDIR > + * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > + * to user in FAN_REPORT_FID mode for all event types. > + */ > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > + /* Do not report FAN_ONDIR without an event type */ > + BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS); > + if (!(test_mask & FANOTIFY_EVENT_TYPES)) > + return 0; > + > + user_mask |= FAN_ONDIR; > + } > + > + /* > + * Unlike legacy fanotify events (open/access/close), dirent events > + * for subdir entries (mkdir/rmdir) will be reported regardless if > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > + * be reported if the user asked for it. > + */ > if (event_mask & FS_ISDIR && > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && I disagree with this. It just seems inconsistent for dirent events for directories to get reported without FAN_ONDIR. I understand there's not great use for not reporting directory dirent events but it's not like adding FAN_ONDIR to the mark mask is that big deal for userspace. And it makes the API more consistent. You could possibly remind the reader in the manpage that FAN_ONDIR is required to get all dirent events. > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > return 0; > > - return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask & > - ~marks_ignored_mask; > + return test_mask & user_mask; The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful I guess. E.g. when implementing recursive watching of a directory. Or what is your intended usecase? It should be said explicitely in the changelog. > static int fanotify_encode_fid(struct fanotify_event *event, > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > index e9d45387089f..f5f86566c277 100644 > --- a/include/linux/fanotify.h > +++ b/include/linux/fanotify.h > @@ -61,13 +61,16 @@ > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > FAN_OPEN_EXEC_PERM) > > +/* Events types that may be reported from vfs */ > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > + FANOTIFY_PERM_EVENTS) > + > /* Extra flags that may be reported with event or control handling of events */ > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > /* Events that may be reported to user */ > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > - FANOTIFY_PERM_EVENTS | \ > - FAN_Q_OVERFLOW) > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > + FAN_Q_OVERFLOW | FAN_ONDIR) > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > FANOTIFY_EVENT_FLAGS) I don't like this renaming. FAN_ONDIR essentially becomes the same type of thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these defines as is... Honza
On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > On Sun 02-12-18 13:38:26, Amir Goldstein wrote: > > dirent modification events (create/delete/move) do not carry the > > child entry name/inode information. Instead, we report FAN_ONDIR > > for mkdir/rmdir so user can differentiate them from creat/unlink. > > > > For backward compatibility and consistency, do not report FAN_ONDIR > > to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > > to user in FAN_REPORT_FID mode for all event types. > > > > Unlike legacy fanotify events (open/access/close), dirent events > > for subdir entries (mkdir/rmdir) will be reported regardless if > > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > be reported if the user asked for it. > > > > Cc: <linux-api@vger.kernel.org> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Some comments below. > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 89c19db4d45f..1aa23cefae5d 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -112,6 +112,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > int data_type) > > { > > __u32 marks_mask = 0, marks_ignored_mask = 0; > > + __u32 test_mask, user_mask = FANOTIFY_EVENT_TYPES; > > const struct path *path = data; > > struct fsnotify_mark *mark; > > int type; > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > marks_ignored_mask |= mark->ignored_mask; > > } > > > > + test_mask = event_mask & marks_mask & ~marks_ignored_mask; > > + > > + /* > > + * dirent modification events (create/delete/move) do not carry the > > + * child entry name/inode information. Instead, we report FAN_ONDIR > > + * for mkdir/rmdir so user can differentiate them from creat/unlink. > > + * > > + * For backward compatibility and consistency, do not report FAN_ONDIR > > + * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > > + * to user in FAN_REPORT_FID mode for all event types. > > + */ > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > > + /* Do not report FAN_ONDIR without an event type */ > > + BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS); > > + if (!(test_mask & FANOTIFY_EVENT_TYPES)) > > + return 0; > > + > > + user_mask |= FAN_ONDIR; > > + } > > + > > + /* > > + * Unlike legacy fanotify events (open/access/close), dirent events > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > + * be reported if the user asked for it. > > + */ > > if (event_mask & FS_ISDIR && > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > I disagree with this. It just seems inconsistent for dirent events for > directories to get reported without FAN_ONDIR. I understand there's not > great use for not reporting directory dirent events but it's not like > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > makes the API more consistent. You could possibly remind the reader in the > manpage that FAN_ONDIR is required to get all dirent events. I see your point. I have no problem with requiring FAN_ONDIR for mkdir events. I believe the strongest argument should be which way is easier to document/understand. Matthew, if you agree that it looks easier to document Jan's proposal, please go a head with this and we will see how man page looks like before making the final decision. Anyway, I will change the logic accordingly for v5 patch set. > > > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > > return 0; > > > > - return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask & > > - ~marks_ignored_mask; > > + return test_mask & user_mask; > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > I guess. E.g. when implementing recursive watching of a directory. Or what > is your intended usecase? It should be said explicitely in the changelog. Recursive watch of directory tree is certainly a use case that could benefit from "mkdir" events. I will add that to commit message. > > > static int fanotify_encode_fid(struct fanotify_event *event, > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > index e9d45387089f..f5f86566c277 100644 > > --- a/include/linux/fanotify.h > > +++ b/include/linux/fanotify.h > > @@ -61,13 +61,16 @@ > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > > FAN_OPEN_EXEC_PERM) > > > > +/* Events types that may be reported from vfs */ > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > + FANOTIFY_PERM_EVENTS) > > + > > /* Extra flags that may be reported with event or control handling of events */ > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > /* Events that may be reported to user */ > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > - FANOTIFY_PERM_EVENTS | \ > > - FAN_Q_OVERFLOW) > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > FANOTIFY_EVENT_FLAGS) > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > defines as is... > Sorry. I don't understand what you mean. FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. Do you not like the new group definition FANOTIFY_EVENT_TYPES? Please explain. Thanks, Amir.
On Fri 04-01-19 13:42:33, Amir Goldstein wrote: > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > > I guess. E.g. when implementing recursive watching of a directory. Or what > > is your intended usecase? It should be said explicitely in the changelog. > > Recursive watch of directory tree is certainly a use case that could benefit > from "mkdir" events. I will add that to commit message. Hum, so it does not seem like you had any particular usecase in mind? :) Before complicating the interface with reporting FAN_ONDIR in some cases I'd prefer we considered whether the usecases are worth it. So let me start that: Reporting FAN_ONDIR can distinguish between file/directory creation/deletion. That can save userspace some rescans of the changed directory if it is interested only in subdirectory creation / deletion (if the application is interested only in file changes it just does not set FAN_ONDIR and that's it). Another motivation is the compatibility with inotify and it's IN_ISDIR flag I guess. Hum, OK, I guess the complication is worth it. > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > > index e9d45387089f..f5f86566c277 100644 > > > --- a/include/linux/fanotify.h > > > +++ b/include/linux/fanotify.h > > > @@ -61,13 +61,16 @@ > > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > > > FAN_OPEN_EXEC_PERM) > > > > > > +/* Events types that may be reported from vfs */ > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > > + FANOTIFY_PERM_EVENTS) > > > + > > > /* Extra flags that may be reported with event or control handling of events */ > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > > > /* Events that may be reported to user */ > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > > - FANOTIFY_PERM_EVENTS | \ > > > - FAN_Q_OVERFLOW) > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > > FANOTIFY_EVENT_FLAGS) > > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > > defines as is... > > > > Sorry. I don't understand what you mean. > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. > Do you not like the new group definition FANOTIFY_EVENT_TYPES? Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported to userspace. I don't like the FANOTIFY_EVENT_TYPES name and FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR there. So how about renaming FANOTIFY_OUTGOING_EVENTS to FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your FANOTIFY_EVENT_TYPES is? Honza
On Fri 04-01-19 13:42:33, Amir Goldstein wrote: > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > > marks_ignored_mask |= mark->ignored_mask; > > > } > > > > > > + test_mask = event_mask & marks_mask & ~marks_ignored_mask; > > > + > > > + /* > > > + * dirent modification events (create/delete/move) do not carry the > > > + * child entry name/inode information. Instead, we report FAN_ONDIR > > > + * for mkdir/rmdir so user can differentiate them from creat/unlink. > > > + * > > > + * For backward compatibility and consistency, do not report FAN_ONDIR > > > + * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > > > + * to user in FAN_REPORT_FID mode for all event types. > > > + */ > > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > > > + /* Do not report FAN_ONDIR without an event type */ > > > + BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS); > > > + if (!(test_mask & FANOTIFY_EVENT_TYPES)) > > > + return 0; > > > + > > > + user_mask |= FAN_ONDIR; > > > + } > > > + > > > + /* > > > + * Unlike legacy fanotify events (open/access/close), dirent events > > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > + * be reported if the user asked for it. > > > + */ > > > if (event_mask & FS_ISDIR && > > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > > > I disagree with this. It just seems inconsistent for dirent events for > > directories to get reported without FAN_ONDIR. I understand there's not > > great use for not reporting directory dirent events but it's not like > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > > makes the API more consistent. You could possibly remind the reader in the > > manpage that FAN_ONDIR is required to get all dirent events. > > I see your point. > I have no problem with requiring FAN_ONDIR for mkdir events. > I believe the strongest argument should be which way is easier > to document/understand. > > Matthew, if you agree that it looks easier to document Jan's proposal, > please go a head with this and we will see how man page looks like > before making the final decision. Agreed. Honza
On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 04-01-19 13:42:33, Amir Goldstein wrote: > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > > > I guess. E.g. when implementing recursive watching of a directory. Or what > > > is your intended usecase? It should be said explicitely in the changelog. > > > > Recursive watch of directory tree is certainly a use case that could benefit > > from "mkdir" events. I will add that to commit message. > > Hum, so it does not seem like you had any particular usecase in mind? :) Our application does have special handling (scanning) for mkdir events. > Before complicating the interface with reporting FAN_ONDIR in some cases > I'd prefer we considered whether the usecases are worth it. So let me start > that: Reporting FAN_ONDIR can distinguish between file/directory > creation/deletion. That can save userspace some rescans of the changed > directory if it is interested only in subdirectory creation / deletion (if > the application is interested only in file changes it just does not set > FAN_ONDIR and that's it). > > Another motivation is the compatibility with inotify and it's IN_ISDIR flag > I guess. > > Hum, OK, I guess the complication is worth it. > Let's see. The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID is set, is something that needs to be clarified. The fact that reported FID refers to the modified directory in dirent events but FAN_ONDIR flag refers to the created/removed/renamed child is another thing that needs to be clarified. Sigh! "things that need to be clarified" are things we need to avoid. Therefore, I am going to make another suggestion. How about if we introduced a new flag FAN_DIRENT_ISDIR? Like IN_ISDIR, it is an out-only flag that cannot be requested. As its name suggests, it is only applicable to dirent events, so will always be set when a sub directory entry is created/renamed/removed. FAN_ONDIR has no effect on dirent events, because FAN_ONDIR refers to the "victim" inode and the "victim" is the modified directory. If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user wants to get notified when *directories* are modified and specifying FAN_ONDIR explicitly is not needed. How about that? > > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > > > index e9d45387089f..f5f86566c277 100644 > > > > --- a/include/linux/fanotify.h > > > > +++ b/include/linux/fanotify.h > > > > @@ -61,13 +61,16 @@ > > > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > > > > FAN_OPEN_EXEC_PERM) > > > > > > > > +/* Events types that may be reported from vfs */ > > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > > > + FANOTIFY_PERM_EVENTS) > > > > + > > > > /* Extra flags that may be reported with event or control handling of events */ > > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > > > > > /* Events that may be reported to user */ > > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > > > - FANOTIFY_PERM_EVENTS | \ > > > > - FAN_Q_OVERFLOW) > > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > > > FANOTIFY_EVENT_FLAGS) > > > > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > > > defines as is... > > > > > > > Sorry. I don't understand what you mean. > > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS > > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. > > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. > > Do you not like the new group definition FANOTIFY_EVENT_TYPES? > > Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported > to userspace. I don't like the FANOTIFY_EVENT_TYPES name and > FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR > there. So how about renaming FANOTIFY_OUTGOING_EVENTS to > FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your > FANOTIFY_EVENT_TYPES is? > Still a bit confusing to me. I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR I can do without the FANOTIFY_EVENT_TYPES group. something like this: #define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) #define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR) #define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ FANOTIFY_PERM_EVENTS | \ FANOTIFY_EVENT_OUT_FLAGS) ... test_mask = event_mask & marks_mask & ~marks_ignored_mask; if ((event_mask & FS_ISDIR) && (test_mask & ALL_FSNOTIFY_DIRENT_EVENTS)) test_mask |= FAN_DIRENT_ISDIR; return test_mask & FANOTIFY_OUTGOING_EVENTS; Thanks, Amir.
On Fri, Jan 04, 2019 at 01:42:33PM +0200, Amir Goldstein wrote: > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > > > On Sun 02-12-18 13:38:26, Amir Goldstein wrote: > > > dirent modification events (create/delete/move) do not carry the > > > child entry name/inode information. Instead, we report FAN_ONDIR > > > for mkdir/rmdir so user can differentiate them from creat/unlink. > > > > > > For backward compatibility and consistency, do not report FAN_ONDIR > > > to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > > > to user in FAN_REPORT_FID mode for all event types. > > > > > > Unlike legacy fanotify events (open/access/close), dirent events > > > for subdir entries (mkdir/rmdir) will be reported regardless if > > > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > be reported if the user asked for it. > > > > > > Cc: <linux-api@vger.kernel.org> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > Some comments below. > > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index 89c19db4d45f..1aa23cefae5d 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -112,6 +112,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > > int data_type) > > > { > > > __u32 marks_mask = 0, marks_ignored_mask = 0; > > > + __u32 test_mask, user_mask = FANOTIFY_EVENT_TYPES; > > > const struct path *path = data; > > > struct fsnotify_mark *mark; > > > int type; > > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > > marks_ignored_mask |= mark->ignored_mask; > > > } > > > > > > + test_mask = event_mask & marks_mask & ~marks_ignored_mask; > > > + > > > + /* > > > + * dirent modification events (create/delete/move) do not carry the > > > + * child entry name/inode information. Instead, we report FAN_ONDIR > > > + * for mkdir/rmdir so user can differentiate them from creat/unlink. > > > + * > > > + * For backward compatibility and consistency, do not report FAN_ONDIR > > > + * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR > > > + * to user in FAN_REPORT_FID mode for all event types. > > > + */ > > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > > > + /* Do not report FAN_ONDIR without an event type */ > > > + BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS); > > > + if (!(test_mask & FANOTIFY_EVENT_TYPES)) > > > + return 0; > > > + > > > + user_mask |= FAN_ONDIR; > > > + } > > > + > > > + /* > > > + * Unlike legacy fanotify events (open/access/close), dirent events > > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > + * be reported if the user asked for it. > > > + */ > > > if (event_mask & FS_ISDIR && > > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > > > I disagree with this. It just seems inconsistent for dirent events for > > directories to get reported without FAN_ONDIR. I understand there's not > > great use for not reporting directory dirent events but it's not like > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > > makes the API more consistent. You could possibly remind the reader in the > > manpage that FAN_ONDIR is required to get all dirent events. > > I see your point. > I have no problem with requiring FAN_ONDIR for mkdir events. > I believe the strongest argument should be which way is easier > to document/understand. > > Matthew, if you agree that it looks easier to document Jan's proposal, > please go a head with this and we will see how man page looks like > before making the final decision. To be fair, for the sake of clarity and consistency with the existing API I do believe it would make it easier for the API consumer to comprehend what Jan has suggested. Simple, in order to receive any events of type dirent, one must supply FAN_ONDIR as part of their mark mask. > > > > > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > > > return 0; > > > > > > - return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask & > > > - ~marks_ignored_mask; > > > + return test_mask & user_mask; > > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > > I guess. E.g. when implementing recursive watching of a directory. Or what > > is your intended usecase? It should be said explicitely in the changelog. > > Recursive watch of directory tree is certainly a use case that could benefit > from "mkdir" events. I will add that to commit message. > > > > > > static int fanotify_encode_fid(struct fanotify_event *event, > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > > index e9d45387089f..f5f86566c277 100644 > > > --- a/include/linux/fanotify.h > > > +++ b/include/linux/fanotify.h > > > @@ -61,13 +61,16 @@ > > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > > > FAN_OPEN_EXEC_PERM) > > > > > > +/* Events types that may be reported from vfs */ > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > > + FANOTIFY_PERM_EVENTS) > > > + > > > /* Extra flags that may be reported with event or control handling of events */ > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > > > /* Events that may be reported to user */ > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > > - FANOTIFY_PERM_EVENTS | \ > > > - FAN_Q_OVERFLOW) > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > > FANOTIFY_EVENT_FLAGS) > > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > > defines as is... > > > > Sorry. I don't understand what you mean. > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. > Do you not like the new group definition FANOTIFY_EVENT_TYPES? > > Please explain.
On Fri, Jan 04, 2019 at 02:39:06PM +0200, Amir Goldstein wrote: > On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote: > > > > On Fri 04-01-19 13:42:33, Amir Goldstein wrote: > > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > > > > I guess. E.g. when implementing recursive watching of a directory. Or what > > > > is your intended usecase? It should be said explicitely in the changelog. > > > > > > Recursive watch of directory tree is certainly a use case that could benefit > > > from "mkdir" events. I will add that to commit message. > > > > Hum, so it does not seem like you had any particular usecase in mind? :) > > Our application does have special handling (scanning) for mkdir events. > > > Before complicating the interface with reporting FAN_ONDIR in some cases > > I'd prefer we considered whether the usecases are worth it. So let me start > > that: Reporting FAN_ONDIR can distinguish between file/directory > > creation/deletion. That can save userspace some rescans of the changed > > directory if it is interested only in subdirectory creation / deletion (if > > the application is interested only in file changes it just does not set > > FAN_ONDIR and that's it). > > > > Another motivation is the compatibility with inotify and it's IN_ISDIR flag > > I guess. > > > > Hum, OK, I guess the complication is worth it. > > > > Let's see. > > The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID > is set, is something that needs to be clarified. > > The fact that reported FID refers to the modified directory in dirent events > but FAN_ONDIR flag refers to the created/removed/renamed child is > another thing that needs to be clarified. > > Sigh! "things that need to be clarified" are things we need to avoid. > Therefore, I am going to make another suggestion. > > How about if we introduced a new flag FAN_DIRENT_ISDIR? > Like IN_ISDIR, it is an out-only flag that cannot be requested. > As its name suggests, it is only applicable to dirent events, so will always > be set when a sub directory entry is created/renamed/removed. > > FAN_ONDIR has no effect on dirent events, because FAN_ONDIR > refers to the "victim" inode and the "victim" is the modified directory. > If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user > wants to get notified when *directories* are modified and specifying > FAN_ONDIR explicitly is not needed. > > How about that? I think this could work. However to me, and if I'm understanding this correctly, it looks like we're going down the route of adding "bonus" flags to particular events, which I thought was something that we wanted to try and avoid in future versions of the API? This would be so that users don't get any sudden surprises when processing their events. It would also go against the notion of only receiving events that were explicitly requested for by the calling process, which was something that we discussed and implemented in a previous patch series? > > > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > > > > index e9d45387089f..f5f86566c277 100644 > > > > > --- a/include/linux/fanotify.h > > > > > +++ b/include/linux/fanotify.h > > > > > @@ -61,13 +61,16 @@ > > > > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ > > > > > FAN_OPEN_EXEC_PERM) > > > > > > > > > > +/* Events types that may be reported from vfs */ > > > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > > > > + FANOTIFY_PERM_EVENTS) > > > > > + > > > > > /* Extra flags that may be reported with event or control handling of events */ > > > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > > > > > > > /* Events that may be reported to user */ > > > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > > > > - FANOTIFY_PERM_EVENTS | \ > > > > > - FAN_Q_OVERFLOW) > > > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > > > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > > > > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > > > > FANOTIFY_EVENT_FLAGS) > > > > > > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > > > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > > > > defines as is... > > > > > > > > > > Sorry. I don't understand what you mean. > > > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS > > > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. > > > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. > > > Do you not like the new group definition FANOTIFY_EVENT_TYPES? > > > > Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported > > to userspace. I don't like the FANOTIFY_EVENT_TYPES name and > > FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR > > there. So how about renaming FANOTIFY_OUTGOING_EVENTS to > > FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your > > FANOTIFY_EVENT_TYPES is? > > > > Still a bit confusing to me. > I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR > I can do without the FANOTIFY_EVENT_TYPES group. > something like this: > > #define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > #define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR) > #define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > FANOTIFY_PERM_EVENTS | \ > FANOTIFY_EVENT_OUT_FLAGS) > > ... > test_mask = event_mask & marks_mask & ~marks_ignored_mask; > > if ((event_mask & FS_ISDIR) && > (test_mask & ALL_FSNOTIFY_DIRENT_EVENTS)) > test_mask |= FAN_DIRENT_ISDIR; > > return test_mask & FANOTIFY_OUTGOING_EVENTS;
... > > > > + /* > > > > + * Unlike legacy fanotify events (open/access/close), dirent events > > > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > > + * be reported if the user asked for it. > > > > + */ > > > > if (event_mask & FS_ISDIR && > > > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > > > > > I disagree with this. It just seems inconsistent for dirent events for > > > directories to get reported without FAN_ONDIR. I understand there's not > > > great use for not reporting directory dirent events but it's not like > > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > > > makes the API more consistent. You could possibly remind the reader in the > > > manpage that FAN_ONDIR is required to get all dirent events. > > > > I see your point. > > I have no problem with requiring FAN_ONDIR for mkdir events. > > I believe the strongest argument should be which way is easier > > to document/understand. > > > > Matthew, if you agree that it looks easier to document Jan's proposal, > > please go a head with this and we will see how man page looks like > > before making the final decision. > > To be fair, for the sake of clarity and consistency with the existing API I do > believe it would make it easier for the API consumer to comprehend what Jan has > suggested. Simple, in order to receive any events of type dirent, one must > supply FAN_ONDIR as part of their mark mask. > But that was not the suggestion. The debate is whether or not user needs to specify (for example) FAN_ONDIR|FAN_CREATE in order to get mkdir events. The three of us understanding FAN_ONDIR intuitively different is what makes me unease. The purpose of my alternative suggestion was to dis-disambiguate which inode each flag refers to. It should be clear that FAN_DIRENT_ISDIR does not refer to the modified directry but to the created/deleted/renamed subdir. We will avoid making a change of behavior making FAN_ONDIR an out flag. Thanks, Amir.
On Sat, Jan 5, 2019 at 2:34 AM Matthew Bobrowski <mbobrowski@mbobrowski.org> wrote: > > On Fri, Jan 04, 2019 at 02:39:06PM +0200, Amir Goldstein wrote: > > On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Fri 04-01-19 13:42:33, Amir Goldstein wrote: > > > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote: > > > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful > > > > > I guess. E.g. when implementing recursive watching of a directory. Or what > > > > > is your intended usecase? It should be said explicitely in the changelog. > > > > > > > > Recursive watch of directory tree is certainly a use case that could benefit > > > > from "mkdir" events. I will add that to commit message. > > > > > > Hum, so it does not seem like you had any particular usecase in mind? :) > > > > Our application does have special handling (scanning) for mkdir events. > > > > > Before complicating the interface with reporting FAN_ONDIR in some cases > > > I'd prefer we considered whether the usecases are worth it. So let me start > > > that: Reporting FAN_ONDIR can distinguish between file/directory > > > creation/deletion. That can save userspace some rescans of the changed > > > directory if it is interested only in subdirectory creation / deletion (if > > > the application is interested only in file changes it just does not set > > > FAN_ONDIR and that's it). > > > > > > Another motivation is the compatibility with inotify and it's IN_ISDIR flag > > > I guess. > > > > > > Hum, OK, I guess the complication is worth it. > > > > > > > Let's see. > > > > The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID > > is set, is something that needs to be clarified. > > > > The fact that reported FID refers to the modified directory in dirent events > > but FAN_ONDIR flag refers to the created/removed/renamed child is > > another thing that needs to be clarified. > > > > Sigh! "things that need to be clarified" are things we need to avoid. > > Therefore, I am going to make another suggestion. > > > > How about if we introduced a new flag FAN_DIRENT_ISDIR? > > Like IN_ISDIR, it is an out-only flag that cannot be requested. > > As its name suggests, it is only applicable to dirent events, so will always > > be set when a sub directory entry is created/renamed/removed. > > > > FAN_ONDIR has no effect on dirent events, because FAN_ONDIR > > refers to the "victim" inode and the "victim" is the modified directory. > > If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user > > wants to get notified when *directories* are modified and specifying > > FAN_ONDIR explicitly is not needed. > > > > How about that? > > I think this could work. > > However to me, and if I'm understanding this correctly, it looks like we're > going down the route of adding "bonus" flags to particular events, which I > thought was something that we wanted to try and avoid in future versions of the > API? This would be so that users don't get any sudden surprises when processing > their events. It would also go against the notion of only receiving events that > were explicitly requested for by the calling process, which was something that > we discussed and implemented in a previous patch series? > This is a bit different case than FAN_OPEN_EXEC, because I wasn't suggesting to let the user opt-in/out of mkdir events. There is nothing wrong with "out event flags" as they do exist in inotify. If we want to take FAN_OPEN_EXEC as an example, we could define completely new set of events FAN_SUBDIR_{CREATE,DELETE,MOVE} but IMO that is not needed and not consistent with what users are accustomed for from inotify. We don't need to stick by inotify always, but if we don't have a good reason to differentiate from it, we should avoid that IMO. There is another option that does not sound very alluring to me, but I'll write it anyway for your consideration: -#define FAN_ONDIR 0x40000000 /* event occurred against dir */ +#define FAN_ISDIR 0x20000000 /* event occurred against dir */ +#define FAN_ONDIR 0x40000000 /* interested in events against dir */ #define FAN_EVENT_ON_CHILD 0x08000000 /* interested in child events */ FAN_ONDIR remains in-only flag and keeps the same meaning. FAN_ISDIR is an in/out flag for all events (not just dirent events) and preserves the same meaning as it has in inotify (refers to dirent OR to victim depending on the event type). Technically, FAN_ISDIR implies FAN_ONDIR, but *also* carries the information in out event, like in inotify. Food for though... Thanks, Amir.
On Sat, Jan 05, 2019 at 09:59:42AM +0200, Amir Goldstein wrote: > ... > > > > > + /* > > > > > + * Unlike legacy fanotify events (open/access/close), dirent events > > > > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > > > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > > > + * be reported if the user asked for it. > > > > > + */ > > > > > if (event_mask & FS_ISDIR && > > > > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > > > > > > > I disagree with this. It just seems inconsistent for dirent events for > > > > directories to get reported without FAN_ONDIR. I understand there's not > > > > great use for not reporting directory dirent events but it's not like > > > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > > > > makes the API more consistent. You could possibly remind the reader in the > > > > manpage that FAN_ONDIR is required to get all dirent events. > > > > > > I see your point. > > > I have no problem with requiring FAN_ONDIR for mkdir events. > > > I believe the strongest argument should be which way is easier > > > to document/understand. > > > > > > Matthew, if you agree that it looks easier to document Jan's proposal, > > > please go a head with this and we will see how man page looks like > > > before making the final decision. > > > > To be fair, for the sake of clarity and consistency with the existing API I do > > believe it would make it easier for the API consumer to comprehend what Jan has > > suggested. Simple, in order to receive any events of type dirent, one must > > supply FAN_ONDIR as part of their mark mask. > > > > But that was not the suggestion. > > The debate is whether or not user needs to specify (for example) > FAN_ONDIR|FAN_CREATE in order to get mkdir events. And I'm agreeing with the fact that I think this ^ i.e. FAN_ONDIR | FAN_CREATE is the way to go moving forward. However, there is still a small part of me that thinks doing it this way seems a little weird and solely supplying FAN_CREATE for example should be sufficient in order to get these type of dirent events. I don't know why, but for whatever reason I have a feeling of uncertainty about this. > The three of us understanding FAN_ONDIR intuitively different is what makes > me unease. > > The purpose of my alternative suggestion was to dis-disambiguate which inode > each flag refers to. > > It should be clear that FAN_DIRENT_ISDIR does not refer to the modified > directry but to the created/deleted/renamed subdir. > We will avoid making a change of behavior making FAN_ONDIR an out flag. Yeah, so maybe using FAN_DIRENT_ISDIR is indeed the solution. I don't really have any objections with it at this particular point. Let's see whether Jan has looked at it from a different perspective and can share his opinion.
On Sat, Jan 5, 2019 at 11:49 AM Matthew Bobrowski <mbobrowski@mbobrowski.org> wrote: > > On Sat, Jan 05, 2019 at 09:59:42AM +0200, Amir Goldstein wrote: > > ... > > > > > > + /* > > > > > > + * Unlike legacy fanotify events (open/access/close), dirent events > > > > > > + * for subdir entries (mkdir/rmdir) will be reported regardless if > > > > > > + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only > > > > > > + * be reported if the user asked for it. > > > > > > + */ > > > > > > if (event_mask & FS_ISDIR && > > > > > > + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && > > > > > > > > > > I disagree with this. It just seems inconsistent for dirent events for > > > > > directories to get reported without FAN_ONDIR. I understand there's not > > > > > great use for not reporting directory dirent events but it's not like > > > > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it > > > > > makes the API more consistent. You could possibly remind the reader in the > > > > > manpage that FAN_ONDIR is required to get all dirent events. > > > > > > > > I see your point. > > > > I have no problem with requiring FAN_ONDIR for mkdir events. > > > > I believe the strongest argument should be which way is easier > > > > to document/understand. > > > > > > > > Matthew, if you agree that it looks easier to document Jan's proposal, > > > > please go a head with this and we will see how man page looks like > > > > before making the final decision. > > > > > > To be fair, for the sake of clarity and consistency with the existing API I do > > > believe it would make it easier for the API consumer to comprehend what Jan has > > > suggested. Simple, in order to receive any events of type dirent, one must > > > supply FAN_ONDIR as part of their mark mask. > > > > > > > But that was not the suggestion. > > > > The debate is whether or not user needs to specify (for example) > > FAN_ONDIR|FAN_CREATE in order to get mkdir events. > > And I'm agreeing with the fact that I think this ^ i.e. FAN_ONDIR | FAN_CREATE > is the way to go moving forward. However, there is still a small part of me > that thinks doing it this way seems a little weird and solely supplying > FAN_CREATE for example should be sufficient in order to get these type of > dirent events. I don't know why, but for whatever reason I have a feeling of > uncertainty about this. > I think that is sufficient consensus for requiring FAN_ONDIR to get mkdir/rmdir events, so this is what I will do. As Jan wrote, it's quite easy to document this weirdness and ease of documentation is the important thing. > > The three of us understanding FAN_ONDIR intuitively different is what makes > > me unease. > > > > The purpose of my alternative suggestion was to dis-disambiguate which inode > > each flag refers to. > > > > It should be clear that FAN_DIRENT_ISDIR does not refer to the modified > > directry but to the created/deleted/renamed subdir. > > We will avoid making a change of behavior making FAN_ONDIR an out flag. > > Yeah, so maybe using FAN_DIRENT_ISDIR is indeed the solution. I don't really > have any objections with it at this particular point. Let's see whether Jan has > looked at it from a different perspective and can share his opinion. > Nah. It's going to complicated rather than clarify IMO. Just need to document that FAN_ONDIR is reported only with FAN_REPORT_FID. If that is too weird, we can propose a new explicit init flag FAN_REPORT_ONDIR. Do you guys thing that is necessary? Thanks, Amir.
> > > > +/* Events types that may be reported from vfs */ > > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ > > > > + FANOTIFY_PERM_EVENTS) > > > > + > > > > /* Extra flags that may be reported with event or control handling of events */ > > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) > > > > > > > > /* Events that may be reported to user */ > > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ > > > > - FANOTIFY_PERM_EVENTS | \ > > > > - FAN_Q_OVERFLOW) > > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ > > > > + FAN_Q_OVERFLOW | FAN_ONDIR) > > > > > > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ > > > > FANOTIFY_EVENT_FLAGS) > > > > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of > > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these > > > defines as is... > > > > > > > Sorry. I don't understand what you mean. > > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS > > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change. > > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS. > > Do you not like the new group definition FANOTIFY_EVENT_TYPES? > > Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported > to userspace. I don't like the FANOTIFY_EVENT_TYPES name and > FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR > there. So how about renaming FANOTIFY_OUTGOING_EVENTS to > FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your > FANOTIFY_EVENT_TYPES is? > Jan, I decided to avert this specific bikeshed and my updated version does not use any new defined. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 89c19db4d45f..1aa23cefae5d 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -112,6 +112,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, int data_type) { __u32 marks_mask = 0, marks_ignored_mask = 0; + __u32 test_mask, user_mask = FANOTIFY_EVENT_TYPES; const struct path *path = data; struct fsnotify_mark *mark; int type; @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, marks_ignored_mask |= mark->ignored_mask; } + test_mask = event_mask & marks_mask & ~marks_ignored_mask; + + /* + * dirent modification events (create/delete/move) do not carry the + * child entry name/inode information. Instead, we report FAN_ONDIR + * for mkdir/rmdir so user can differentiate them from creat/unlink. + * + * For backward compatibility and consistency, do not report FAN_ONDIR + * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR + * to user in FAN_REPORT_FID mode for all event types. + */ + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { + /* Do not report FAN_ONDIR without an event type */ + BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS); + if (!(test_mask & FANOTIFY_EVENT_TYPES)) + return 0; + + user_mask |= FAN_ONDIR; + } + + /* + * Unlike legacy fanotify events (open/access/close), dirent events + * for subdir entries (mkdir/rmdir) will be reported regardless if + * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only + * be reported if the user asked for it. + */ if (event_mask & FS_ISDIR && + !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) && !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) return 0; - return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask & - ~marks_ignored_mask; + return test_mask & user_mask; } static int fanotify_encode_fid(struct fanotify_event *event, diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h index e9d45387089f..f5f86566c277 100644 --- a/include/linux/fanotify.h +++ b/include/linux/fanotify.h @@ -61,13 +61,16 @@ #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ FAN_OPEN_EXEC_PERM) +/* Events types that may be reported from vfs */ +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \ + FANOTIFY_PERM_EVENTS) + /* Extra flags that may be reported with event or control handling of events */ #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR) /* Events that may be reported to user */ -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \ - FANOTIFY_PERM_EVENTS | \ - FAN_Q_OVERFLOW) +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \ + FAN_Q_OVERFLOW | FAN_ONDIR) #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \ FANOTIFY_EVENT_FLAGS)
dirent modification events (create/delete/move) do not carry the child entry name/inode information. Instead, we report FAN_ONDIR for mkdir/rmdir so user can differentiate them from creat/unlink. For backward compatibility and consistency, do not report FAN_ONDIR to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR to user in FAN_REPORT_FID mode for all event types. Unlike legacy fanotify events (open/access/close), dirent events for subdir entries (mkdir/rmdir) will be reported regardless if user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only be reported if the user asked for it. Cc: <linux-api@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 31 +++++++++++++++++++++++++++++-- include/linux/fanotify.h | 9 ++++++--- 2 files changed, 35 insertions(+), 5 deletions(-)