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

[Jan Kara <jack@suse.cz>]

On Wed 09-01-19 09:51:08, Amir Goldstein wrote:
> On Tue, Jan 8, 2019 at 6:46 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.
> 
> It a bit hard to follow this patch. Can we make the patch that
> shifts responsibility to free the event a separate patch?
> fsnotify_remove_queued_event() helper can tag along with it
> or separate patch as you wish.

I'll have a look how to best split this. It should be possible.

> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> I think it would be good to add reported-by tand the links you provided
> in cover letter in this patch as well.

Good point. Will do.

> >
> > @@ -87,8 +116,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
> >         if (response & FAN_AUDIT)
> >                 audit_fanotify(response & ~FAN_AUDIT);
> >
> > -       pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
> > -                group, event, ret);
> 
> Looks useful. Why remove?

OK, I'll leave it alone.

> > +static void set_event_response(struct fsnotify_group *group,
> > +                              struct fanotify_perm_event_info *event,
> > +                              unsigned int response)
> > +{
> > +       spin_lock(&group->notification_lock);
> > +       /* Waiter got aborted by a signal? Free the event. */
> > +       if (unlikely(event->response == FAN_EVENT_CANCELED)) {
> > +               spin_unlock(&group->notification_lock);
> > +               fsnotify_destroy_event(group, &event->fae.fse);
> > +               return;
> > +       }
> > +       event->response = response | FAN_EVENT_ANSWERED;
> > +       spin_unlock(&group->notification_lock);
> > +}
> > +
> 
> I don't understand. AFAICS, all callers of set_event_response()
> call immediately after spin_unlock(&group->notification_lock),
> without any user wait involved.
> I think it makes more sense for set_event_response() to assert the
> lock than it is to take it.
> 
> Am I missing anything?

In case we need to destroy the event, we want to drop the
notification_lock. So to avoid a situation where set_event_response() drops
a lock it did not acquire (which is not very intuitive), I've decided for
the less efficient scheme of dropping and retaking the lock.

But maybe with better function name and some asserts, we could live with
dropping the lock inside the function without taking it.

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