On Thu, Aug 7, 2025 at 12:06 AM Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> wrote: > > This adds logic in order to support restarting pending permission > events. In terms of implementation, we reinsert events into > notification queue so they can be reprocessed on subsequent read > ops (an alternative is restarting fsnotify call [1]). > Restart will be triggered upon queue fd release. > > [1] https://lore.kernel.org/linux-fsdevel/2ogjwnem7o3jwukzoq2ywnxha5ljiqnjnr4o4b5xvdvwpbyeac@v4i7jygvk7fj/2-0001-fanotify-Add-support-for-resending-unanswered-permis.patch > > Suggested-by: Jan Kara <jack@xxxxxxx> > Link: https://lore.kernel.org/linux-fsdevel/sx5g7pmkchjqucfbzi77xh7wx4wua5nteqi5bsa2hfqgxua2a2@v7x6ja3gsirn/ > Signed-off-by: Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> > --- > fs/notify/fanotify/fanotify.c | 4 +-- > fs/notify/fanotify/fanotify.h | 6 ++++ > fs/notify/fanotify/fanotify_user.c | 57 ++++++++++++++++++++++++------ > 3 files changed, 55 insertions(+), 12 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index bfe884d624e7..6f5f43a3e6bd 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -179,7 +179,7 @@ static bool fanotify_should_merge(struct fanotify_event *old, > #define FANOTIFY_MAX_MERGE_EVENTS 128 > > /* and the list better be locked by something too! */ > -static int fanotify_merge(struct fsnotify_group *group, > +int fanotify_merge(struct fsnotify_group *group, > struct fsnotify_event *event) fanotify_merge is not for permission events, its irrelevant > { > struct fanotify_event *old, *new = FANOTIFY_E(event); > @@ -904,7 +904,7 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info) > /* > * Add an event to hash table for faster merge. > */ > -static void fanotify_insert_event(struct fsnotify_group *group, > +void fanotify_insert_event(struct fsnotify_group *group, > struct fsnotify_event *fsn_event) You should not use fanotify_insert_event callback because it is irrelevant for permission events. > { > struct fanotify_event *event = FANOTIFY_E(fsn_event); > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index b78308975082..c0dffbc3370d 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -550,3 +550,9 @@ static inline u32 fanotify_get_response_errno(int res) > { > return (res >> FAN_ERRNO_SHIFT) & FAN_ERRNO_MASK; > } > + > +extern void fanotify_insert_event(struct fsnotify_group *group, > + struct fsnotify_event *fsn_event); > + > +extern int fanotify_merge(struct fsnotify_group *group, > + struct fsnotify_event *event); > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index b192ee068a7a..01d273d35936 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1000,6 +1000,51 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t > return count; > } > > +static void clear_queue(struct file *file, bool restart_events) > +{ > + struct fsnotify_group *group = file->private_data; > + struct fsnotify_event *fsn_event; > + int insert_ret; > + > + /* > + * Clear all pending permission events from the access_list. If > + * restart is requested, move them back into the notification queue > + * for reprocessing, otherwise simulate a reply from userspace. > + */ > + spin_lock(&group->notification_lock); > + while (!list_empty(&group->fanotify_data.access_list)) { > + struct fanotify_perm_event *event; > + > + event = list_first_entry(&group->fanotify_data.access_list, > + struct fanotify_perm_event, > + fae.fse.list); > + list_del_init(&event->fae.fse.list); > + > + if (restart_events) { > + // requeue the event > + spin_unlock(&group->notification_lock); > + fsn_event = &event->fae.fse; > + > + insert_ret = fsnotify_insert_event( > + group, fsn_event, fanotify_merge, > + fanotify_insert_event); 1. Restarted events should be inserted to the head of queue 2. merge and insert callbacks are irrelevant 3. I don't think we should deal with overflow for restarted events. (possibly, we can count pending events in group->q_len) IOW, a dedicated fsnotify_restart_event() is probably in order > + if (insert_ret) { > + /* > + * insertion for permission events can fail if group itself > + * is being shutdown. In this case, simply reply ALLOW for > + * the event. > + */ > + spin_lock(&group->notification_lock); > + finish_permission_event(group, event, FAN_ALLOW, NULL); > + } > + } else { > + finish_permission_event(group, event, FAN_ALLOW, NULL); > + } > + spin_lock(&group->notification_lock); > + } > + spin_unlock(&group->notification_lock); > +} > + I find very little value (negative value in fact) in this clear-or-restart helper. The two cases are almost completely different code paths and the helper looks complex and overly nested. Thanks, Amir.
