| Message ID | 20200524072441.18258-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: fix ignore mask logic for events on child and on dir | expand |
On Sun 24-05-20 10:24:41, Amir Goldstein wrote: > The comments in fanotify_group_event_mask() say: > > "If the event is on dir/child and this mark doesn't care about > events on dir/child, don't send it!" > > Specifically, mount and filesystem marks do not care about events > on child, but they can still specify an ignore mask for those events. > For example, a group that has: > - A mount mark with mask 0 and ignore_mask FAN_OPEN > - An inode mark on a directory with mask FAN_OPEN | FAN_OPEN_EXEC > with flag FAN_EVENT_ON_CHILD > > A child file open for exec would be reported to group with the FAN_OPEN > event despite the fact that FAN_OPEN is in ignore mask of mount mark, > because the mark iteration loop skips over non-inode marks for events > on child when calculating the ignore mask. > > Move ignore mask calculation to the top of the iteration loop block > before excluding marks for events on dir/child. > > Reported-by: Jan Kara <jack@suse.cz> > Link: https://lore.kernel.org/linux-fsdevel/20200521162443.GA26052@quack2.suse.cz/ > Fixes: 55bf882c7f13 "fanotify: fix merging marks masks with FAN_ONDIR" > Fixes: b469e7e47c8a "fanotify: fix handling of events on child..." > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Thanks! I've added the patch to my tree. I don't think this is really urgent fix so I plan to push it to Linus in the coming merge window. Honza > --- > > Jan, > > As you suspected we had a bug in ignore mask logic. > The actual test case is quite asoteric, but it's worth fixing the logic > anyway. > > Judging by LTP tests fanotify10 and fanotify12, we were pretty paranoid > about adding the compound event FAN_OPEN | FAN_OPEN_EXEC, so Matthew > wrote a lot of tests even for ignore mask, but we still missed this > corner case. > > It was, however, trivial to add a test case for this issue [1]. > I couldn't figure out if a similar bug exists with FAN_ONDIR, because > the FAN_OPEN_EXEC event is not applicable and it is quite hard to figure > out if fsnotify_change() is ever called with a combination of ia_valid > flags that ends up with a compound event on dir. > > Thanks, > Amir. > > [1] https://github.com/amir73il/ltp/commits/fsnotify-fixes > > fs/notify/fanotify/fanotify.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 95480d3dcff7..e22fd8f8c281 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -232,6 +232,10 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > if (!fsnotify_iter_should_report_type(iter_info, type)) > continue; > mark = iter_info->marks[type]; > + > + /* Apply ignore mask regardless of ISDIR and ON_CHILD flags */ > + marks_ignored_mask |= mark->ignored_mask; > + > /* > * If the event is on dir and this mark doesn't care about > * events on dir, don't send it! > @@ -249,7 +253,6 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > continue; > > marks_mask |= mark->mask; > - marks_ignored_mask |= mark->ignored_mask; > } > > test_mask = event_mask & marks_mask & ~marks_ignored_mask; > -- > 2.17.1 >
On Mon, May 25, 2020 at 11:44 AM Jan Kara <jack@suse.cz> wrote: > > On Sun 24-05-20 10:24:41, Amir Goldstein wrote: > > The comments in fanotify_group_event_mask() say: > > > > "If the event is on dir/child and this mark doesn't care about > > events on dir/child, don't send it!" > > > > Specifically, mount and filesystem marks do not care about events > > on child, but they can still specify an ignore mask for those events. > > For example, a group that has: > > - A mount mark with mask 0 and ignore_mask FAN_OPEN > > - An inode mark on a directory with mask FAN_OPEN | FAN_OPEN_EXEC > > with flag FAN_EVENT_ON_CHILD > > > > A child file open for exec would be reported to group with the FAN_OPEN > > event despite the fact that FAN_OPEN is in ignore mask of mount mark, > > because the mark iteration loop skips over non-inode marks for events > > on child when calculating the ignore mask. > > > > Move ignore mask calculation to the top of the iteration loop block > > before excluding marks for events on dir/child. > > > > Reported-by: Jan Kara <jack@suse.cz> > > Link: https://lore.kernel.org/linux-fsdevel/20200521162443.GA26052@quack2.suse.cz/ > > Fixes: 55bf882c7f13 "fanotify: fix merging marks masks with FAN_ONDIR" > > Fixes: b469e7e47c8a "fanotify: fix handling of events on child..." > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Thanks! I've added the patch to my tree. I don't think this is really > urgent fix so I plan to push it to Linus in the coming merge window. > Agreed. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 95480d3dcff7..e22fd8f8c281 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -232,6 +232,10 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, if (!fsnotify_iter_should_report_type(iter_info, type)) continue; mark = iter_info->marks[type]; + + /* Apply ignore mask regardless of ISDIR and ON_CHILD flags */ + marks_ignored_mask |= mark->ignored_mask; + /* * If the event is on dir and this mark doesn't care about * events on dir, don't send it! @@ -249,7 +253,6 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, continue; marks_mask |= mark->mask; - marks_ignored_mask |= mark->ignored_mask; } test_mask = event_mask & marks_mask & ~marks_ignored_mask;
The comments in fanotify_group_event_mask() say: "If the event is on dir/child and this mark doesn't care about events on dir/child, don't send it!" Specifically, mount and filesystem marks do not care about events on child, but they can still specify an ignore mask for those events. For example, a group that has: - A mount mark with mask 0 and ignore_mask FAN_OPEN - An inode mark on a directory with mask FAN_OPEN | FAN_OPEN_EXEC with flag FAN_EVENT_ON_CHILD A child file open for exec would be reported to group with the FAN_OPEN event despite the fact that FAN_OPEN is in ignore mask of mount mark, because the mark iteration loop skips over non-inode marks for events on child when calculating the ignore mask. Move ignore mask calculation to the top of the iteration loop block before excluding marks for events on dir/child. Reported-by: Jan Kara <jack@suse.cz> Link: https://lore.kernel.org/linux-fsdevel/20200521162443.GA26052@quack2.suse.cz/ Fixes: 55bf882c7f13 "fanotify: fix merging marks masks with FAN_ONDIR" Fixes: b469e7e47c8a "fanotify: fix handling of events on child..." Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, As you suspected we had a bug in ignore mask logic. The actual test case is quite asoteric, but it's worth fixing the logic anyway. Judging by LTP tests fanotify10 and fanotify12, we were pretty paranoid about adding the compound event FAN_OPEN | FAN_OPEN_EXEC, so Matthew wrote a lot of tests even for ignore mask, but we still missed this corner case. It was, however, trivial to add a test case for this issue [1]. I couldn't figure out if a similar bug exists with FAN_ONDIR, because the FAN_OPEN_EXEC event is not applicable and it is quite hard to figure out if fsnotify_change() is ever called with a combination of ia_valid flags that ends up with a compound event on dir. Thanks, Amir. [1] https://github.com/amir73il/ltp/commits/fsnotify-fixes fs/notify/fanotify/fanotify.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)