From 8b73a4c5ae4ad7f11a47b133c26129acde549ac0 Mon Sep 17 00:00:00 2001
From: Alan Somers <asomers@FreeBSD.org>
Date: Fri, 10 May 2019 15:02:29 +0000
Subject: [PATCH] fusefs: fix running multiple daemons concurrently

When a FUSE daemon dies or closes /dev/fuse, all of that daemon's pending
requests must be terminated.  Previously that was done in /dev/fuse's
.d_close method.  However, d_close only gets called on the *last* close of
the device.  That means that if multiple daemons were running concurrently,
all but the last daemon to close would leave their I/O hanging around.  The
problem was easily visible just by running "kyua -v parallelism=2 test" in
fusefs's test directory.

Fix this bug by terminating a daemon's pending I/O during /dev/fuse's
cdvpriv dtor method instead.  That method runs on every close of a file.

Also, fix some potential races in the tests:
* Clear SA_RESTART when registering the daemon's signal handler so read(2)
  will return EINTR.
* Wait for the daemon to die before unmounting the mountpoint, so we won't
  see an unwanted FUSE_DESTROY operation in the mock file system.

Sponsored by:	The FreeBSD Foundation
---
 sys/fs/fuse/fuse_device.c     | 60 ++++++++++++++---------------------
 tests/sys/fs/fusefs/mockfs.cc | 28 ++++++++++------
 2 files changed, 42 insertions(+), 46 deletions(-)

diff --git a/sys/fs/fuse/fuse_device.c b/sys/fs/fuse/fuse_device.c
index c481a55083d3..2ab2c9d1b65b 100644
--- a/sys/fs/fuse/fuse_device.c
+++ b/sys/fs/fuse/fuse_device.c
@@ -94,14 +94,12 @@ SDT_PROBE_DEFINE2(fusefs, , device, trace, "int", "char*");
 static struct cdev *fuse_dev;
 
 static d_open_t fuse_device_open;
-static d_close_t fuse_device_close;
 static d_poll_t fuse_device_poll;
 static d_read_t fuse_device_read;
 static d_write_t fuse_device_write;
 
 static struct cdevsw fuse_device_cdevsw = {
 	.d_open = fuse_device_open,
-	.d_close = fuse_device_close,
 	.d_name = "fuse",
 	.d_poll = fuse_device_poll,
 	.d_read = fuse_device_read,
@@ -119,8 +117,31 @@ static void
 fdata_dtor(void *arg)
 {
 	struct fuse_data *fdata;
+	struct fuse_ticket *tick;
 
 	fdata = arg;
+	if (fdata == NULL)
+		return;
+
+	fdata_set_dead(fdata);
+
+	FUSE_LOCK();
+	fuse_lck_mtx_lock(fdata->aw_mtx);
+	/* wakup poll()ers */
+	selwakeuppri(&fdata->ks_rsel, PZERO + 1);
+	/* Don't let syscall handlers wait in vain */
+	while ((tick = fuse_aw_pop(fdata))) {
+		fuse_lck_mtx_lock(tick->tk_aw_mtx);
+		fticket_set_answered(tick);
+		tick->tk_aw_errno = ENOTCONN;
+		wakeup(tick);
+		fuse_lck_mtx_unlock(tick->tk_aw_mtx);
+		FUSE_ASSERT_AW_DONE(tick);
+		fuse_ticket_drop(tick);
+	}
+	fuse_lck_mtx_unlock(fdata->aw_mtx);
+	FUSE_UNLOCK();
+
 	fdata_trydestroy(fdata);
 }
 
@@ -144,41 +165,6 @@ fuse_device_open(struct cdev *dev, int oflags, int devtype, struct thread *td)
 	return (error);
 }
 
-static int
-fuse_device_close(struct cdev *dev, int fflag, int devtype, struct thread *td)
-{
-	struct fuse_data *data;
-	struct fuse_ticket *tick;
-	int error;
-
-	error = devfs_get_cdevpriv((void **)&data);
-	if (error != 0)
-		return (error);
-	if (!data)
-		panic("no fuse data upon fuse device close");
-	fdata_set_dead(data);
-
-	FUSE_LOCK();
-	fuse_lck_mtx_lock(data->aw_mtx);
-	/* wakup poll()ers */
-	selwakeuppri(&data->ks_rsel, PZERO + 1);
-	/* Don't let syscall handlers wait in vain */
-	while ((tick = fuse_aw_pop(data))) {
-		fuse_lck_mtx_lock(tick->tk_aw_mtx);
-		fticket_set_answered(tick);
-		tick->tk_aw_errno = ENOTCONN;
-		wakeup(tick);
-		fuse_lck_mtx_unlock(tick->tk_aw_mtx);
-		FUSE_ASSERT_AW_DONE(tick);
-		fuse_ticket_drop(tick);
-	}
-	fuse_lck_mtx_unlock(data->aw_mtx);
-	FUSE_UNLOCK();
-
-	SDT_PROBE2(fusefs, , device, trace, 1, "device close");
-	return (0);
-}
-
 int
 fuse_device_poll(struct cdev *dev, int events, struct thread *td)
 {
diff --git a/tests/sys/fs/fusefs/mockfs.cc b/tests/sys/fs/fusefs/mockfs.cc
index c80d2c8a5732..d3dabc1d17e6 100644
--- a/tests/sys/fs/fusefs/mockfs.cc
+++ b/tests/sys/fs/fusefs/mockfs.cc
@@ -147,7 +147,7 @@ ReturnImmediate(std::function<void(const struct mockfs_buf_in *in,
 }
 
 void sigint_handler(int __unused sig) {
-	quit = 1;
+	// Don't do anything except interrupt the daemon's read(2) call
 }
 
 void debug_fuseop(const mockfs_buf_in *in)
@@ -280,6 +280,7 @@ void debug_fuseop(const mockfs_buf_in *in)
 MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions,
 	bool push_symlinks_in, bool ro, uint32_t flags)
 {
+	struct sigaction sa;
 	struct iovec *iov = NULL;
 	int iovlen = 0;
 	char fdstr[15];
@@ -340,7 +341,12 @@ MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions,
 		.WillByDefault(Invoke(this, &MockFS::process_default));
 
 	init(flags);
-	signal(SIGUSR1, sigint_handler);
+	bzero(&sa, sizeof(sa));
+	sa.sa_handler = sigint_handler;
+	sa.sa_flags = 0;	/* Don't set SA_RESTART! */
+	if (0 != sigaction(SIGUSR1, &sa, NULL))
+		throw(std::system_error(errno, std::system_category(),
+			"Couldn't handle SIGUSR1"));
 	if (pthread_create(&m_daemon_id, NULL, service, (void*)this))
 		throw(std::system_error(errno, std::system_category(),
 			"Couldn't Couldn't start fuse thread"));
@@ -348,11 +354,11 @@ MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions,
 
 MockFS::~MockFS() {
 	kill_daemon();
-	::unmount("mountpoint", MNT_FORCE);
 	if (m_daemon_id != NULL) {
 		pthread_join(m_daemon_id, NULL);
 		m_daemon_id = NULL;
 	}
+	::unmount("mountpoint", MNT_FORCE);
 	rmdir("mountpoint");
 }
 
@@ -393,13 +399,14 @@ void MockFS::init(uint32_t flags) {
 }
 
 void MockFS::kill_daemon() {
-	if (m_daemon_id != NULL) {
+	quit = 1;
+	if (m_daemon_id != NULL)
 		pthread_kill(m_daemon_id, SIGUSR1);
-		// Closing the /dev/fuse file descriptor first allows unmount
-		// to succeed even if the daemon doesn't correctly respond to
-		// commands during the unmount sequence.
-		close(m_fuse_fd);
-	}
+	// Closing the /dev/fuse file descriptor first allows unmount to
+	// succeed even if the daemon doesn't correctly respond to commands
+	// during the unmount sequence.
+	close(m_fuse_fd);
+	m_fuse_fd = -1;
 }
 
 void MockFS::loop() {
@@ -462,6 +469,9 @@ bool MockFS::pid_ok(pid_t pid) {
 void MockFS::process_default(const mockfs_buf_in *in,
 		std::vector<mockfs_buf_out*> &out)
 {
+	if (verbosity > 1)
+		printf("%-11s REJECTED (wrong pid %d)\n",
+			opcode2opname(in->header.opcode), in->header.pid);
 	auto out0 = new mockfs_buf_out;
 	out0->header.unique = in->header.unique;
 	out0->header.error = -EOPNOTSUPP;