Re: [PATCH v3 2/2] fanotify: introduce restartable permission events

[Gao Xiang <xiang@kernel.org>]

Hi Ibrahim, all,

On Wed, Apr 22, 2026 at 09:54:37AM -0700, Ibrahim Jirdeh wrote:
> On Wed, Apr 22, 2026 at 4:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Apr 22, 2026 at 12:36 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 22-04-26 03:22:32, Ibrahim Jirdeh wrote:
> > > > On Mon, Apr 20, 2026 at 7:37 AM Jan Kara <jack@suse.cz> wrote:
> > > > > > +const struct file_operations fanotify_control_fops = {
> > > > > > +     .show_fdinfo    = fanotify_show_fdinfo,
> > > > > > +     .release        = fanotify_release,
> > > > > > +     .unlocked_ioctl = fanotify_control_ioctl,
> > > > > > +     .llseek         = noop_llseek,
> > > > > > +};
> > > > > > +
> > > > > > +const struct file_operations fanotify_queue_fops = {
> > > > > > +     .poll           = fanotify_poll,
> > > > > > +     .read           = fanotify_read,
> > > > > > +     .write          = fanotify_write,
> > > > > > +     .release        = fanotify_queue_release,
> > > > > > +     .unlocked_ioctl = fanotify_ioctl,
> > > > > > +     .compat_ioctl   = compat_ptr_ioctl,
> > > > > > +     .llseek         = noop_llseek,
> > > > > > +};
> > > > >
> > > > > The new scheme with queue fd & control fd opens up an interesting new set
> > > > > of races which needs a deeper thought I think. As it is currently
> > > > > implemented for example if there's a task waiting for event on queue fd, it
> > > > > will never wake up if control fd is closed. Similar problem is there for
> > > > > fanotify_poll(). So we need to carefully define how operations on queue fd
> > > > > synchronize with operations on control fd and closing of it in particular.
> > > > > Besides dealing with the hung waits (which can be dealt with by just waking
> > > > > the notification_waitq from fanotify_release()) we also need to deal with
> > > > > fanotify_read() moving events from notification list to access list. I'm a
> > > > > bit unhappy with how ad hoc and fragile this looks but so far don't have an
> > > > > idea for a cleaner solution.
> > > > >
> > > >
> > > > I wanted to ask a few more questions here about the race conditions you are
> > > > outlining. Is it accurate that the main risk of races is primarily
> > > > related to early control fd shutdown with this new two fd case, or are
> > > > there other races to worry about?
> > >
> > > No, this is my main concern.
> > >
> > > > I added a few protections in this patch so far and wanted to check if
> > > > there are outsanding risks:
> > > > 1) Have queue fd also maintain a group reference as well
> > >
> > > Yes, this is good and takes care that the group is not freed. But it still
> > > can be fully cleaned up even if you hold a reference.
> > >
> > > > 2) Check for control fd close (group->shutdown) in fanotify_read. I will also
> > > > incorporate the wake up in fanotify_release you have suggested here.
> > >
> > > Yes, I've noticed that but that check is racy...
> > >
> > > > Regarding fanotify_read moving events from notification list to access list, do
> > > > we need to allow pending events in fanotify_restart_pending_events
> > > > after all to prevent potential hung waits stemming from race with shutdown?
> > >
> > > Thinking about this some more... Won't it be easier if the queue fd held a
> > > struct file reference of the control fd? That way the notification group
> > > couldn't be shut down until queue fd is also closed. That will deal with
> > > all the possible races, we don't even have to hold a notification group
> > > reference. I don't think this behavior would be surprising to userspace and
> > > I don't see any other possible downsides either. Amir, any opinion?
> > >
> >
> > hmm, that sounds pretty neat.
> >
> > queue_file->private_data = control_file;
> > control_file->private_data = group;
> >
> > Makes sense to me.
> >
> > Please remove the group->shutdown check in read and any other
> > safe race guards that were added.
> 
> Thanks, this should be much more straightforward. I will send over an updated
> version containing this change to hold file reference and other fixes soon.

Just a friendly ping: I wonder if this series is still working in progress?
Currently fanotify lacks of failover implementation, and our EROFS fanotify
lazy pulling needs this too.

Thanks,
Gao Xiang