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@xxxxxxxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
