RE: Re: [PATCH] fanotify: allow freeze on suspend when waiting for response from userspace

[Kunal Shubham]

>> On Fri 16-02-18 15:14:40, t.vivek@xxxxxxxxxxx wrote:
>> From: Vivek Trivedi <t.vivek@xxxxxxxxxxx>
>> 
>> 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@xxxxxxx>
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@xxxxxxxxxxx>
CC : jack@xxxxxxx, amir73il@xxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, PANKAJ MISHRA<pankaj.m@xxxxxxxxxxx>, Kunal Shubham<k.shubham@xxxxxxxxxxx>
Â
OnÂFriÂ16-02-18Â15:14:40,Ât.vivek@xxxxxxxxxxxÂwrote:
>ÂFrom:ÂVivekÂTrivediÂ<t.vivek@xxxxxxxxxxx>
>Â
>Â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@xxxxxxxx>
SUSEÂLabs,ÂCR
Â
Â