On Thu, Feb 14, 2019 at 8:01 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 13-02-19 23:02:17, Amir Goldstein wrote:
> > On Wed, Feb 13, 2019 at 4:54 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxx>
> > > Reported-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
> > > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
Thanks,
Amir.
