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;
From:   Orion Poplawski <orion@nwra.com>
Subject: Re: Re: [PATCH] fanotify: allow freeze on suspend when waiting for
 response from userspace
To:     linux-kernel@vger.kernel.org, Vivek Trivedi <t.vivek@samsung.com>,
        Jan Kara <jack@suse.com>
Organization: NorthWest Research Associates
Message-ID: <c1bb16b7-9eee-9cea-2c96-a512d8b3b9c7@nwra.com>
Date:   Sat, 29 Dec 2018 15:04:48 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.0
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> 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
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Is there any progress here?  This has become a real pain for us while 
running BitDefender on EL7 laptops.  I tried applying the following to 
the EL7 kernel:

diff -up 
linux-3.10.0-957.1.3.el7.x86_64/fs/notify/fanotify/fanotify.c.orig 
kernel-3.10.0-957.1.3.el7/linux-3.10.0-957.1.3.el7.x86_64/fs/notify/fanotify/fanotify.c
--- linux-3.10.0-957.1.3.el7.x86_64/fs/notify/fanotify/fanotify.c.orig 
2018-11-15 10:07:13.000000000 -0700
+++ linux-3.10.0-957.1.3.el7.x86_64/fs/notify/fanotify/fanotify.c 
2018-12-28 15:44:26.452895337 -0700
@@ -9,6 +9,7 @@
  #include <linux/types.h>
  #include <linux/wait.h>
  #include <linux/audit.h>
+#include <linux/freezer.h>

  #include "fanotify.h"

@@ -64,7 +65,12 @@ static int fanotify_get_response(struct

         pr_debug("%s: group=%p event=%p\n", __func__, group, event);

-       wait_event(group->fanotify_data.access_waitq, event->response);
+       while (!event->response) {
+               ret = 
wait_event_freezable(group->fanotify_data.access_waitq,
+                                          event->response);
+               if (ret < 0)
+                       return ret;
+       }

         /* userspace responded, convert to something usable */
         switch (event->response & ~FAN_AUDIT) {

but I get a kernel panic shortly after logging in to the system.

-- 
Orion Poplawski
Manager of NWRA Technical Systems          720-772-5637
NWRA, Boulder/CoRA Office             FAX: 303-415-9702
3380 Mitchell Lane                       orion@nwra.com
Boulder, CO 80301                 https://www.nwra.com/