Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Mime-Version: 1.0
Subject: RE: Re: [PATCH] fanotify: allow freeze on suspend when waiting for
 response from userspace
Reply-To: k.shubham@samsung.com
From:   Kunal Shubham <k.shubham@samsung.com>
To:     "jack@suse.cz" <jack@suse.cz>
CC:     VIVEK TRIVEDI <t.vivek@samsung.com>,
        "amir73il@gmail.com" <amir73il@gmail.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        PANKAJ MISHRA <pankaj.m@samsung.com>,
        Kunal Shubham <k.shubham@samsung.com>
In-Reply-To: <20180216102913.jcgvc5rhjqtzlkb6@quack2.suse.cz>
Message-ID: <20180222094454epcms5p15e3538bdba8bcbc036dc379583bc3a55@epcms5p1>
Date:   Thu, 22 Feb 2018 15:14:54 +0530
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <20180216102913.jcgvc5rhjqtzlkb6@quack2.suse.cz>
        <1518774280-38090-1-git-send-email-t.vivek@samsung.com>
        <CGME20180216094525epcas5p1e52ea815ed7dfb7516578384dedcf5ae@epcms5p1>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

>> On Fri 16-02-18 15:14:40, t.vivek=40samsung.com wrote:
>> From: Vivek Trivedi <t.vivek=40samsung.com>
>>=20
>> 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.
>>=20
>> Use freeze-friendly wait API to handle this issue.
>>=20
>> Same problem was reported here:
>> https://bbs.archlinux.org/viewtopic.php?id=3D232270
>>=20
>> Freezing of tasks failed after 20.005 seconds
>> (1 tasks refusing to freeze, wq_busy=3D0)
>>=20
>> Backtrace:
>> =5B<c0582f80>=5D (__schedule) from =5B<c05835d0>=5D (schedule+0x4c/0xa4)
>> =5B<c0583584>=5D (schedule) from =5B<c01cb648>=5D (fanotify_handle_event=
+0x1c8/0x218)
>> =5B<c01cb480>=5D (fanotify_handle_event) from =5B<c01c8238>=5D (fsnotify=
+0x17c/0x38c)
>> =5B<c01c80bc>=5D (fsnotify) from =5B<c02676dc>=5D (security_file_open+0x=
88/0x8c)
>> =5B<c0267654>=5D (security_file_open) from =5B<c01854b0>=5D (do_dentry_o=
pen+0xc0/0x338)
>> =5B<c01853f0>=5D (do_dentry_open) from =5B<c0185a38>=5D (vfs_open+0x54/0=
x58)
>> =5B<c01859e4>=5D (vfs_open) from =5B<c0195480>=5D (do_last.isra.10+0x45c=
/0xcf8)
>> =5B<c0195024>=5D (do_last.isra.10) from =5B<c0196140>=5D (path_openat+0x=
424/0x600)
>> =5B<c0195d1c>=5D (path_openat) from =5B<c0197498>=5D (do_filp_open+0x3c/=
0x98)
>> =5B<c019745c>=5D (do_filp_open) from =5B<c0186b44>=5D (do_sys_open+0x120=
/0x1e4)
>> =5B<c0186a24>=5D (do_sys_open) from =5B<c0186c30>=5D (SyS_open+0x28/0x2c=
)
>> =5B<c0186c08>=5D (SyS_open) from =5B<c0010200>=5D (__sys_trace_return+0x=
0/0x20)
>
> Yeah, good catch.
>
>> =40=40 -63,7 +64,9 =40=40 static int fanotify_get_response(struct fsnoti=
fy_group *group,
>> =20
>>  	pr_debug(=22%s: group=3D%p event=3D%p=5Cn=22, __func__, group, event);
>> =20
>> -	wait_event(group->fanotify_data.access_waitq, event->response);
>> +	while (=21event->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?=20
If agree, I will send v2 patch.

=40=40 -63,7 +64,11 =40=40 static int fanotify_get_response(struct fsnotify=
_group *group,

        pr_debug(=22%s: group=3D%p event=3D%p=5Cn=22, __func__, group, even=
t);

-       wait_event(group->fanotify_data.access_waitq, event->response);
+       while (=21event->response) =7B
+               if (wait_event_freezable(group->fanotify_data.access_waitq,
+                                       event->response))
+                       flush_signals(current);
+       =7D

Thanks

=C2=A0=0D=0A---------=20Original=20Message=20---------=0D=0ASender=20:=20Ja=
n=20Kara=C2=A0<jack=40suse.cz>=0D=0ADate=20=20=20:=202018-02-16=2015:59=20(=
GMT+5:30)=0D=0ATitle=20=20:=20Re:=20=5BPATCH=5D=20fanotify:=20allow=20freez=
e=20on=20suspend=20when=20waiting=20for=20response=20from=20userspace=0D=0A=
To=20:=20VIVEK=20TRIVEDI<t.vivek=40samsung.com>=0D=0ACC=20:=20jack=40suse.c=
z,=20amir73il=40gmail.com,=20linux-fsdevel=40vger.kernel.org,=20linux-kerne=
l=40vger.kernel.org,=20PANKAJ=20MISHRA<pankaj.m=40samsung.com>,=20Kunal=20S=
hubham<k.shubham=40samsung.com>=0D=0A=C2=A0=0D=0AOn=C2=A0Fri=C2=A016-02-18=
=C2=A015:14:40,=C2=A0t.vivek=40samsung.com=C2=A0wrote:=0D=0A>=C2=A0From:=C2=
=A0Vivek=C2=A0Trivedi=C2=A0<t.vivek=40samsung.com>=0D=0A>=C2=A0=0D=0A>=C2=
=A0If=C2=A0fanotify=C2=A0userspace=C2=A0response=C2=A0server=C2=A0thread=C2=
=A0is=C2=A0frozen=C2=A0first,=0D=0A>=C2=A0it=C2=A0may=C2=A0fail=C2=A0to=C2=
=A0send=C2=A0response=C2=A0from=C2=A0userspace=C2=A0to=C2=A0kernel=C2=A0spa=
ce=C2=A0listener.=0D=0A>=C2=A0In=C2=A0this=C2=A0scenario,=C2=A0fanotify=C2=
=A0response=C2=A0listener=C2=A0will=C2=A0never=C2=A0get=C2=A0response=0D=0A=
>=C2=A0from=C2=A0userepace=C2=A0and=C2=A0fail=C2=A0to=C2=A0suspend.=0D=0A>=
=C2=A0=0D=0A>=C2=A0Use=C2=A0freeze-friendly=C2=A0wait=C2=A0API=C2=A0to=C2=
=A0handle=C2=A0this=C2=A0issue.=0D=0A>=C2=A0=0D=0A>=C2=A0Same=C2=A0problem=
=C2=A0was=C2=A0reported=C2=A0here:=0D=0A>=C2=A0https://bbs.archlinux.org/vi=
ewtopic.php?id=3D232270=0D=0A>=C2=A0=0D=0A>=C2=A0Freezing=C2=A0of=C2=A0task=
s=C2=A0failed=C2=A0after=C2=A020.005=C2=A0seconds=0D=0A>=C2=A0(1=C2=A0tasks=
=C2=A0refusing=C2=A0to=C2=A0freeze,=C2=A0wq_busy=3D0)=0D=0A>=C2=A0=0D=0A>=
=C2=A0Backtrace:=0D=0A>=C2=A0=5B<c0582f80>=5D=C2=A0(__schedule)=C2=A0from=
=C2=A0=5B<c05835d0>=5D=C2=A0(schedule+0x4c/0xa4)=0D=0A>=C2=A0=5B<c0583584>=
=5D=C2=A0(schedule)=C2=A0from=C2=A0=5B<c01cb648>=5D=C2=A0(fanotify_handle_e=
vent+0x1c8/0x218)=0D=0A>=C2=A0=5B<c01cb480>=5D=C2=A0(fanotify_handle_event)=
=C2=A0from=C2=A0=5B<c01c8238>=5D=C2=A0(fsnotify+0x17c/0x38c)=0D=0A>=C2=A0=
=5B<c01c80bc>=5D=C2=A0(fsnotify)=C2=A0from=C2=A0=5B<c02676dc>=5D=C2=A0(secu=
rity_file_open+0x88/0x8c)=0D=0A>=C2=A0=5B<c0267654>=5D=C2=A0(security_file_=
open)=C2=A0from=C2=A0=5B<c01854b0>=5D=C2=A0(do_dentry_open+0xc0/0x338)=0D=
=0A>=C2=A0=5B<c01853f0>=5D=C2=A0(do_dentry_open)=C2=A0from=C2=A0=5B<c0185a3=
8>=5D=C2=A0(vfs_open+0x54/0x58)=0D=0A>=C2=A0=5B<c01859e4>=5D=C2=A0(vfs_open=
)=C2=A0from=C2=A0=5B<c0195480>=5D=C2=A0(do_last.isra.10+0x45c/0xcf8)=0D=0A>=
=C2=A0=5B<c0195024>=5D=C2=A0(do_last.isra.10)=C2=A0from=C2=A0=5B<c0196140>=
=5D=C2=A0(path_openat+0x424/0x600)=0D=0A>=C2=A0=5B<c0195d1c>=5D=C2=A0(path_=
openat)=C2=A0from=C2=A0=5B<c0197498>=5D=C2=A0(do_filp_open+0x3c/0x98)=0D=0A=
>=C2=A0=5B<c019745c>=5D=C2=A0(do_filp_open)=C2=A0from=C2=A0=5B<c0186b44>=5D=
=C2=A0(do_sys_open+0x120/0x1e4)=0D=0A>=C2=A0=5B<c0186a24>=5D=C2=A0(do_sys_o=
pen)=C2=A0from=C2=A0=5B<c0186c30>=5D=C2=A0(SyS_open+0x28/0x2c)=0D=0A>=C2=A0=
=5B<c0186c08>=5D=C2=A0(SyS_open)=C2=A0from=C2=A0=5B<c0010200>=5D=C2=A0(__sy=
s_trace_return+0x0/0x20)=0D=0A=C2=A0=0D=0AYeah,=C2=A0good=C2=A0catch.=0D=0A=
=C2=A0=0D=0A>=C2=A0=40=40=C2=A0-63,7=C2=A0+64,9=C2=A0=40=40=C2=A0static=C2=
=A0int=C2=A0fanotify_get_response(struct=C2=A0fsnotify_group=C2=A0*group,=
=0D=0A>=C2=A0=C2=A0=0D=0A>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0pr_debug(=22%s:=C2=A0group=3D%p=C2=A0event=3D%p=5Cn=22,=C2=A0__=
func__,=C2=A0group,=C2=A0event);=0D=0A>=C2=A0=C2=A0=0D=0A>=C2=A0-=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0wait_event(group->fanotify_data.acce=
ss_waitq,=C2=A0event->response);=0D=0A>=C2=A0+=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0while=C2=A0(=21event->response)=0D=0A>=C2=A0+=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0wait_event_freezable(group->fanotify_data.access_waitq,=0D=0A>=
=C2=A0+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0event->response);=0D=0A=C2=A0=0D=0ABut=C2=A0if=C2=A0the=C2=A0process=
=C2=A0gets=C2=A0a=C2=A0signal=C2=A0while=C2=A0waiting,=C2=A0we=C2=A0will=C2=
=A0just=C2=A0livelock=C2=A0the=0D=0Akernel=C2=A0in=C2=A0this=C2=A0loop=C2=
=A0as=C2=A0wait_event_freezable()=C2=A0will=C2=A0keep=C2=A0returning=0D=0AE=
RESTARTSYS.=C2=A0So=C2=A0you=C2=A0need=C2=A0to=C2=A0be=C2=A0a=C2=A0bit=C2=
=A0more=C2=A0clever=C2=A0here...=0D=0A=C2=A0=0D=0A=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Honza=0D=0A--=
=C2=A0=0D=0AJan=C2=A0Kara=C2=A0<jack=40suse.com>=0D=0ASUSE=C2=A0Labs,=C2=A0=
CR=0D=0A=C2=A0=0D=0A=C2=A0