On Fri, Jul 11, 2025 at 4:36 AM Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> wrote: > > This adds support for responding to events via response identifier. This > prevents races if there are multiple processes backing the same fanotify > group (eg. handover of fanotify group to new instance of a backing daemon). > It is also useful for reporting pre-dir-content events without an > event->fd: > https://lore.kernel.org/linux-fsdevel/2dx3pbcnv5w75fxb2ghqtsk6gzl6cuxmd2rinzwbq7xxfjf5z7@3nqidi3mno46/. > > Rather than introducing a new event identifier field and extending > fanotify_event_metadata, we have opted to overload event->fd and restrict > this functionality to use-cases which are using file handle apis > (FAN_REPORT_FID). > > In terms of how response ids are allocated, we use an idr for allocation > and restrict the id range to below -255 to ensure there is no overlap with > existing fd-as-identifier usage. We can also leverage the added idr for > more efficient lookup when handling response although that is not done > in this patch. > > Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx> > Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxheeLXdTLLWrixnTJcxVP+BV4ViXijbvERHPenzgDMUTA@xxxxxxxxxxxxxx/ > Signed-off-by: Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> > --- > fs/notify/fanotify/fanotify.c | 3 ++ > fs/notify/fanotify/fanotify.h | 11 ++++- > fs/notify/fanotify/fanotify_user.c | 63 ++++++++++++++++++++--------- > include/linux/fanotify.h | 1 + > include/linux/fsnotify_backend.h | 1 + > include/uapi/linux/fanotify.h | 11 ++++- > tools/include/uapi/linux/fanotify.h | 11 ++++- > 7 files changed, 77 insertions(+), 24 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 34acb7c16e8b..d9aebd359199 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -1106,6 +1106,9 @@ static void fanotify_free_event(struct fsnotify_group *group, > > event = FANOTIFY_E(fsn_event); > put_pid(event->pid); > + if (fanotify_is_perm_event(event->mask) && > + FAN_GROUP_FLAG(group, FAN_REPORT_RESPONSE_ID)) > + idr_remove(&group->response_idr, -FANOTIFY_PERM(event)->id); idr_*() are not thread safe and there is no group lock held here. But I misled you. You should use ida_alloc_min()/ida_free() which are simpler and thread safe. > switch (event->type) { > case FANOTIFY_EVENT_TYPE_PATH: > fanotify_free_path_event(event); > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index f6d25fcf8692..b6a414f44acc 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -444,7 +444,7 @@ struct fanotify_perm_event { > size_t count; > u32 response; /* userspace answer to the event */ > unsigned short state; /* state of the event */ > - int fd; /* fd we passed to userspace for this event */ > + int id; /* id we passed to userspace for this event */ > union { > struct fanotify_response_info_header hdr; > struct fanotify_response_info_audit_rule audit_rule; > @@ -559,3 +559,12 @@ static inline u32 fanotify_get_response_errno(int res) > { > return (res >> FAN_ERRNO_SHIFT) & FAN_ERRNO_MASK; > } > + > +static inline bool fanotify_is_valid_response_id(struct fsnotify_group *group, > + int id) > +{ > + if (FAN_GROUP_FLAG(group, FAN_REPORT_RESPONSE_ID)) > + return id < -255; > + > + return id >= 0; > +} > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 19d3f2d914fe..2e14db38d298 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -330,14 +330,19 @@ static int process_access_response(struct fsnotify_group *group, > size_t info_len) > { > struct fanotify_perm_event *event; > - int fd = response_struct->fd; > + int id = response_struct->id; > u32 response = response_struct->response; > int errno = fanotify_get_response_errno(response); > int ret = info_len; > struct fanotify_response_info_audit_rule friar; > > - pr_debug("%s: group=%p fd=%d response=%x errno=%d buf=%p size=%zu\n", > - __func__, group, fd, response, errno, info, info_len); > + BUILD_BUG_ON(sizeof(response_struct->id) != > + sizeof(response_struct->fd)); > + BUILD_BUG_ON(offsetof(struct fanotify_response, id) != > + offsetof(struct fanotify_response, fd)); > + > + pr_debug("%s: group=%p id=%d response=%x errno=%d buf=%p size=%zu\n", > + __func__, group, id, response, errno, info, info_len); > /* > * make sure the response is valid, if invalid we do nothing and either > * userspace can send a valid response or we will clean it up after the > @@ -385,19 +390,18 @@ static int process_access_response(struct fsnotify_group *group, > ret = process_access_response_info(info, info_len, &friar); > if (ret < 0) > return ret; > - if (fd == FAN_NOFD) > + if (id == FAN_NOFD) > return ret; > } else { > ret = 0; > } > - > - if (fd < 0) > + if (!fanotify_is_valid_response_id(group, id)) > return -EINVAL; > > spin_lock(&group->notification_lock); > list_for_each_entry(event, &group->fanotify_data.access_list, > fae.fse.list) { > - if (event->fd != fd) > + if (event->id != id) > continue; > > list_del_init(&event->fae.fse.list); > @@ -765,14 +769,20 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > task_tgid(current) != event->pid) > metadata.pid = 0; > > - /* > - * For now, fid mode is required for an unprivileged listener and > - * fid mode does not report fd in events. Keep this check anyway > - * for safety in case fid mode requirement is relaxed in the future > - * to allow unprivileged listener to get events with no fd and no fid. > - */ > - if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) && > - path && path->mnt && path->dentry) { > + if (FAN_GROUP_FLAG(group, FAN_REPORT_RESPONSE_ID)) { > + ret = idr_alloc_cyclic(&group->response_idr, event, 256, > + INT_MAX, GFP_NOWAIT); So please use ida_alloc_min() with GFP_KERNEL there is no reason for special memory allocation flags in this context. > + if (ret < 0) > + return ret; > + fd = -ret; > + } else if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) && path && > + path->mnt && path->dentry) { > + /* > + * For now, fid mode is required for an unprivileged listener and > + * fid mode does not report fd in events. Actually, my FAN_CLASS_PRE_CONTENT_FID patch makes this comment inaccurate. The code is still correct, but not the reasoning to explain it. I could change it myself in my patch, but as you move the comment might as well fix it. /* * For now, fid mode and no-permission-events class are required * for FANOTIFY_UNPRIV listener and fid mode does not report fd in * non-permission notification events. Keep this check anyway... > Keep this check anyway > + * for safety in case fid mode requirement is relaxed in the future > + * to allow unprivileged listener to get events with no fd and no fid. > + */ > fd = create_fd(group, path, &f); > /* > * Opening an fd from dentry can fail for several reasons. > @@ -803,7 +813,11 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > } > } > } > - if (FAN_GROUP_FLAG(group, FAN_REPORT_FD_ERROR)) > + > + BUILD_BUG_ON(sizeof(metadata.id) != sizeof(metadata.fd)); > + BUILD_BUG_ON(offsetof(struct fanotify_event_metadata, id) != > + offsetof(struct fanotify_event_metadata, fd)); > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FD_ERROR | FAN_REPORT_RESPONSE_ID)) > metadata.fd = fd; > else > metadata.fd = fd >= 0 ? fd : FAN_NOFD; > @@ -859,7 +873,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > fd_install(pidfd, pidfd_file); > > if (fanotify_is_perm_event(event->mask)) > - FANOTIFY_PERM(event)->fd = fd; > + FANOTIFY_PERM(event)->id = fd; > > return metadata.event_len; > > @@ -944,7 +958,9 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > if (!fanotify_is_perm_event(event->mask)) { > fsnotify_destroy_event(group, &event->fse); > } else { > - if (ret <= 0 || FANOTIFY_PERM(event)->fd < 0) { > + if (ret <= 0 || > + !fanotify_is_valid_response_id( > + group, FANOTIFY_PERM(event)->id)) { > spin_lock(&group->notification_lock); > finish_permission_event(group, > FANOTIFY_PERM(event), FAN_DENY, NULL); > @@ -1584,6 +1600,14 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) > return -EINVAL; > > /* > + * With group that reports fid info and allows pre-content events, > + * user may request to get a response id instead of event->fd. > + */ Seems like the indentation here is off. > + if ((flags & FAN_REPORT_RESPONSE_ID) && > + (!fid_mode || class == FAN_CLASS_NOTIF)) > + return -EINVAL; > + > + /* > * Child name is reported with parent fid so requires dir fid. > * We can report both child fid and dir fid with or without name. > */ > @@ -1660,6 +1684,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) > fd = -EINVAL; > goto out_destroy_group; > } > + idr_init(&group->response_idr); So ida_init() and you are also missing ida_destroy() in fanotify_free_group_priv() Thanks, Amir.
