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

[Jan Kara <jack@suse.cz>]

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

On Tue 08-01-19 17:46:07, 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 - I'll
> send that separately later. 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/
> 

Patch adding LTP test I have created for excercising new code is attached.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-syscalls-fanotify12-New-test-to-test-signal-handling.patch --]
[-- Type: text/x-patch, Size: 5705 bytes --]

>From 53bdff3951596f8084127998c085d4ff94667873 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 8 Jan 2019 17:01:40 +0100
Subject: [PATCH] syscalls/fanotify12: New test to test signal handling for
 permission events

Test whether kernel survives when processes waiting for response to
fanotify permission event are interrupted by a signal.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 testcases/kernel/syscalls/fanotify/.gitignore   |   1 +
 testcases/kernel/syscalls/fanotify/fanotify12.c | 217 ++++++++++++++++++++++++
 2 files changed, 218 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify12.c

diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index 3818e241ffcf..d583d459b4aa 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -9,3 +9,4 @@
 /fanotify09
 /fanotify10
 /fanotify11
+/fanotify12
diff --git a/testcases/kernel/syscalls/fanotify/fanotify12.c b/testcases/kernel/syscalls/fanotify/fanotify12.c
new file mode 100644
index 000000000000..462c85031c0b
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify12.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2019 SUSE.  All Rights Reserved.
+ *
+ * Started by Jan Kara <jack@suse.cz>
+ *
+ * DESCRIPTION
+ *     Check how fanotify permission events deal with signals.
+ */
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/fcntl.h>
+#include <sys/wait.h>
+#include <errno.h>
+#include <string.h>
+#include <signal.h>
+#include <sys/syscall.h>
+#include "tst_test.h"
+#include "lapi/syscalls.h"
+#include "fanotify.h"
+
+#if defined(HAVE_SYS_FANOTIFY_H)
+#include <sys/fanotify.h>
+
+#define BUF_SIZE 256
+static char fname[BUF_SIZE];
+static char buf[BUF_SIZE];
+static volatile int fd_notify;
+
+/* Number of children we start */
+#define MAX_CHILDREN 16
+static pid_t child_pid[MAX_CHILDREN];
+
+/* Number of events we process before stopping */
+#define MAX_EVENTS 10000
+
+static void generate_events(void)
+{
+	int fd;
+
+	/*
+	 * generate sequence of events
+	 */
+	if ((fd = open(fname, O_RDWR | O_CREAT, 0700)) == -1)
+		exit(1);
+
+	/* Run until killed... */
+	while (1) {
+		lseek(fd, 0, SEEK_SET);
+		if (read(fd, buf, BUF_SIZE) == -1)
+			exit(3);
+	}
+}
+
+static void usrhandler(int sig)
+{
+	/* Do nothing */
+}
+
+static void send_signals(void)
+{
+	while (1) {
+		usleep(random() % 200);
+		SAFE_KILL(child_pid[random() % (MAX_CHILDREN-1)], SIGUSR1);
+	}
+}
+
+static void run_children(void)
+{
+	int i;
+
+	for (i = 0; i < MAX_CHILDREN; i++) {
+		child_pid[i] = SAFE_FORK();
+		if (!child_pid[i]) {
+			/* Child will generate events now */
+			close(fd_notify);
+			if (i == MAX_CHILDREN - 1) {
+				send_signals();
+			} else {
+				SAFE_SIGNAL(SIGUSR1, usrhandler);
+				generate_events();
+			}
+			exit(0);
+		}
+	}
+}
+
+static int stop_children(void)
+{
+	int child_ret;
+	int i, ret = 0;
+
+	for (i = 0; i < MAX_CHILDREN; i++)
+		SAFE_KILL(child_pid[i], SIGKILL);
+
+	for (i = 0; i < MAX_CHILDREN; i++) {
+		SAFE_WAITPID(child_pid[i], &child_ret, 0);
+		if (!WIFSIGNALED(child_ret))
+			ret = 1;
+	}
+
+	return ret;
+}
+
+static int setup_instance(void)
+{
+	int fd;
+
+	fd = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
+
+	if (fanotify_mark(fd, FAN_MARK_ADD, FAN_ACCESS_PERM, AT_FDCWD,
+			  fname) < 0) {
+		close(fd);
+		if (errno == EINVAL) {
+			tst_brk(TCONF | TERRNO,
+				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
+				"configured in kernel?");
+		} else {
+			tst_brk(TBROK | TERRNO,
+				"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS_PERM, "
+				"AT_FDCWD, %s) failed.", fd, fname);
+		}
+	}
+
+	return fd;
+}
+
+static void handle_fanotify_events(void)
+{
+	int events = 0;
+
+	/*
+	 * check events
+	 */
+	while (events < MAX_EVENTS) {
+		struct fanotify_event_metadata event;
+		struct fanotify_response resp;
+
+		/* Sleep randomly to give time for signal to be delivered */
+		usleep(random() % 1000);
+
+		/* Get more events */
+		SAFE_READ(1, fd_notify, &event, sizeof(event));
+
+		if (event.mask != FAN_ACCESS_PERM) {
+			tst_res(TFAIL,
+				"got event: mask=%llx (expected %llx) "
+				"pid=%u fd=%d",
+				(unsigned long long)event.mask,
+				(unsigned long long)FAN_ACCESS_PERM,
+				(unsigned)event.pid, event.fd);
+			break;
+		}
+
+		/* Sleep randomly to give time for signal to be delivered */
+		usleep(random() % 1000);
+		/* Write response to permission event */
+		resp.fd = event.fd;
+		resp.response = FAN_ALLOW;
+		SAFE_WRITE(1, fd_notify, &resp, sizeof(resp));
+		SAFE_CLOSE(event.fd);
+		events++;
+	}
+}
+
+static void test_fanotify(void)
+{
+	int ret;
+
+	fd_notify = setup_instance();
+	run_children();
+	handle_fanotify_events();
+
+	/*
+	 * Now destroy the fanotify instance while there are permission
+	 * events at various stages of processing. This may provoke
+	 * kernel hangs or crashes.
+	 */
+	SAFE_CLOSE(fd_notify);
+
+	ret = stop_children();
+	if (ret)
+		tst_res(TFAIL, "child exited for unexpected reason");
+	else
+		tst_res(TPASS, "all children exited successfully");
+}
+
+static void setup(void)
+{
+	sprintf(fname, "fname_%d", getpid());
+	SAFE_FILE_PRINTF(fname, "%s", fname);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+}
+
+static struct tst_test test = {
+	.test_all = test_fanotify,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.needs_root = 1,
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
-- 
2.16.4