Re: [PATCH 13/22] fanotify: Release SRCU lock when waiting for userspace response

From: Jan Kara <jack@suse.cz>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jan Kara <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org,
	Amir Goldstein <amir73il@gmail.com>,
	Paul Moore <paul@paul-moore.com>
Subject: Re: [PATCH 13/22] fanotify: Release SRCU lock when waiting for userspace response
Date: Tue, 31 Jan 2017 14:28:49 +0100	[thread overview]
Message-ID: <20170131132849.GA14556@quack2.suse.cz> (raw)
In-Reply-To: <CAJfpegsCQ4nw_N33uZXLRLQ-EuE39Hw=T7oqmqo0KMfS4Mb8JA@mail.gmail.com>

On Wed 25-01-17 16:22:12, Miklos Szeredi wrote:
> On Fri, Jan 20, 2017 at 2:21 PM, Jan Kara <jack@suse.cz> wrote:
> > When userspace task processing fanotify permission events screws up and
> > does not respond, fsnotify_mark_srcu SRCU is held indefinitely which
> > causes further hangs in the whole notification subsystem. Although we
> > cannot easily solve the problem of operations blocked waiting for
> > response from userspace, we can at least somewhat localize the damage by
> > dropping SRCU lock before waiting for userspace response and reacquiring
> > it when userspace responds.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/notify/fanotify/fanotify.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 2e8ca885fb3e..98d7dc94d34c 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -61,14 +61,28 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
> >
> >  #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> >  static int fanotify_get_response(struct fsnotify_group *group,
> > -                                struct fanotify_perm_event_info *event)
> > +                                struct fsnotify_mark *inode_mark,
> > +                                struct fsnotify_mark *vfsmount_mark,
> > +                                struct fanotify_perm_event_info *event,
> > +                                int *srcu_idx)
> 
> Should these (inode_mark, vfsmount_mark, srcu_idx) be passed in an
> opaque struct to simplify the interface?

After some thought, having an opaque struct containing also mark pointers
and pass it around instead of srcu_idx looks like a neat idea. We will have
mark pointers in two places (explicit arguments of ->handle_event and the
opaque struct) but that should not be a problem as notification frameworks
cannot play any tricks with them anyway as it would break iteration in the
generic notification core. I'll do this, thanks for the idea.

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