Re: [PATCH v2 0/6] fanotify: Make wait for permission event response interruptible

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]

Sorry, forgot to attach the patch...

								Honza

On Thu 21-02-19 11:53:10, Jan Kara wrote:
> On Wed 20-02-19 10:27:04, Orion Poplawski wrote:
> > On 2/13/19 7:54 AM, Jan Kara wrote:
> > > Hello,
> > > 
> > > When waiting for response to fanotify permission events, we currently use
> > > uninterruptible waits. That makes code simple however it can cause lots of
> > > processes to end up in uninterruptible sleep with hard reboot being the only
> > > alternative in case fanotify listener process stops responding (e.g. due to a
> > > bug in its implementation) - reported e.g. in [1]. Uninterruptible sleep also
> > > makes system hibernation fail if the listener gets frozen before the process
> > > generating fanotify permission event (as reported e.g. here [2]).
> > > 
> > > This patch set modifies fanotify so that it will use interruptible wait when
> > > waiting for fanotify permission event response. Patches are based on current
> > > Linus' tree for the ease of testing (I plan to rebase them on top of Amir's
> > > pending changes later). I have also create LTP test which stresses handling of
> > > permission events while sending processes signals to test the new code [3]
> > > Review, comments, and testing are welcome.
> > > 
> > > [1] https://lore.kernel.org/lkml/153474898224.6806.12518115530793064797.stgit@buzz/
> > > [2] https://lore.kernel.org/lkml/c1bb16b7-9eee-9cea-2c96-a512d8b3b9c7@nwra.com/
> > > [3] https://lwn.net/ml/linux-fsdevel/20190108165307.GA11259@quack2.suse.cz/
> > > 
> > > Changes since v1:
> > > * leave pr_debug() calls alone (Amir)
> > > * simplify permission event state tracking (Amir)
> > > * split some changes into separate patches (Amir)
> > > 
> > > 								Honza
> > > 
> > 
> > I backported these patches to the RHEL7 kernel and have started running that.
> > One thing I've noticed are messages like the following at login time:
> > 
> > bash: /etc/bash_completion.d/itweb-settings.bash: Interrupted system call
> > 
> > I've commented on a bash bug report here
> > https://savannah.gnu.org/support/?109159
> 
> Thanks for trying these out! Yes, so the patches can definitely lead to
> EINTR returns from open(2) if there's fanotify permission event generated
> by it and the opening process has a signal pending. Now EINTR is documented
> as a possible return from open(2) but Marko is right that in practice
> open(2) on local filesystem never returns EINTR so programs just don't
> bother handling it. Since breaking userspace is no-go, we probably cannot
> apply the change as is.
> 
> What we can do easily is to change the wait_event_interruptible() to
> wait_event_killable(). This is what we commonly do when we want to allow
> administrator to interrupt a syscall but userspace is not prepared for
> EINTR. That will at least allow processes that are waiting for fanotify
> response to be killed. So I'll do this for the coming merge window
> (attached patch). However this will not solve your problems with
> hibernation as TASK_KILLABLE tasks cannot be hibernated AFAICS. I will have
> to talk with people more knowledgeable about hibernation if there's a
> solution to this.
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-fanotify-Make-waits-for-fanotify-events-only-killabl.patch --]
[-- Type: text/x-patch, Size: 1642 bytes --]

From b51905798195eeb427c873643b3ada0d7bd991a7 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Thu, 21 Feb 2019 11:47:23 +0100
Subject: [PATCH] fanotify: Make waits for fanotify events only killable

Making waits for response to fanotify permission events interruptible
can result in EINTR returns from open(2) or other syscalls when there's
e.g. AV software that's monitoring the file. Orion reports that e.g.
bash is complaining like:

bash: /etc/bash_completion.d/itweb-settings.bash: Interrupted system call

So for now convert the wait from interruptible to only killable one.
That is mostly invisible to userspace. Sadly this breaks hibernation
with fanotify permission events pending again but we have to put more
thought into how to fix this without regressing userspace visible
behavior.

Reported-by: Orion Poplawski <orion@nwra.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index ff7b8a1cdfe1..6b9c27548997 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -92,8 +92,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	ret = wait_event_interruptible(group->fanotify_data.access_waitq,
-				       event->state == FAN_EVENT_ANSWERED);
+	ret = wait_event_killable(group->fanotify_data.access_waitq,
+				  event->state == FAN_EVENT_ANSWERED);
 	/* Signal pending? */
 	if (ret < 0) {
 		spin_lock(&group->notification_lock);
-- 
2.16.4