| Message ID | 20170120132123.9670-14-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Alternatively, waiting could be done by fsnotify core (a-la poll). That's a bit bigger change though. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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) { int ret; pr_debug("%s: group=%p event=%p\n", __func__, group, event); + /* + * fsnotify_prepare_user_wait() fails if we race with mark deletion. + * Just let the operation pass in that case. + */ + if (!fsnotify_prepare_user_wait(inode_mark, vfsmount_mark, srcu_idx)) { + event->response = FAN_ALLOW; + goto out; + } + wait_event(group->fanotify_data.access_waitq, event->response); + fsnotify_finish_user_wait(inode_mark, vfsmount_mark, srcu_idx); +out: /* userspace responded, convert to something usable */ switch (event->response) { case FAN_ALLOW: @@ -220,7 +234,8 @@ static int fanotify_handle_event(struct fsnotify_group *group, #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS if (mask & FAN_ALL_PERM_EVENTS) { - ret = fanotify_get_response(group, FANOTIFY_PE(fsn_event)); + ret = fanotify_get_response(group, inode_mark, fanotify_mark, + FANOTIFY_PE(fsn_event), srcu_idx); fsnotify_destroy_event(group, fsn_event); } #endif