| Message ID | 1518774280-38090-1-git-send-email-t.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 16-02-18 15:14:40, t.vivek@samsung.com wrote: > From: Vivek Trivedi <t.vivek@samsung.com> > > If fanotify userspace response server thread is frozen first, > it may fail to send response from userspace to kernel space listener. > In this scenario, fanotify response listener will never get response > from userepace and fail to suspend. > > Use freeze-friendly wait API to handle this issue. > > Same problem was reported here: > https://bbs.archlinux.org/viewtopic.php?id=232270 > > Freezing of tasks failed after 20.005 seconds > (1 tasks refusing to freeze, wq_busy=0) > > Backtrace: > [<c0582f80>] (__schedule) from [<c05835d0>] (schedule+0x4c/0xa4) > [<c0583584>] (schedule) from [<c01cb648>] (fanotify_handle_event+0x1c8/0x218) > [<c01cb480>] (fanotify_handle_event) from [<c01c8238>] (fsnotify+0x17c/0x38c) > [<c01c80bc>] (fsnotify) from [<c02676dc>] (security_file_open+0x88/0x8c) > [<c0267654>] (security_file_open) from [<c01854b0>] (do_dentry_open+0xc0/0x338) > [<c01853f0>] (do_dentry_open) from [<c0185a38>] (vfs_open+0x54/0x58) > [<c01859e4>] (vfs_open) from [<c0195480>] (do_last.isra.10+0x45c/0xcf8) > [<c0195024>] (do_last.isra.10) from [<c0196140>] (path_openat+0x424/0x600) > [<c0195d1c>] (path_openat) from [<c0197498>] (do_filp_open+0x3c/0x98) > [<c019745c>] (do_filp_open) from [<c0186b44>] (do_sys_open+0x120/0x1e4) > [<c0186a24>] (do_sys_open) from [<c0186c30>] (SyS_open+0x28/0x2c) > [<c0186c08>] (SyS_open) from [<c0010200>] (__sys_trace_return+0x0/0x20) Yeah, good catch. > @@ -63,7 +64,9 @@ 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->response); > + while (!event->response) > + wait_event_freezable(group->fanotify_data.access_waitq, > + event->response); But if the process gets a signal while waiting, we will just livelock the kernel in this loop as wait_event_freezable() will keep returning ERESTARTSYS. So you need to be a bit more clever here... Honza
>> On Fri 16-02-18 15:14:40, t.vivek@samsung.com wrote: >> From: Vivek Trivedi <t.vivek@samsung.com> >> >> If fanotify userspace response server thread is frozen first, >> it may fail to send response from userspace to kernel space listener. >> In this scenario, fanotify response listener will never get response >> from userepace and fail to suspend. >> >> Use freeze-friendly wait API to handle this issue. >> >> Same problem was reported here: >> https://bbs.archlinux.org/viewtopic.php?id=232270 >> >> Freezing of tasks failed after 20.005 seconds >> (1 tasks refusing to freeze, wq_busy=0) >> >> Backtrace: >> [<c0582f80>] (__schedule) from [<c05835d0>] (schedule+0x4c/0xa4) >> [<c0583584>] (schedule) from [<c01cb648>] (fanotify_handle_event+0x1c8/0x218) >> [<c01cb480>] (fanotify_handle_event) from [<c01c8238>] (fsnotify+0x17c/0x38c) >> [<c01c80bc>] (fsnotify) from [<c02676dc>] (security_file_open+0x88/0x8c) >> [<c0267654>] (security_file_open) from [<c01854b0>] (do_dentry_open+0xc0/0x338) >> [<c01853f0>] (do_dentry_open) from [<c0185a38>] (vfs_open+0x54/0x58) >> [<c01859e4>] (vfs_open) from [<c0195480>] (do_last.isra.10+0x45c/0xcf8) >> [<c0195024>] (do_last.isra.10) from [<c0196140>] (path_openat+0x424/0x600) >> [<c0195d1c>] (path_openat) from [<c0197498>] (do_filp_open+0x3c/0x98) >> [<c019745c>] (do_filp_open) from [<c0186b44>] (do_sys_open+0x120/0x1e4) >> [<c0186a24>] (do_sys_open) from [<c0186c30>] (SyS_open+0x28/0x2c) >> [<c0186c08>] (SyS_open) from [<c0010200>] (__sys_trace_return+0x0/0x20) > > Yeah, good catch. > >> @@ -63,7 +64,9 @@ 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->response); >> + while (!event->response) >> + wait_event_freezable(group->fanotify_data.access_waitq, >> + event->response); > > But if the process gets a signal while waiting, we will just livelock the > kernel in this loop as wait_event_freezable() will keep returning > ERESTARTSYS. So you need to be a bit more clever here... Hi Jack, Thanks for the quick review. To avoid livelock issue, is it fine to use below change? If agree, I will send v2 patch. @@ -63,7 +64,11 @@ 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->response); + while (!event->response) { + if (wait_event_freezable(group->fanotify_data.access_waitq, + event->response)) + flush_signals(current); + } Thanks --------- Original Message --------- Sender : Jan Kara <jack@suse.cz> Date : 2018-02-16 15:59 (GMT+5:30) Title : Re: [PATCH] fanotify: allow freeze on suspend when waiting for response from userspace To : VIVEK TRIVEDI<t.vivek@samsung.com> CC : jack@suse.cz, amir73il@gmail.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, PANKAJ MISHRA<pankaj.m@samsung.com>, Kunal Shubham<k.shubham@samsung.com> On Fri 16-02-18 15:14:40, t.vivek@samsung.com wrote: > From: Vivek Trivedi <t.vivek@samsung.com> > > If fanotify userspace response server thread is frozen first, > it may fail to send response from userspace to kernel space listener. > In this scenario, fanotify response listener will never get response > from userepace and fail to suspend. > > Use freeze-friendly wait API to handle this issue. > > Same problem was reported here: > https://bbs.archlinux.org/viewtopic.php?id=232270 > > Freezing of tasks failed after 20.005 seconds > (1 tasks refusing to freeze, wq_busy=0) > > Backtrace: > [<c0582f80>] (__schedule) from [<c05835d0>] (schedule+0x4c/0xa4) > [<c0583584>] (schedule) from [<c01cb648>] (fanotify_handle_event+0x1c8/0x218) > [<c01cb480>] (fanotify_handle_event) from [<c01c8238>] (fsnotify+0x17c/0x38c) > [<c01c80bc>] (fsnotify) from [<c02676dc>] (security_file_open+0x88/0x8c) > [<c0267654>] (security_file_open) from [<c01854b0>] (do_dentry_open+0xc0/0x338) > [<c01853f0>] (do_dentry_open) from [<c0185a38>] (vfs_open+0x54/0x58) > [<c01859e4>] (vfs_open) from [<c0195480>] (do_last.isra.10+0x45c/0xcf8) > [<c0195024>] (do_last.isra.10) from [<c0196140>] (path_openat+0x424/0x600) > [<c0195d1c>] (path_openat) from [<c0197498>] (do_filp_open+0x3c/0x98) > [<c019745c>] (do_filp_open) from [<c0186b44>] (do_sys_open+0x120/0x1e4) > [<c0186a24>] (do_sys_open) from [<c0186c30>] (SyS_open+0x28/0x2c) > [<c0186c08>] (SyS_open) from [<c0010200>] (__sys_trace_return+0x0/0x20) Yeah, good catch. > @@ -63,7 +64,9 @@ 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->response); > + while (!event->response) > + wait_event_freezable(group->fanotify_data.access_waitq, > + event->response); But if the process gets a signal while waiting, we will just livelock the kernel in this loop as wait_event_freezable() will keep returning ERESTARTSYS. So you need to be a bit more clever here... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
On Thu 22-02-18 15:14:54, Kunal Shubham wrote: > >> On Fri 16-02-18 15:14:40, t.vivek@samsung.com wrote: > >> From: Vivek Trivedi <t.vivek@samsung.com> > >> > >> If fanotify userspace response server thread is frozen first, > >> it may fail to send response from userspace to kernel space listener. > >> In this scenario, fanotify response listener will never get response > >> from userepace and fail to suspend. > >> > >> Use freeze-friendly wait API to handle this issue. > >> > >> Same problem was reported here: > >> https://bbs.archlinux.org/viewtopic.php?id=232270 > >> > >> Freezing of tasks failed after 20.005 seconds > >> (1 tasks refusing to freeze, wq_busy=0) > >> > >> Backtrace: > >> [<c0582f80>] (__schedule) from [<c05835d0>] (schedule+0x4c/0xa4) > >> [<c0583584>] (schedule) from [<c01cb648>] (fanotify_handle_event+0x1c8/0x218) > >> [<c01cb480>] (fanotify_handle_event) from [<c01c8238>] (fsnotify+0x17c/0x38c) > >> [<c01c80bc>] (fsnotify) from [<c02676dc>] (security_file_open+0x88/0x8c) > >> [<c0267654>] (security_file_open) from [<c01854b0>] (do_dentry_open+0xc0/0x338) > >> [<c01853f0>] (do_dentry_open) from [<c0185a38>] (vfs_open+0x54/0x58) > >> [<c01859e4>] (vfs_open) from [<c0195480>] (do_last.isra.10+0x45c/0xcf8) > >> [<c0195024>] (do_last.isra.10) from [<c0196140>] (path_openat+0x424/0x600) > >> [<c0195d1c>] (path_openat) from [<c0197498>] (do_filp_open+0x3c/0x98) > >> [<c019745c>] (do_filp_open) from [<c0186b44>] (do_sys_open+0x120/0x1e4) > >> [<c0186a24>] (do_sys_open) from [<c0186c30>] (SyS_open+0x28/0x2c) > >> [<c0186c08>] (SyS_open) from [<c0010200>] (__sys_trace_return+0x0/0x20) > > > > Yeah, good catch. > > > >> @@ -63,7 +64,9 @@ 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->response); > >> + while (!event->response) > >> + wait_event_freezable(group->fanotify_data.access_waitq, > >> + event->response); > > > > But if the process gets a signal while waiting, we will just livelock the > > kernel in this loop as wait_event_freezable() will keep returning > > ERESTARTSYS. So you need to be a bit more clever here... > > Hi Jack, > Thanks for the quick review. > To avoid livelock issue, is it fine to use below change? > If agree, I will send v2 patch. > > @@ -63,7 +64,11 @@ 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->response); > + while (!event->response) { > + if (wait_event_freezable(group->fanotify_data.access_waitq, > + event->response)) > + flush_signals(current); > + } Hum, I don't think this is correct either as this way if any signal was delivered while waiting for fanotify response, we'd just lose it while previously it has been properly handled. So what I think needs to be done is that we just use wait_event_freezable() and propagate non-zero return value (-ERESTARTSYS) up to the caller to handle the signal and restart the syscall as necessary. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 6702a6a..1d65899 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -11,6 +11,7 @@ #include <linux/types.h> #include <linux/wait.h> #include <linux/audit.h> +#include <linux/freezer.h> #include "fanotify.h" @@ -63,7 +64,9 @@ 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->response); + while (!event->response) + wait_event_freezable(group->fanotify_data.access_waitq, + event->response); /* userspace responded, convert to something usable */ switch (event->response & ~FAN_AUDIT) {