| Message ID | 20220511092914.731897-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for fanotify parent dir ignore mask logic | expand |
On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > of iteratating all marks of a specific group interested in an event > by consulting the iterator report_mask. > > Use an open coded version of that iterator in fsnotify_iter_next() > that collects all marks of the current iteration group without > consulting the iterator report_mask. > > At the moment, the two iterator variants are the same, but this > decoupling will allow us to exclude some of the group's marks from > reporting the event, for example for event on child and inode marks > on parent did not request to watch events on children. > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > Reported-by: Jan Kara <jack@suse.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Mostly looks good. Two nits below. > /* > - * Pop from iter_info multi head queue, the marks that were iterated in the > + * Pop from iter_info multi head queue, the marks that belong to the group of > * current iteration step. > */ > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > { > + struct fsnotify_mark *mark; > int type; > > fsnotify_foreach_iter_type(type) { > - if (fsnotify_iter_should_report_type(iter_info, type)) > + mark = iter_info->marks[type]; > + if (mark && mark->group == iter_info->current_group) > iter_info->marks[type] = > fsnotify_next_mark(iter_info->marks[type]); Wouldn't it be more natural here to use the new helper fsnotify_foreach_iter_mark_type()? In principle we want to advance mark types which were already reported... > @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB) > > #define fsnotify_foreach_iter_type(type) \ > for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) > +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \ > + for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \ > + if (!(mark = fsnotify_iter_mark(iter, type))) {} else Hum, you're really inventive here ;) I'd rather go for something a bit more conservative and readable like: static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type, struct fsnotify_mark **markp) { while (type < FSNOTIFY_ITER_TYPE_COUNT) { *markp = fsnotify_iter_mark(iter, type); if (*markp) break; type++; } return type; } #define fsnotify_foreach_iter_mark_type(iter, mark, type) \ for (type = 0; \ (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \ type++) Honza
On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > > of iteratating all marks of a specific group interested in an event > > by consulting the iterator report_mask. > > > > Use an open coded version of that iterator in fsnotify_iter_next() > > that collects all marks of the current iteration group without > > consulting the iterator report_mask. > > > > At the moment, the two iterator variants are the same, but this > > decoupling will allow us to exclude some of the group's marks from > > reporting the event, for example for event on child and inode marks > > on parent did not request to watch events on children. > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > > Reported-by: Jan Kara <jack@suse.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Mostly looks good. Two nits below. > > > /* > > - * Pop from iter_info multi head queue, the marks that were iterated in the > > + * Pop from iter_info multi head queue, the marks that belong to the group of > > * current iteration step. > > */ > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > > { > > + struct fsnotify_mark *mark; > > int type; > > > > fsnotify_foreach_iter_type(type) { > > - if (fsnotify_iter_should_report_type(iter_info, type)) > > + mark = iter_info->marks[type]; > > + if (mark && mark->group == iter_info->current_group) > > iter_info->marks[type] = > > fsnotify_next_mark(iter_info->marks[type]); > > Wouldn't it be more natural here to use the new helper > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark > types which were already reported... Took me an embarrassing amount of time to figure out why this would be wrong and I must have known this a few weeks ago when I wrote the patch, so a comment is in order: /* * We cannot use fsnotify_foreach_iter_mark_type() here because we * may need to check if next group has a mark of type X even if current * group did not have a mark of type X. */ > > > @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB) > > > > #define fsnotify_foreach_iter_type(type) \ > > for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) > > +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \ > > + for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \ > > + if (!(mark = fsnotify_iter_mark(iter, type))) {} else > > Hum, you're really inventive here ;) I'd rather go for something a bit more > conservative and readable like: It's good that you are here to restrain me ;-) > > static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type, > struct fsnotify_mark **markp) > { > while (type < FSNOTIFY_ITER_TYPE_COUNT) { > *markp = fsnotify_iter_mark(iter, type); > if (*markp) > break; > type++; > } > return type; > } > > #define fsnotify_foreach_iter_mark_type(iter, mark, type) \ > for (type = 0; \ > (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \ > type++) > > That looks nicer. Thanks, Amir.
On Wed 11-05-22 21:26:16, Amir Goldstein wrote: > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > > > of iteratating all marks of a specific group interested in an event > > > by consulting the iterator report_mask. > > > > > > Use an open coded version of that iterator in fsnotify_iter_next() > > > that collects all marks of the current iteration group without > > > consulting the iterator report_mask. > > > > > > At the moment, the two iterator variants are the same, but this > > > decoupling will allow us to exclude some of the group's marks from > > > reporting the event, for example for event on child and inode marks > > > on parent did not request to watch events on children. > > > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > > > Reported-by: Jan Kara <jack@suse.com> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > Mostly looks good. Two nits below. > > > > > /* > > > - * Pop from iter_info multi head queue, the marks that were iterated in the > > > + * Pop from iter_info multi head queue, the marks that belong to the group of > > > * current iteration step. > > > */ > > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > > > { > > > + struct fsnotify_mark *mark; > > > int type; > > > > > > fsnotify_foreach_iter_type(type) { > > > - if (fsnotify_iter_should_report_type(iter_info, type)) > > > + mark = iter_info->marks[type]; > > > + if (mark && mark->group == iter_info->current_group) > > > iter_info->marks[type] = > > > fsnotify_next_mark(iter_info->marks[type]); > > > > Wouldn't it be more natural here to use the new helper > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark > > types which were already reported... > > Took me an embarrassing amount of time to figure out why this would be wrong > and I must have known this a few weeks ago when I wrote the patch, so > a comment is in order: > > /* > * We cannot use fsnotify_foreach_iter_mark_type() here because we > * may need to check if next group has a mark of type X even if current > * group did not have a mark of type X. > */ Well, but this function is just advancing the lists for marks we have already processed. And processed marks are exactly those set in report_mask. So your code should be equivalent to the old one but using fsnotify_foreach_iter_mark_type() should work as well AFAICT. Honza
On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 11-05-22 21:26:16, Amir Goldstein wrote: > > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > > > > of iteratating all marks of a specific group interested in an event > > > > by consulting the iterator report_mask. > > > > > > > > Use an open coded version of that iterator in fsnotify_iter_next() > > > > that collects all marks of the current iteration group without > > > > consulting the iterator report_mask. > > > > > > > > At the moment, the two iterator variants are the same, but this > > > > decoupling will allow us to exclude some of the group's marks from > > > > reporting the event, for example for event on child and inode marks > > > > on parent did not request to watch events on children. > > > > > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > > > > Reported-by: Jan Kara <jack@suse.com> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > Mostly looks good. Two nits below. > > > > > > > /* > > > > - * Pop from iter_info multi head queue, the marks that were iterated in the > > > > + * Pop from iter_info multi head queue, the marks that belong to the group of > > > > * current iteration step. > > > > */ > > > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > > > > { > > > > + struct fsnotify_mark *mark; > > > > int type; > > > > > > > > fsnotify_foreach_iter_type(type) { > > > > - if (fsnotify_iter_should_report_type(iter_info, type)) > > > > + mark = iter_info->marks[type]; > > > > + if (mark && mark->group == iter_info->current_group) > > > > iter_info->marks[type] = > > > > fsnotify_next_mark(iter_info->marks[type]); > > > > > > Wouldn't it be more natural here to use the new helper > > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark > > > types which were already reported... > > > > Took me an embarrassing amount of time to figure out why this would be wrong > > and I must have known this a few weeks ago when I wrote the patch, so > > a comment is in order: > > > > /* > > * We cannot use fsnotify_foreach_iter_mark_type() here because we > > * may need to check if next group has a mark of type X even if current > > * group did not have a mark of type X. > > */ > > Well, but this function is just advancing the lists for marks we have > already processed. And processed marks are exactly those set in report_mask. > So your code should be equivalent to the old one but using > fsnotify_foreach_iter_mark_type() should work as well AFAICT. Yes, you are right about that. But, With the 2 patches applied, fanotify09 test gets into livelock here. Fixing the livelock requires that fsnotify_iter_should_report_type() condition is removed. I am assuming that it is the next patch that introduces the livelock, but I cannot figure out why the comment I have written above is not true in the code in master. Could it be that the livelock already exists but we do not have a test case to trigger it and the test case in fanotify09 which checks the next patch is just a private case? In any case, even if the logic change here is not needed before the next patch, it would make no sense to convert this function to fsnotify_foreach_iter_mark_type() and change it back. The most that could make sense is to add this comment and remove fsnotify_iter_should_report_type() together with the next patch. Can you wrap your head around this? Because I can't. Thanks, Amir.
On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 11-05-22 21:26:16, Amir Goldstein wrote: > > > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > > > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > > > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > > > > > of iteratating all marks of a specific group interested in an event > > > > > by consulting the iterator report_mask. > > > > > > > > > > Use an open coded version of that iterator in fsnotify_iter_next() > > > > > that collects all marks of the current iteration group without > > > > > consulting the iterator report_mask. > > > > > > > > > > At the moment, the two iterator variants are the same, but this > > > > > decoupling will allow us to exclude some of the group's marks from > > > > > reporting the event, for example for event on child and inode marks > > > > > on parent did not request to watch events on children. > > > > > > > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > > > > > Reported-by: Jan Kara <jack@suse.com> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > > Mostly looks good. Two nits below. > > > > > > > > > /* > > > > > - * Pop from iter_info multi head queue, the marks that were iterated in the > > > > > + * Pop from iter_info multi head queue, the marks that belong to the group of > > > > > * current iteration step. > > > > > */ > > > > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > > > > > { > > > > > + struct fsnotify_mark *mark; > > > > > int type; > > > > > > > > > > fsnotify_foreach_iter_type(type) { > > > > > - if (fsnotify_iter_should_report_type(iter_info, type)) > > > > > + mark = iter_info->marks[type]; > > > > > + if (mark && mark->group == iter_info->current_group) > > > > > iter_info->marks[type] = > > > > > fsnotify_next_mark(iter_info->marks[type]); > > > > > > > > Wouldn't it be more natural here to use the new helper > > > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark > > > > types which were already reported... > > > > > > Took me an embarrassing amount of time to figure out why this would be wrong > > > and I must have known this a few weeks ago when I wrote the patch, so > > > a comment is in order: > > > > > > /* > > > * We cannot use fsnotify_foreach_iter_mark_type() here because we > > > * may need to check if next group has a mark of type X even if current > > > * group did not have a mark of type X. > > > */ > > > > Well, but this function is just advancing the lists for marks we have > > already processed. And processed marks are exactly those set in report_mask. > > So your code should be equivalent to the old one but using > > fsnotify_foreach_iter_mark_type() should work as well AFAICT. > > Yes, you are right about that. But, > With the 2 patches applied, fanotify09 test gets into livelock here. > Fixing the livelock requires that fsnotify_iter_should_report_type() > condition is removed. > > I am assuming that it is the next patch that introduces the livelock, > but I cannot figure out why the comment I have written above is not true > in the code in master. Nevermind. I got it. > > Could it be that the livelock already exists but we do not have a test case > to trigger it and the test case in fanotify09 which checks the next patch > is just a private case? > No. This is because of the change of behavior in the next patch. > In any case, even if the logic change here is not needed before the next > patch, it would make no sense to convert this function to > fsnotify_foreach_iter_mark_type() and change it back. > The most that could make sense is to add this comment and remove > fsnotify_iter_should_report_type() together with the next patch. Yes. I will do that and better explain in the comment. Thanks, Amir.
On Fri, May 13, 2022 at 6:07 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 11-05-22 21:26:16, Amir Goldstein wrote: > > > > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote: > > > > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code > > > > > > of iteratating all marks of a specific group interested in an event > > > > > > by consulting the iterator report_mask. > > > > > > > > > > > > Use an open coded version of that iterator in fsnotify_iter_next() > > > > > > that collects all marks of the current iteration group without > > > > > > consulting the iterator report_mask. > > > > > > > > > > > > At the moment, the two iterator variants are the same, but this > > > > > > decoupling will allow us to exclude some of the group's marks from > > > > > > reporting the event, for example for event on child and inode marks > > > > > > on parent did not request to watch events on children. > > > > > > > > > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") > > > > > > Reported-by: Jan Kara <jack@suse.com> > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > > > > Mostly looks good. Two nits below. > > > > > > > > > > > /* > > > > > > - * Pop from iter_info multi head queue, the marks that were iterated in the > > > > > > + * Pop from iter_info multi head queue, the marks that belong to the group of > > > > > > * current iteration step. > > > > > > */ > > > > > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > > > > > > { > > > > > > + struct fsnotify_mark *mark; > > > > > > int type; > > > > > > > > > > > > fsnotify_foreach_iter_type(type) { > > > > > > - if (fsnotify_iter_should_report_type(iter_info, type)) > > > > > > + mark = iter_info->marks[type]; > > > > > > + if (mark && mark->group == iter_info->current_group) > > > > > > iter_info->marks[type] = > > > > > > fsnotify_next_mark(iter_info->marks[type]); > > > > > > > > > > Wouldn't it be more natural here to use the new helper > > > > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark > > > > > types which were already reported... > > > > > > > > Took me an embarrassing amount of time to figure out why this would be wrong > > > > and I must have known this a few weeks ago when I wrote the patch, so > > > > a comment is in order: > > > > > > > > /* > > > > * We cannot use fsnotify_foreach_iter_mark_type() here because we > > > > * may need to check if next group has a mark of type X even if current > > > > * group did not have a mark of type X. > > > > */ > > > > > > Well, but this function is just advancing the lists for marks we have > > > already processed. And processed marks are exactly those set in report_mask. > > > So your code should be equivalent to the old one but using > > > fsnotify_foreach_iter_mark_type() should work as well AFAICT. > > > > Yes, you are right about that. But, > > With the 2 patches applied, fanotify09 test gets into livelock here. > > Fixing the livelock requires that fsnotify_iter_should_report_type() > > condition is removed. > > > > I am assuming that it is the next patch that introduces the livelock, > > but I cannot figure out why the comment I have written above is not true > > in the code in master. > > Nevermind. I got it. > > > > > Could it be that the livelock already exists but we do not have a test case > > to trigger it and the test case in fanotify09 which checks the next patch > > is just a private case? > > > > No. This is because of the change of behavior in the next patch. Bah, no it is not. Emailing before coffee is not a good idea... It is *this* patch that changes the behavior: fsnotify_iter_select_report_types() can return true with 0 report_mask and it is valid. This is documented in the updated comments above fsnotify_iter_next() and fsnotify_iter_select_report_types() and also in commit message: Use an open coded version of that iterator in fsnotify_iter_next() that collects all marks of the current iteration group without consulting the iterator report_mask. The comment I added to v2 could be better phrased as: /* * We cannot use fsnotify_foreach_iter_mark_type() here because we * may need to advance a mark of type X that belongs to current_group * but was not selected for reporting. */ Is that better? Do you want me to re-post or can you fix that on commit? Maybe even phrase it better than I did. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 985e995d2a39..263d303d8f8f 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -319,11 +319,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, return 0; } - fsnotify_foreach_iter_type(type) { - if (!fsnotify_iter_should_report_type(iter_info, type)) - continue; - mark = iter_info->marks[type]; - + fsnotify_foreach_iter_mark_type(iter_info, mark, type) { /* Apply ignore mask regardless of ISDIR and ON_CHILD flags */ marks_ignored_mask |= mark->ignored_mask; @@ -849,16 +845,14 @@ static struct fanotify_event *fanotify_alloc_event( */ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info) { + struct fsnotify_mark *mark; int type; __kernel_fsid_t fsid = {}; - fsnotify_foreach_iter_type(type) { + fsnotify_foreach_iter_mark_type(iter_info, mark, type) { struct fsnotify_mark_connector *conn; - if (!fsnotify_iter_should_report_type(iter_info, type)) - continue; - - conn = READ_ONCE(iter_info->marks[type]->connector); + conn = READ_ONCE(mark->connector); /* Mark is just getting destroyed or created? */ if (!conn) continue; diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 6eee19d15e8c..00f11ef4c0ed 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -335,31 +335,23 @@ static int send_to_group(__u32 mask, const void *data, int data_type, struct fsnotify_mark *mark; int type; - if (WARN_ON(!iter_info->report_mask)) + if (!iter_info->report_mask) return 0; /* clear ignored on inode modification */ if (mask & FS_MODIFY) { - fsnotify_foreach_iter_type(type) { - if (!fsnotify_iter_should_report_type(iter_info, type)) - continue; - mark = iter_info->marks[type]; - if (mark && - !(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) + fsnotify_foreach_iter_mark_type(iter_info, mark, type) { + if (!(mark->flags & + FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) mark->ignored_mask = 0; } } - fsnotify_foreach_iter_type(type) { - if (!fsnotify_iter_should_report_type(iter_info, type)) - continue; - mark = iter_info->marks[type]; - /* does the object mark tell us to do something? */ - if (mark) { - group = mark->group; - marks_mask |= mark->mask; - marks_ignored_mask |= mark->ignored_mask; - } + /* Are any of the group marks interested in this event? */ + fsnotify_foreach_iter_mark_type(iter_info, mark, type) { + group = mark->group; + marks_mask |= mark->mask; + marks_ignored_mask |= mark->ignored_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", @@ -403,11 +395,11 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark) /* * iter_info is a multi head priority queue of marks. - * Pick a subset of marks from queue heads, all with the - * same group and set the report_mask for selected subset. - * Returns the report_mask of the selected subset. + * Pick a subset of marks from queue heads, all with the same group + * and set the report_mask to a subset of the selected marks. + * Returns false if there are no more groups to iterate. */ -static unsigned int fsnotify_iter_select_report_types( +static bool fsnotify_iter_select_report_types( struct fsnotify_iter_info *iter_info) { struct fsnotify_group *max_prio_group = NULL; @@ -423,30 +415,32 @@ static unsigned int fsnotify_iter_select_report_types( } if (!max_prio_group) - return 0; + return false; /* Set the report mask for marks from same group as max prio group */ + iter_info->current_group = max_prio_group; iter_info->report_mask = 0; fsnotify_foreach_iter_type(type) { mark = iter_info->marks[type]; - if (mark && - fsnotify_compare_groups(max_prio_group, mark->group) == 0) + if (mark && mark->group == iter_info->current_group) fsnotify_iter_set_report_type(iter_info, type); } - return iter_info->report_mask; + return true; } /* - * Pop from iter_info multi head queue, the marks that were iterated in the + * Pop from iter_info multi head queue, the marks that belong to the group of * current iteration step. */ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) { + struct fsnotify_mark *mark; int type; fsnotify_foreach_iter_type(type) { - if (fsnotify_iter_should_report_type(iter_info, type)) + mark = iter_info->marks[type]; + if (mark && mark->group == iter_info->current_group) iter_info->marks[type] = fsnotify_next_mark(iter_info->marks[type]); } diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 9a1a9e78f69f..a21dccc255ee 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -399,6 +399,7 @@ static inline bool fsnotify_valid_obj_type(unsigned int obj_type) struct fsnotify_iter_info { struct fsnotify_mark *marks[FSNOTIFY_ITER_TYPE_COUNT]; + struct fsnotify_group *current_group; unsigned int report_mask; int srcu_idx; }; @@ -415,20 +416,19 @@ static inline void fsnotify_iter_set_report_type( iter_info->report_mask |= (1U << iter_type); } -static inline void fsnotify_iter_set_report_type_mark( - struct fsnotify_iter_info *iter_info, int iter_type, - struct fsnotify_mark *mark) +static inline struct fsnotify_mark *fsnotify_iter_mark( + struct fsnotify_iter_info *iter_info, int iter_type) { - iter_info->marks[iter_type] = mark; - iter_info->report_mask |= (1U << iter_type); + if (fsnotify_iter_should_report_type(iter_info, iter_type)) + return iter_info->marks[iter_type]; + return NULL; } #define FSNOTIFY_ITER_FUNCS(name, NAME) \ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \ struct fsnotify_iter_info *iter_info) \ { \ - return (iter_info->report_mask & (1U << FSNOTIFY_ITER_TYPE_##NAME)) ? \ - iter_info->marks[FSNOTIFY_ITER_TYPE_##NAME] : NULL; \ + return fsnotify_iter_mark(iter_info, FSNOTIFY_ITER_TYPE_##NAME); \ } FSNOTIFY_ITER_FUNCS(inode, INODE) @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB) #define fsnotify_foreach_iter_type(type) \ for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \ + for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \ + if (!(mark = fsnotify_iter_mark(iter, type))) {} else /* * fsnotify_connp_t is what we embed in objects which connector can be attached
fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code of iteratating all marks of a specific group interested in an event by consulting the iterator report_mask. Use an open coded version of that iterator in fsnotify_iter_next() that collects all marks of the current iteration group without consulting the iterator report_mask. At the moment, the two iterator variants are the same, but this decoupling will allow us to exclude some of the group's marks from reporting the event, for example for event on child and inode marks on parent did not request to watch events on children. Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir") Reported-by: Jan Kara <jack@suse.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 14 +++------- fs/notify/fsnotify.c | 48 ++++++++++++++------------------ include/linux/fsnotify_backend.h | 17 ++++++----- 3 files changed, 35 insertions(+), 44 deletions(-)