Re: [PATCH 6/6] fanotify: Use interruptible wait when waiting for permission events

[Jan Kara <jack@suse.cz>]

On Thu 14-02-19 20:53:38, Amir Goldstein wrote:
> On Thu, Feb 14, 2019 at 8:01 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 13-02-19 23:02:17, Amir Goldstein wrote:
> > > On Wed, Feb 13, 2019 at 4:54 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > When waiting for response to fanotify permission events, we currently
> > > > use uninterruptible waits. That makes code simple however it can cause
> > > > lots of processes to end up in uninterruptible sleep with hard reboot
> > > > being the only alternative in case fanotify listener process stops
> > > > responding (e.g. due to a bug in its implementation). Uninterruptible
> > > > sleep also makes system hibernation fail if the listener gets frozen
> > > > before the process generating fanotify permission event.
> > > >
> > > > Fix these problems by using interruptible sleep for waiting for response
> > > > to fanotify event. This is slightly tricky though - we have to
> > > > detect when the event got already reported to userspace as in that
> > > > case we must not free the event. Instead we push the responsibility for
> > > > freeing the event to the process that will write response to the
> > > > event.
> > > >
> > > > Reported-by: Orion Poplawski <orion@nwra.com>
> > > > Reported-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/notify/fanotify/fanotify.c | 35 ++++++++++++++++++++++++++++++++---
> > > >  fs/notify/fanotify/fanotify.h |  3 ++-
> > > >  2 files changed, 34 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > > index e725831be161..2e40d5d8660b 100644
> > > > --- a/fs/notify/fanotify/fanotify.c
> > > > +++ b/fs/notify/fanotify/fanotify.c
> > > > @@ -57,6 +57,13 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
> > > >         return 0;
> > > >  }
> > > >
> > > > +/*
> > > > + * Wait for response to permission event. The function also takes care of
> > > > + * freeing the permission event (or offloads that in case the wait is canceled
> > > > + * by a signal). The function returns 0 in case access got allowed by userspace,
> > > > + * -EPERM in case userspace disallowed the access, and -ERESTARTSYS in case
> > > > + * the wait got interrupted by a signal.
> > > > + */
> > > >  static int fanotify_get_response(struct fsnotify_group *group,
> > > >                                  struct fanotify_perm_event_info *event,
> > > >                                  struct fsnotify_iter_info *iter_info)
> > > > @@ -65,8 +72,29 @@ static int fanotify_get_response(struct fsnotify_group *group,
> > > >
> > > >         pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> > > >
> > > > -       wait_event(group->fanotify_data.access_waitq,
> > > > -                  event->state == FAN_EVENT_ANSWERED);
> > > > +       ret = wait_event_interruptible(group->fanotify_data.access_waitq,
> > > > +                                      event->state == FAN_EVENT_ANSWERED);
> > > > +       /* Signal pending? */
> > > > +       if (ret < 0) {
> > > > +               spin_lock(&group->notification_lock);
> > > > +               /* Event reported to userspace and no answer yet? */
> > > > +               if (event->state == FAN_EVENT_REPORTED) {
> > > > +                       /* Event will get freed once userspace answers to it */
> > >
> > > Did you forget to commit the
> > > if (event->state == FAN_EVENT_CANCELED)
> > > code?
> >
> > Yeah, somehow I forgot that bit. Thanks for noticing! Attached is a fixed
> > version of the patch.
> >
> 
> Looks ok.
> 
> Feel free to add:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks for review Amir! I'll queue patches to my tree then.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR