| Message ID | 20161222091538.28702-16-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 22, 2016 at 11:15 AM, 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. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- Looks good. one nit below. Reviewed-by: Amir Goldstein <amir73il@gmail.com> > fs/notify/fanotify/fanotify.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 2e8ca885fb3e..284d2d112ad2 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -61,7 +61,10 @@ 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; > > @@ -69,6 +72,15 @@ static int fanotify_get_response(struct fsnotify_group *group, > > wait_event(group->fanotify_data.access_waitq, event->response); > > + if (!fsnotify_prepare_user_wait(inode_mark, vfsmount_mark, srcu_idx)) { Since it is not clear for reader of this code the conditions where fsnotify_prepare_user_wait() can fail, a comment here would be nice to explain the choice of ALLOW > + 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 +232,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 > -- > 2.10.2 > -- 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 Mon 26-12-16 17:22:58, Amir Goldstein wrote: > > #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; > > > > @@ -69,6 +72,15 @@ static int fanotify_get_response(struct fsnotify_group *group, > > > > wait_event(group->fanotify_data.access_waitq, event->response); > > > > + if (!fsnotify_prepare_user_wait(inode_mark, vfsmount_mark, srcu_idx)) { > > Since it is not clear for reader of this code the conditions where > fsnotify_prepare_user_wait() can fail, a comment here would be nice > to explain the choice of ALLOW Good point. Will add. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 2e8ca885fb3e..284d2d112ad2 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -61,7 +61,10 @@ 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; @@ -69,6 +72,15 @@ static int fanotify_get_response(struct fsnotify_group *group, wait_event(group->fanotify_data.access_waitq, event->response); + 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 +232,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
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. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/notify/fanotify/fanotify.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)