Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask
From: Jan Kara <jack@suse.cz>
Date: 2022-06-22 15:54:29
Also in:
linux-fsdevel
On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect. The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and ignore mask is always implicitly applied to events on directories. Define a mark flag that replaces this legacy behavior with logic of applying the ignore mask according to event flags in ignore mask. Implement the new logic to prepare for supporting an ignore mask that ignores events on children and ignore mask that does not ignore events on directories. To emphasize the change in terminology, also rename ignored_mask mark member to ignore_mask and use accessor to get only ignored events or events and flags. This change in terminology finally aligns with the "ignore mask" language in man pages and in most of the comments. Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Looks mostly good to me. Just one question / suggestion: You are introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events(). So shouldn't we be using these helpers as much as possible throughout the code? Because in several places I had to check the code around whether using mark->ignore_mask directly is actually fine. In particular:
quoted hunk ↗ jump to hunk
@@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, return 0; } else if (!(fid_mode & FAN_REPORT_FID)) { /* Do we have a directory inode to report? */ - if (!dir && !(event_mask & FS_ISDIR)) + if (!dir && !ondir) return 0; } fsnotify_foreach_iter_mark_type(iter_info, mark, type) { - /* Apply ignore mask regardless of mark's ISDIR flag */ - marks_ignored_mask |= mark->ignored_mask; + /* + * Apply ignore mask depending on whether FAN_ONDIR flag in + * ignore mask should be checked to ignore events on dirs. + */ + if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR) + marks_ignore_mask |= mark->ignore_mask; /* * If the event is on dir and this mark doesn't care about * events on dir, don't send it! */ - if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR)) + if (ondir && !(mark->mask & FAN_ONDIR)) continue; marks_mask |= mark->mask;
So for example here I'm wondering whether a helper should not be used...
quoted hunk ↗ jump to hunk
@@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, *match_mask |= 1U << type; } - test_mask = event_mask & marks_mask & ~marks_ignored_mask; + test_mask = event_mask & marks_mask & ~marks_ignore_mask;
Especially because here if say FAN_EVENT_ON_CHILD becomes a part of marks_ignore_mask it can result in clearing this flag in the returned 'mask' which is likely not what we want if there are some events left unignored in the 'mask'?
quoted hunk ↗ jump to hunk
@@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type, fsnotify_foreach_iter_mark_type(iter_info, mark, type) { group = mark->group; marks_mask |= mark->mask; - marks_ignored_mask |= mark->ignored_mask; + if (!(mask & FS_ISDIR) || + (fsnotify_ignore_mask(mark) & FS_ISDIR)) + marks_ignore_mask |= mark->ignore_mask; } - pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n", - __func__, group, mask, marks_mask, marks_ignored_mask, + pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n", + __func__, group, mask, marks_mask, marks_ignore_mask, data, data_type, dir, cookie); - if (!(test_mask & marks_mask & ~marks_ignored_mask)) + if (!(test_mask & marks_mask & ~marks_ignore_mask)) return 0;
And I'm wondering about similar things here... Honza -- Jan Kara [off-list ref] SUSE Labs, CR