[PATCH v4 0/2] fuse: add timeout option for requests

[PATCH v4 0/2] fuse: add timeout option for requests

[Joanne Koong]

There are situations where fuse servers can become unresponsive or take
too long to reply to a request. Currently there is no upper bound on
how long a request may take, which may be frustrating to users who get
stuck waiting for a request to complete.

This patchset adds a timeout option for requests and two dynamically
configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
for controlling/enforcing timeout behavior system-wide.

Existing fuse servers will not be affected unless they explicitly opt into the
timeout.

v3: https://lore.kernel.org/linux-fsdevel/20240808190110.3188039-1-joannelkoong@gmail.com/
Changes from v3 -> v4:
- Fix wording on some comments to make it more clear
- Use simpler logic for timer (eg remove extra if checks, use mod timer API) (Josef)
- Sanity-check should be on FR_FINISHING not FR_FINISHED (Jingbo)
- Fix comment for "processing queue", add req->fpq = NULL safeguard  (Bernd)

v2: https://lore.kernel.org/linux-fsdevel/20240730002348.3431931-1-joannelkoong@gmail.com/
Changes from v2 -> v3:
- Disarm / rearm timer in dev_do_read to handle race conditions (Bernrd)
- Disarm timer in error handling for fatal interrupt (Yafang)
- Clean up do_fuse_request_end (Jingbo)
- Add timer for notify retrieve requests 
- Fix kernel test robot errors for #define no-op functions

v1: https://lore.kernel.org/linux-fsdevel/20240717213458.1613347-1-joannelkoong@gmail.com/
Changes from v1 -> v2:
- Add timeout for background requests
- Handle resend race condition
- Add sysctls

Joanne Koong (2):
  fuse: add optional kernel-enforced timeout for requests
  fuse: add default_request_timeout and max_request_timeout sysctls

 Documentation/admin-guide/sysctl/fs.rst |  17 +++
 fs/fuse/Makefile                        |   2 +-
 fs/fuse/dev.c                           | 192 +++++++++++++++++++++++-
 fs/fuse/fuse_i.h                        |  30 ++++
 fs/fuse/inode.c                         |  24 +++
 fs/fuse/sysctl.c                        |  42 ++++++
 6 files changed, 298 insertions(+), 9 deletions(-)
 create mode 100644 fs/fuse/sysctl.c

-- 
2.43.5

[PATCH v4 1/2] fuse: add optional kernel-enforced timeout for requests

[Joanne Koong]

There are situations where fuse servers can become unresponsive or take
too long to reply to a request. Currently there is no upper bound on
how long a request may take, which may be frustrating to users who get
stuck waiting for a request to complete.

This commit adds a timeout option (in seconds) for requests. If the
timeout elapses before the server replies to the request, the request
will fail with -ETIME.

There are 3 possibilities for a request that times out:
a) The request times out before the request has been sent to userspace
b) The request times out after the request has been sent to userspace
and before it receives a reply from the server
c) The request times out after the request has been sent to userspace
and the server replies while the kernel is timing out the request

While a request timeout is being handled, there may be other handlers
running at the same time if:
a) the kernel is forwarding the request to the server
b) the kernel is processing the server's reply to the request
c) the request is being re-sent
d) the connection is aborting
e) the device is getting released

Proper synchronization must be added to ensure that the request is
handled correctly in all of these cases. To this effect, there is a new
FR_FINISHING bit added to the request flags, which is set atomically by
either the timeout handler (see fuse_request_timeout()) which is invoked
after the request timeout elapses or set by the request reply handler
(see dev_do_write()), whichever gets there first. If the reply handler
and the timeout handler are executing simultaneously and the reply handler
sets FR_FINISHING before the timeout handler, then the request will be
handled as if the timeout did not elapse. If the timeout handler sets
FR_FINISHING before the reply handler, then the request will fail with
-ETIME and the request will be cleaned up.

Currently, this is the refcount lifecycle of a request:

Synchronous request is created:
fuse_simple_request -> allocates request, sets refcount to 1
  __fuse_request_send -> acquires refcount
    queues request and waits for reply...
fuse_simple_request -> drops refcount

Background request is created:
fuse_simple_background -> allocates request, sets refcount to 1

Request is replied to:
fuse_dev_do_write
  fuse_request_end -> drops refcount on request

Proper acquires on the request reference must be added to ensure that the
timeout handler does not drop the last refcount on the request while
other handlers may be operating on the request. Please note that the
timeout handler may get invoked at any phase of the request's
lifetime (eg before the request has been forwarded to userspace, etc).

It is always guaranteed that there is a refcount on the request when the
timeout handler is executing. The timeout handler will be either
deactivated by the reply/abort/release handlers, or if the timeout
handler is concurrently executing on another CPU, the reply/abort/release
handlers will wait for the timeout handler to finish executing first before
it drops the final refcount on the request.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev.c    | 192 +++++++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h |  14 ++++
 fs/fuse/inode.c  |   7 ++
 3 files changed, 205 insertions(+), 8 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 9eb191b5c4de..cdfbce07bbfa 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -31,6 +31,8 @@ MODULE_ALIAS("devname:fuse");
 
 static struct kmem_cache *fuse_req_cachep;
 
+static void fuse_request_timeout(struct timer_list *timer);
+
 static struct fuse_dev *fuse_get_dev(struct file *file)
 {
 	/*
@@ -48,6 +50,8 @@ static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
 	refcount_set(&req->count, 1);
 	__set_bit(FR_PENDING, &req->flags);
 	req->fm = fm;
+	if (fm->fc->req_timeout)
+		timer_setup(&req->timer, fuse_request_timeout, 0);
 }
 
 static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags)
@@ -277,7 +281,7 @@ static void flush_bg_queue(struct fuse_conn *fc)
  * the 'end' callback is called if given, else the reference to the
  * request is released
  */
-void fuse_request_end(struct fuse_req *req)
+static void do_fuse_request_end(struct fuse_req *req)
 {
 	struct fuse_mount *fm = req->fm;
 	struct fuse_conn *fc = fm->fc;
@@ -296,8 +300,6 @@ void fuse_request_end(struct fuse_req *req)
 		list_del_init(&req->intr_entry);
 		spin_unlock(&fiq->lock);
 	}
-	WARN_ON(test_bit(FR_PENDING, &req->flags));
-	WARN_ON(test_bit(FR_SENT, &req->flags));
 	if (test_bit(FR_BACKGROUND, &req->flags)) {
 		spin_lock(&fc->bg_lock);
 		clear_bit(FR_BACKGROUND, &req->flags);
@@ -329,8 +331,104 @@ void fuse_request_end(struct fuse_req *req)
 put_request:
 	fuse_put_request(req);
 }
+
+void fuse_request_end(struct fuse_req *req)
+{
+	WARN_ON(test_bit(FR_PENDING, &req->flags));
+	WARN_ON(test_bit(FR_SENT, &req->flags));
+
+	if (req->timer.function)
+		timer_delete_sync(&req->timer);
+
+	do_fuse_request_end(req);
+}
 EXPORT_SYMBOL_GPL(fuse_request_end);
 
+static void timeout_inflight_req(struct fuse_req *req)
+{
+	struct fuse_conn *fc = req->fm->fc;
+	struct fuse_iqueue *fiq = &fc->iq;
+	struct fuse_pqueue *fpq;
+
+	spin_lock(&fiq->lock);
+	fpq = req->fpq;
+	spin_unlock(&fiq->lock);
+
+	/*
+	 * If fpq has not been set yet, then the request is aborting (which
+	 * clears FR_PENDING flag) before dev_do_read (which sets req->fpq)
+	 * has been called. Let the abort handler handle this request.
+	 */
+	if (!fpq)
+		return;
+
+	spin_lock(&fpq->lock);
+	if (!fpq->connected || req->out.h.error == -ECONNABORTED) {
+		/*
+		 * Connection is being aborted or the fuse_dev is being released.
+		 * The abort / release will clean up the request
+		 */
+		spin_unlock(&fpq->lock);
+		return;
+	}
+
+	if (!test_bit(FR_PRIVATE, &req->flags))
+		list_del_init(&req->list);
+
+	spin_unlock(&fpq->lock);
+
+	req->out.h.error = -ETIME;
+
+	do_fuse_request_end(req);
+}
+
+static void timeout_pending_req(struct fuse_req *req)
+{
+	struct fuse_conn *fc = req->fm->fc;
+	struct fuse_iqueue *fiq = &fc->iq;
+	bool background = test_bit(FR_BACKGROUND, &req->flags);
+
+	if (background)
+		spin_lock(&fc->bg_lock);
+	spin_lock(&fiq->lock);
+
+	if (!test_bit(FR_PENDING, &req->flags)) {
+		spin_unlock(&fiq->lock);
+		if (background)
+			spin_unlock(&fc->bg_lock);
+		timeout_inflight_req(req);
+		return;
+	}
+
+	if (!test_bit(FR_PRIVATE, &req->flags))
+		list_del_init(&req->list);
+
+	spin_unlock(&fiq->lock);
+	if (background)
+		spin_unlock(&fc->bg_lock);
+
+	req->out.h.error = -ETIME;
+
+	do_fuse_request_end(req);
+}
+
+static void fuse_request_timeout(struct timer_list *timer)
+{
+	struct fuse_req *req = container_of(timer, struct fuse_req, timer);
+
+	/*
+	 * Request reply is being finished by the kernel right now.
+	 * No need to time out the request.
+	 */
+	if (test_and_set_bit(FR_FINISHING, &req->flags))
+		return;
+
+	if (test_bit(FR_PENDING, &req->flags))
+		timeout_pending_req(req);
+	else
+		timeout_inflight_req(req);
+}
+
 static int queue_interrupt(struct fuse_req *req)
 {
 	struct fuse_iqueue *fiq = &req->fm->fc->iq;
@@ -393,6 +491,8 @@ static void request_wait_answer(struct fuse_req *req)
 		if (test_bit(FR_PENDING, &req->flags)) {
 			list_del(&req->list);
 			spin_unlock(&fiq->lock);
+			if (req->timer.function && !timer_delete_sync(&req->timer))
+				return;
 			__fuse_put_request(req);
 			req->out.h.error = -EINTR;
 			return;
@@ -409,7 +509,8 @@ static void request_wait_answer(struct fuse_req *req)
 
 static void __fuse_request_send(struct fuse_req *req)
 {
-	struct fuse_iqueue *fiq = &req->fm->fc->iq;
+	struct fuse_conn *fc = req->fm->fc;
+	struct fuse_iqueue *fiq = &fc->iq;
 
 	BUG_ON(test_bit(FR_BACKGROUND, &req->flags));
 	spin_lock(&fiq->lock);
@@ -421,6 +522,8 @@ static void __fuse_request_send(struct fuse_req *req)
 		/* acquire extra reference, since request is still needed
 		   after fuse_request_end() */
 		__fuse_get_request(req);
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fc->req_timeout);
 		queue_request_and_unlock(fiq, req);
 
 		request_wait_answer(req);
@@ -539,6 +642,8 @@ static bool fuse_request_queue_background(struct fuse_req *req)
 		if (fc->num_background == fc->max_background)
 			fc->blocked = 1;
 		list_add_tail(&req->list, &fc->bg_queue);
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fc->req_timeout);
 		flush_bg_queue(fc);
 		queued = true;
 	}
@@ -594,6 +699,8 @@ static int fuse_simple_notify_reply(struct fuse_mount *fm,
 
 	spin_lock(&fiq->lock);
 	if (fiq->connected) {
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fm->fc->req_timeout);
 		queue_request_and_unlock(fiq, req);
 	} else {
 		err = -ENODEV;
@@ -1268,8 +1375,26 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	req = list_entry(fiq->pending.next, struct fuse_req, list);
 	clear_bit(FR_PENDING, &req->flags);
 	list_del_init(&req->list);
+	/* Acquire a reference in case the timeout handler starts executing */
+	__fuse_get_request(req);
+	req->fpq = fpq;
 	spin_unlock(&fiq->lock);
 
+	if (req->timer.function) {
+		/*
+		 * Temporarily disable the timer on the request to avoid race
+		 * conditions between this code and the timeout handler.
+		 *
+		 * The timer is readded at the end of this function.
+		 */
+		if (!timer_delete_sync(&req->timer)) {
+			/* sanity check that the timer handler did run */
+			WARN_ON(!test_bit(FR_FINISHING, &req->flags));
+			fuse_put_request(req);
+			goto restart;
+		}
+	}
+
 	args = req->args;
 	reqsize = req->in.h.len;
 
@@ -1280,6 +1405,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 		if (args->opcode == FUSE_SETXATTR)
 			req->out.h.error = -E2BIG;
 		fuse_request_end(req);
+		fuse_put_request(req);
 		goto restart;
 	}
 	spin_lock(&fpq->lock);
@@ -1316,8 +1442,12 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	}
 	hash = fuse_req_hash(req->in.h.unique);
 	list_move_tail(&req->list, &fpq->processing[hash]);
-	__fuse_get_request(req);
 	set_bit(FR_SENT, &req->flags);
+
+	/* re-arm the original timer */
+	if (req->timer.function)
+		add_timer(&req->timer);
+
 	spin_unlock(&fpq->lock);
 	/* matches barrier in request_wait_answer() */
 	smp_mb__after_atomic();
@@ -1332,6 +1462,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 		list_del_init(&req->list);
 	spin_unlock(&fpq->lock);
 	fuse_request_end(req);
+	fuse_put_request(req);
 	return err;
 
  err_unlock:
@@ -1806,8 +1937,25 @@ static void fuse_resend(struct fuse_conn *fc)
 		struct fuse_pqueue *fpq = &fud->pq;
 
 		spin_lock(&fpq->lock);
-		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
+		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
+			list_for_each_entry(req, &fpq->processing[i], list) {
+				/*
+				 * We must acquire a reference here in case the timeout
+				 * handler is running at the same time. Else the
+				 * request might get freed out from under us
+				 */
+				__fuse_get_request(req);
+
+				/*
+				 * While we have an acquired reference on the request,
+				 * the request must remain on the list so that we
+				 * can release the reference on it
+				 */
+				set_bit(FR_PRIVATE, &req->flags);
+			}
+
 			list_splice_tail_init(&fpq->processing[i], &to_queue);
+		}
 		spin_unlock(&fpq->lock);
 	}
 	spin_unlock(&fc->lock);
@@ -1820,6 +1968,12 @@ static void fuse_resend(struct fuse_conn *fc)
 	}
 
 	spin_lock(&fiq->lock);
+	list_for_each_entry_safe(req, next, &to_queue, list) {
+		if (test_bit(FR_FINISHING, &req->flags))
+			list_del_init(&req->list);
+		clear_bit(FR_PRIVATE, &req->flags);
+		fuse_put_request(req);
+	}
 	/* iq and pq requests are both oldest to newest */
 	list_splice(&to_queue, &fiq->pending);
 	fiq->ops->wake_pending_and_unlock(fiq);
@@ -1951,9 +2105,10 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 		goto copy_finish;
 	}
 
+	__fuse_get_request(req);
+
 	/* Is it an interrupt reply ID? */
 	if (oh.unique & FUSE_INT_REQ_BIT) {
-		__fuse_get_request(req);
 		spin_unlock(&fpq->lock);
 
 		err = 0;
@@ -1969,6 +2124,18 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 		goto copy_finish;
 	}
 
+	if (test_and_set_bit(FR_FINISHING, &req->flags)) {
+		/* timeout handler is already finishing the request */
+		spin_unlock(&fpq->lock);
+		fuse_put_request(req);
+		goto copy_finish;
+	}
+
+	/*
+	 * FR_FINISHING ensures the timeout handler will be a no-op if it runs,
+	 * but unset req->fpq here as an extra safeguard
+	 */
+	req->fpq = NULL;
 	clear_bit(FR_SENT, &req->flags);
 	list_move(&req->list, &fpq->io);
 	req->out.h = oh;
@@ -1995,6 +2162,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 	spin_unlock(&fpq->lock);
 
 	fuse_request_end(req);
+	fuse_put_request(req);
 out:
 	return err ? err : nbytes;
 
@@ -2260,13 +2428,21 @@ int fuse_dev_release(struct inode *inode, struct file *file)
 	if (fud) {
 		struct fuse_conn *fc = fud->fc;
 		struct fuse_pqueue *fpq = &fud->pq;
+		struct fuse_req *req;
 		LIST_HEAD(to_end);
 		unsigned int i;
 
 		spin_lock(&fpq->lock);
 		WARN_ON(!list_empty(&fpq->io));
-		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
+		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
+			/*
+			 * Set the req error here so that the timeout
+			 * handler knows it's being released
+			 */
+			list_for_each_entry(req, &fpq->processing[i], list)
+				req->out.h.error = -ECONNABORTED;
 			list_splice_init(&fpq->processing[i], &to_end);
+		}
 		spin_unlock(&fpq->lock);
 
 		end_requests(&to_end);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f23919610313..0a2fa487a3bf 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -375,6 +375,8 @@ struct fuse_io_priv {
  * FR_FINISHED:		request is finished
  * FR_PRIVATE:		request is on private list
  * FR_ASYNC:		request is asynchronous
+ * FR_FINISHING:	request is being finished, by either the timeout handler
+ *			or the reply handler
  */
 enum fuse_req_flag {
 	FR_ISREPLY,
@@ -389,6 +391,7 @@ enum fuse_req_flag {
 	FR_FINISHED,
 	FR_PRIVATE,
 	FR_ASYNC,
+	FR_FINISHING,
 };
 
 /**
@@ -435,6 +438,12 @@ struct fuse_req {
 
 	/** fuse_mount this request belongs to */
 	struct fuse_mount *fm;
+
+	/** processing queue for the fuse_dev this request belongs to */
+	struct fuse_pqueue *fpq;
+
+	/** optional timer for request replies, if timeout is enabled */
+	struct timer_list timer;
 };
 
 struct fuse_iqueue;
@@ -574,6 +583,8 @@ struct fuse_fs_context {
 	enum fuse_dax_mode dax_mode;
 	unsigned int max_read;
 	unsigned int blksize;
+	/*  Request timeout (in seconds). 0 = no timeout (infinite wait) */
+	unsigned int req_timeout;
 	const char *subtype;
 
 	/* DAX device, may be NULL */
@@ -633,6 +644,9 @@ struct fuse_conn {
 	/** Constrain ->max_pages to this value during feature negotiation */
 	unsigned int max_pages_limit;
 
+	/* Request timeout (in jiffies). 0 = no timeout (infinite wait) */
+	unsigned long req_timeout;
+
 	/** Input queue */
 	struct fuse_iqueue iq;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 99e44ea7d875..9e69006fc026 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -733,6 +733,7 @@ enum {
 	OPT_ALLOW_OTHER,
 	OPT_MAX_READ,
 	OPT_BLKSIZE,
+	OPT_REQUEST_TIMEOUT,
 	OPT_ERR
 };
 
@@ -747,6 +748,7 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
 	fsparam_u32	("max_read",		OPT_MAX_READ),
 	fsparam_u32	("blksize",		OPT_BLKSIZE),
 	fsparam_string	("subtype",		OPT_SUBTYPE),
+	fsparam_u32	("request_timeout",	OPT_REQUEST_TIMEOUT),
 	{}
 };
 
@@ -830,6 +832,10 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
 		ctx->blksize = result.uint_32;
 		break;
 
+	case OPT_REQUEST_TIMEOUT:
+		ctx->req_timeout = result.uint_32;
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -1724,6 +1730,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->group_id = ctx->group_id;
 	fc->legacy_opts_show = ctx->legacy_opts_show;
 	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
+	fc->req_timeout = ctx->req_timeout * HZ;
 	fc->destroy = ctx->destroy;
 	fc->no_control = ctx->no_control;
 	fc->no_force_umount = ctx->no_force_umount;
-- 
2.43.5

[PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

Introduce two new sysctls, "default_request_timeout" and
"max_request_timeout". These control timeouts on replies by the
server to kernel-issued fuse requests.

"default_request_timeout" sets a timeout if no timeout is specified by
the fuse server on mount. 0 (default) indicates no timeout should be enforced.

"max_request_timeout" sets a maximum timeout for fuse requests. If the
fuse server attempts to set a timeout greater than max_request_timeout,
the system will default to max_request_timeout. Similarly, if the max
default timeout is greater than the max request timeout, the system will
default to the max request timeout. 0 (default) indicates no timeout should
be enforced.

$ sysctl -a | grep fuse
fs.fuse.default_request_timeout = 0
fs.fuse.max_request_timeout = 0

$ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument

$ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
0xFFFFFFFF

$ sysctl -a | grep fuse
fs.fuse.default_request_timeout = 4294967295
fs.fuse.max_request_timeout = 0

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Bernd Schubert <bschubert@ddn.com>
---
 Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
 fs/fuse/Makefile                        |  2 +-
 fs/fuse/fuse_i.h                        | 16 ++++++++++
 fs/fuse/inode.c                         | 19 ++++++++++-
 fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
 5 files changed, 94 insertions(+), 2 deletions(-)
 create mode 100644 fs/fuse/sysctl.c

diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
index 47499a1742bd..44fd495f69b4 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
 on a 64-bit one.
 The current default value for ``max_user_watches`` is 4% of the
 available low memory, divided by the "watch" cost in bytes.
+
+5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
+=====================================================================
+
+This directory contains the following configuration options for FUSE
+filesystems:
+
+``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
+setting/getting the default timeout (in seconds) for a fuse server to
+reply to a kernel-issued request in the event where the server did not
+specify a timeout at mount. 0 indicates no timeout.
+
+``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
+setting/getting the maximum timeout (in seconds) for a fuse server to
+reply to a kernel-issued request. If the server attempts to set a
+timeout greater than max_request_timeout, the system will use
+max_request_timeout as the timeout. 0 indicates no timeout.
diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
index 6e0228c6d0cb..cd4ef3e08ebf 100644
--- a/fs/fuse/Makefile
+++ b/fs/fuse/Makefile
@@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
 obj-$(CONFIG_CUSE) += cuse.o
 obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
 
-fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
+fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
 fuse-y += iomode.o
 fuse-$(CONFIG_FUSE_DAX) += dax.o
 fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 0a2fa487a3bf..dae9977fa050 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -47,6 +47,14 @@
 /** Number of dentries for each connection in the control filesystem */
 #define FUSE_CTL_NUM_DENTRIES 5
 
+/*
+ * Default timeout (in seconds) for the server to reply to a request
+ * if no timeout was specified on mount
+ */
+extern u32 fuse_default_req_timeout;
+/** Max timeout (in seconds) for the server to reply to a request */
+extern u32 fuse_max_req_timeout;
+
 /** List of active connections */
 extern struct list_head fuse_conn_list;
 
@@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
 				      size_t len, unsigned int flags);
 ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
 
+#ifdef CONFIG_SYSCTL
+int fuse_sysctl_register(void);
+void fuse_sysctl_unregister(void);
+#else
+static inline int fuse_sysctl_register(void) { return 0; }
+static inline void fuse_sysctl_unregister(void) { return; }
+#endif /* CONFIG_SYSCTL */
+
 #endif /* _FS_FUSE_I_H */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 9e69006fc026..cf333448f2d3 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
 
 static int set_global_limit(const char *val, const struct kernel_param *kp);
 
+/* default is no timeout */
+u32 fuse_default_req_timeout = 0;
+u32 fuse_max_req_timeout = 0;
+
 unsigned max_user_bgreq;
 module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
 		  &max_user_bgreq, 0644);
@@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	struct fuse_conn *fc = fm->fc;
 	struct inode *root;
 	struct dentry *root_dentry;
+	u32 req_timeout;
 	int err;
 
 	err = -EINVAL;
@@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->group_id = ctx->group_id;
 	fc->legacy_opts_show = ctx->legacy_opts_show;
 	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
-	fc->req_timeout = ctx->req_timeout * HZ;
 	fc->destroy = ctx->destroy;
 	fc->no_control = ctx->no_control;
 	fc->no_force_umount = ctx->no_force_umount;
+	req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
+	if (!fuse_max_req_timeout)
+		fc->req_timeout = req_timeout * HZ;
+	else if (!req_timeout)
+		fc->req_timeout = fuse_max_req_timeout * HZ;
+	else
+		fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
 
 	err = -ENOMEM;
 	root = fuse_get_root_inode(sb, ctx->rootmode);
@@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
 	if (err)
 		goto out3;
 
+	err = fuse_sysctl_register();
+	if (err)
+		goto out4;
+
 	return 0;
 
+ out4:
+	unregister_filesystem(&fuse_fs_type);
  out3:
 	unregister_fuseblk();
  out2:
diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
new file mode 100644
index 000000000000..c87bb0ecbfa9
--- /dev/null
+++ b/fs/fuse/sysctl.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+* linux/fs/fuse/fuse_sysctl.c
+*
+* Sysctl interface to fuse parameters
+*/
+#include <linux/sysctl.h>
+
+#include "fuse_i.h"
+
+static struct ctl_table_header *fuse_table_header;
+
+static struct ctl_table fuse_sysctl_table[] = {
+	{
+		.procname	= "default_request_timeout",
+		.data		= &fuse_default_req_timeout,
+		.maxlen		= sizeof(fuse_default_req_timeout),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+	{
+		.procname	= "max_request_timeout",
+		.data		= &fuse_max_req_timeout,
+		.maxlen		= sizeof(fuse_max_req_timeout),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+};
+
+int fuse_sysctl_register(void)
+{
+	fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
+	if (!fuse_table_header)
+		return -ENOMEM;
+	return 0;
+}
+
+void fuse_sysctl_unregister(void)
+{
+	unregister_sysctl_table(fuse_table_header);
+	fuse_table_header = NULL;
+}
-- 
2.43.5

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Yafang Shao]

On Wed, Aug 14, 2024 at 7:24 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> Introduce two new sysctls, "default_request_timeout" and
> "max_request_timeout". These control timeouts on replies by the
> server to kernel-issued fuse requests.
>
> "default_request_timeout" sets a timeout if no timeout is specified by
> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
>
> "max_request_timeout" sets a maximum timeout for fuse requests. If the
> fuse server attempts to set a timeout greater than max_request_timeout,
> the system will default to max_request_timeout. Similarly, if the max
> default timeout is greater than the max request timeout, the system will
> default to the max request timeout. 0 (default) indicates no timeout should
> be enforced.
>
> $ sysctl -a | grep fuse
> fs.fuse.default_request_timeout = 0
> fs.fuse.max_request_timeout = 0
>
> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
>
> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> 0xFFFFFFFF
>
> $ sysctl -a | grep fuse
> fs.fuse.default_request_timeout = 4294967295
> fs.fuse.max_request_timeout = 0
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
>  fs/fuse/Makefile                        |  2 +-
>  fs/fuse/fuse_i.h                        | 16 ++++++++++
>  fs/fuse/inode.c                         | 19 ++++++++++-
>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
>  5 files changed, 94 insertions(+), 2 deletions(-)
>  create mode 100644 fs/fuse/sysctl.c
>
> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> index 47499a1742bd..44fd495f69b4 100644
> --- a/Documentation/admin-guide/sysctl/fs.rst
> +++ b/Documentation/admin-guide/sysctl/fs.rst
> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
>  on a 64-bit one.
>  The current default value for ``max_user_watches`` is 4% of the
>  available low memory, divided by the "watch" cost in bytes.
> +
> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> +=====================================================================
> +
> +This directory contains the following configuration options for FUSE
> +filesystems:
> +
> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> +setting/getting the default timeout (in seconds) for a fuse server to
> +reply to a kernel-issued request in the event where the server did not
> +specify a timeout at mount. 0 indicates no timeout.

While testing on my servers, I observed that the timeout value appears
to be doubled. For instance, if I set the timeout to 10 seconds, the
"Timer expired" message occurs after 20 seconds.

Is this an expected behavior, or is the doubling unavoidable? I'm okay
with it as long as we have a functioning timeout. However, I recommend
documenting this behavior to avoid any potential confusion for users.

--
Regards
Yafang

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

On Mon, Aug 19, 2024 at 11:40 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, Aug 14, 2024 at 7:24 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > Introduce two new sysctls, "default_request_timeout" and
> > "max_request_timeout". These control timeouts on replies by the
> > server to kernel-issued fuse requests.
> >
> > "default_request_timeout" sets a timeout if no timeout is specified by
> > the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> >
> > "max_request_timeout" sets a maximum timeout for fuse requests. If the
> > fuse server attempts to set a timeout greater than max_request_timeout,
> > the system will default to max_request_timeout. Similarly, if the max
> > default timeout is greater than the max request timeout, the system will
> > default to the max request timeout. 0 (default) indicates no timeout should
> > be enforced.
> >
> > $ sysctl -a | grep fuse
> > fs.fuse.default_request_timeout = 0
> > fs.fuse.max_request_timeout = 0
> >
> > $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> >
> > $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > 0xFFFFFFFF
> >
> > $ sysctl -a | grep fuse
> > fs.fuse.default_request_timeout = 4294967295
> > fs.fuse.max_request_timeout = 0
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> > ---
> >  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
> >  fs/fuse/Makefile                        |  2 +-
> >  fs/fuse/fuse_i.h                        | 16 ++++++++++
> >  fs/fuse/inode.c                         | 19 ++++++++++-
> >  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
> >  5 files changed, 94 insertions(+), 2 deletions(-)
> >  create mode 100644 fs/fuse/sysctl.c
> >
> > diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> > index 47499a1742bd..44fd495f69b4 100644
> > --- a/Documentation/admin-guide/sysctl/fs.rst
> > +++ b/Documentation/admin-guide/sysctl/fs.rst
> > @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
> >  on a 64-bit one.
> >  The current default value for ``max_user_watches`` is 4% of the
> >  available low memory, divided by the "watch" cost in bytes.
> > +
> > +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> > +=====================================================================
> > +
> > +This directory contains the following configuration options for FUSE
> > +filesystems:
> > +
> > +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> > +setting/getting the default timeout (in seconds) for a fuse server to
> > +reply to a kernel-issued request in the event where the server did not
> > +specify a timeout at mount. 0 indicates no timeout.
>
> While testing on my servers, I observed that the timeout value appears
> to be doubled. For instance, if I set the timeout to 10 seconds, the
> "Timer expired" message occurs after 20 seconds.
>
> Is this an expected behavior, or is the doubling unavoidable? I'm okay
> with it as long as we have a functioning timeout. However, I recommend
> documenting this behavior to avoid any potential confusion for users.

Hi Yafang,

Are you testing this by running "cat hello" from the libfuse hello
example server and seeing the doubled timeout?

This is happening because cat hello sends two FUSE_READ requests. The
first FUSE_READ is a background request (called from
fuse_readahead()). I confirmed that this takes 10 seconds to time out
and then the timeout handler kicks in. Then the second FUSE_READ is a
regular request (called from fuse_read_folio() -> fuse_do_readpage()).
This second request also takes 10 seconds to time out. After this
second request times out is when the "cat hello" returns, which is why
the overall time is 20 seconds because of the 2 requests each taking
10 seconds.

Thanks,
Joanne

>
> --
> Regards
> Yafang

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Yafang Shao]

On Wed, Aug 21, 2024 at 2:31 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Aug 19, 2024 at 11:40 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Wed, Aug 14, 2024 at 7:24 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > Introduce two new sysctls, "default_request_timeout" and
> > > "max_request_timeout". These control timeouts on replies by the
> > > server to kernel-issued fuse requests.
> > >
> > > "default_request_timeout" sets a timeout if no timeout is specified by
> > > the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> > >
> > > "max_request_timeout" sets a maximum timeout for fuse requests. If the
> > > fuse server attempts to set a timeout greater than max_request_timeout,
> > > the system will default to max_request_timeout. Similarly, if the max
> > > default timeout is greater than the max request timeout, the system will
> > > default to the max request timeout. 0 (default) indicates no timeout should
> > > be enforced.
> > >
> > > $ sysctl -a | grep fuse
> > > fs.fuse.default_request_timeout = 0
> > > fs.fuse.max_request_timeout = 0
> > >
> > > $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > > tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> > >
> > > $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > > 0xFFFFFFFF
> > >
> > > $ sysctl -a | grep fuse
> > > fs.fuse.default_request_timeout = 4294967295
> > > fs.fuse.max_request_timeout = 0
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > > Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> > > ---
> > >  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
> > >  fs/fuse/Makefile                        |  2 +-
> > >  fs/fuse/fuse_i.h                        | 16 ++++++++++
> > >  fs/fuse/inode.c                         | 19 ++++++++++-
> > >  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
> > >  5 files changed, 94 insertions(+), 2 deletions(-)
> > >  create mode 100644 fs/fuse/sysctl.c
> > >
> > > diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> > > index 47499a1742bd..44fd495f69b4 100644
> > > --- a/Documentation/admin-guide/sysctl/fs.rst
> > > +++ b/Documentation/admin-guide/sysctl/fs.rst
> > > @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
> > >  on a 64-bit one.
> > >  The current default value for ``max_user_watches`` is 4% of the
> > >  available low memory, divided by the "watch" cost in bytes.
> > > +
> > > +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> > > +=====================================================================
> > > +
> > > +This directory contains the following configuration options for FUSE
> > > +filesystems:
> > > +
> > > +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> > > +setting/getting the default timeout (in seconds) for a fuse server to
> > > +reply to a kernel-issued request in the event where the server did not
> > > +specify a timeout at mount. 0 indicates no timeout.
> >
> > While testing on my servers, I observed that the timeout value appears
> > to be doubled. For instance, if I set the timeout to 10 seconds, the
> > "Timer expired" message occurs after 20 seconds.
> >
> > Is this an expected behavior, or is the doubling unavoidable? I'm okay
> > with it as long as we have a functioning timeout. However, I recommend
> > documenting this behavior to avoid any potential confusion for users.
>
> Hi Yafang,
>
> Are you testing this by running "cat hello" from the libfuse hello
> example server and seeing the doubled timeout?

right.

>
> This is happening because cat hello sends two FUSE_READ requests. The
> first FUSE_READ is a background request (called from
> fuse_readahead()). I confirmed that this takes 10 seconds to time out
> and then the timeout handler kicks in. Then the second FUSE_READ is a
> regular request (called from fuse_read_folio() -> fuse_do_readpage()).
> This second request also takes 10 seconds to time out. After this
> second request times out is when the "cat hello" returns, which is why
> the overall time is 20 seconds because of the 2 requests each taking
> 10 seconds.
>

Thanks for your detailed explanation.

-- 
Regards
Yafang

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Yafang Shao]

On Wed, Aug 14, 2024 at 7:23 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> There are situations where fuse servers can become unresponsive or take
> too long to reply to a request. Currently there is no upper bound on
> how long a request may take, which may be frustrating to users who get
> stuck waiting for a request to complete.
>
> This patchset adds a timeout option for requests and two dynamically
> configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
> for controlling/enforcing timeout behavior system-wide.
>
> Existing fuse servers will not be affected unless they explicitly opt into the
> timeout.
>
> v3: https://lore.kernel.org/linux-fsdevel/20240808190110.3188039-1-joannelkoong@gmail.com/
> Changes from v3 -> v4:
> - Fix wording on some comments to make it more clear
> - Use simpler logic for timer (eg remove extra if checks, use mod timer API) (Josef)
> - Sanity-check should be on FR_FINISHING not FR_FINISHED (Jingbo)
> - Fix comment for "processing queue", add req->fpq = NULL safeguard  (Bernd)
>
> v2: https://lore.kernel.org/linux-fsdevel/20240730002348.3431931-1-joannelkoong@gmail.com/
> Changes from v2 -> v3:
> - Disarm / rearm timer in dev_do_read to handle race conditions (Bernrd)
> - Disarm timer in error handling for fatal interrupt (Yafang)
> - Clean up do_fuse_request_end (Jingbo)
> - Add timer for notify retrieve requests
> - Fix kernel test robot errors for #define no-op functions
>
> v1: https://lore.kernel.org/linux-fsdevel/20240717213458.1613347-1-joannelkoong@gmail.com/
> Changes from v1 -> v2:
> - Add timeout for background requests
> - Handle resend race condition
> - Add sysctls
>
> Joanne Koong (2):
>   fuse: add optional kernel-enforced timeout for requests
>   fuse: add default_request_timeout and max_request_timeout sysctls
>
>  Documentation/admin-guide/sysctl/fs.rst |  17 +++
>  fs/fuse/Makefile                        |   2 +-
>  fs/fuse/dev.c                           | 192 +++++++++++++++++++++++-
>  fs/fuse/fuse_i.h                        |  30 ++++
>  fs/fuse/inode.c                         |  24 +++
>  fs/fuse/sysctl.c                        |  42 ++++++
>  6 files changed, 298 insertions(+), 9 deletions(-)
>  create mode 100644 fs/fuse/sysctl.c
>
> --
> 2.43.5
>

For this series,

Tested-by: Yafang Shao <laoar.shao@gmail.com>

-- 
Regards
Yafang

Re: [PATCH v4 1/2] fuse: add optional kernel-enforced timeout for requests

[Jingbo Xu]

Hi, Joanne,

On 8/14/24 7:22 AM, Joanne Koong wrote:
> There are situations where fuse servers can become unresponsive or take
> too long to reply to a request. Currently there is no upper bound on
> how long a request may take, which may be frustrating to users who get
> stuck waiting for a request to complete.
> 
> This commit adds a timeout option (in seconds) for requests. If the
> timeout elapses before the server replies to the request, the request
> will fail with -ETIME.
> 
> There are 3 possibilities for a request that times out:
> a) The request times out before the request has been sent to userspace
> b) The request times out after the request has been sent to userspace
> and before it receives a reply from the server
> c) The request times out after the request has been sent to userspace
> and the server replies while the kernel is timing out the request
> 
> While a request timeout is being handled, there may be other handlers
> running at the same time if:
> a) the kernel is forwarding the request to the server
> b) the kernel is processing the server's reply to the request
> c) the request is being re-sent
> d) the connection is aborting
> e) the device is getting released
> 
> Proper synchronization must be added to ensure that the request is
> handled correctly in all of these cases. To this effect, there is a new
> FR_FINISHING bit added to the request flags, which is set atomically by
> either the timeout handler (see fuse_request_timeout()) which is invoked
> after the request timeout elapses or set by the request reply handler
> (see dev_do_write()), whichever gets there first. If the reply handler
> and the timeout handler are executing simultaneously and the reply handler
> sets FR_FINISHING before the timeout handler, then the request will be
> handled as if the timeout did not elapse. If the timeout handler sets
> FR_FINISHING before the reply handler, then the request will fail with
> -ETIME and the request will be cleaned up.
> 
> Currently, this is the refcount lifecycle of a request:
> 
> Synchronous request is created:
> fuse_simple_request -> allocates request, sets refcount to 1
>   __fuse_request_send -> acquires refcount
>     queues request and waits for reply...
> fuse_simple_request -> drops refcount
> 
> Background request is created:
> fuse_simple_background -> allocates request, sets refcount to 1
> 
> Request is replied to:
> fuse_dev_do_write
>   fuse_request_end -> drops refcount on request
> 
> Proper acquires on the request reference must be added to ensure that the
> timeout handler does not drop the last refcount on the request while
> other handlers may be operating on the request. Please note that the
> timeout handler may get invoked at any phase of the request's
> lifetime (eg before the request has been forwarded to userspace, etc).
> 
> It is always guaranteed that there is a refcount on the request when the
> timeout handler is executing. The timeout handler will be either
> deactivated by the reply/abort/release handlers, or if the timeout
> handler is concurrently executing on another CPU, the reply/abort/release
> handlers will wait for the timeout handler to finish executing first before
> it drops the final refcount on the request.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev.c    | 192 +++++++++++++++++++++++++++++++++++++++++++++--
>  fs/fuse/fuse_i.h |  14 ++++
>  fs/fuse/inode.c  |   7 ++
>  3 files changed, 205 insertions(+), 8 deletions(-)

> @@ -1951,9 +2105,10 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
>  		goto copy_finish;
>  	}
>  
> +	__fuse_get_request(req);
> +

While re-inspecting the patch, I doubt if acquiring an extra req->count
here is really needed here.

There are three conditions for concurrency between reply receiving and
timeout handler:

1. timeout handler acquires fpq->lock first and delets the request from
processing[] table.  In this case, fuse_dev_write() has no chance of
accessing this request since it has previously already been removed from
the processing[] table.  No concurrency and no extra refcount needed here.

2. fuse_dev_write() acquires fpq->lock first and sets FR_FINISHING.  In
this case the timeout handler will be disactivated when seeing
FR_FINISHING.  Also No concurrency and no extra refcount needed here.

2. fuse_dev_write() acquires fpq->lock first but timeout handler sets
FR_FINISHING first.  In this case, fuse_dev_write() handler will return,
leaving the request to the timeout hadler. The access to fuse_req from
fuse_dev_write() is safe as long as fuse_dev_write() still holds
fpq->lock, as the timeout handler may free the request only after
acquiring and releasing fpq->lock.  Besides, as for fuse_dev_write(),
the only operation after releasing fpq->lock is fuse_copy_finish(cs),
which shall be safe even when the fuse_req may have been freed by the
timeout handler (not seriously confirmed though)?

Please correct me if I missed something.

>  	/* Is it an interrupt reply ID? */
>  	if (oh.unique & FUSE_INT_REQ_BIT) {
> -		__fuse_get_request(req);
>  		spin_unlock(&fpq->lock);
>  
>  		err = 0;
> @@ -1969,6 +2124,18 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
>  		goto copy_finish;
>  	}
>  
> +	if (test_and_set_bit(FR_FINISHING, &req->flags)) {
> +		/* timeout handler is already finishing the request */
> +		spin_unlock(&fpq->lock);
> +		fuse_put_request(req);
> +		goto copy_finish;
> +	}
> +
> +	/*
> +	 * FR_FINISHING ensures the timeout handler will be a no-op if it runs,
> +	 * but unset req->fpq here as an extra safeguard
> +	 */
> +	req->fpq = NULL;
>  	clear_bit(FR_SENT, &req->flags);
>  	list_move(&req->list, &fpq->io);
>  	req->out.h = oh;
> @@ -1995,6 +2162,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
>  	spin_unlock(&fpq->lock);
>  
>  	fuse_request_end(req);
> +	fuse_put_request(req);
>  out:
>  	return err ? err : nbytes;
>  


-- 
Thanks,
Jingbo

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Wed, 14 Aug 2024 at 01:23, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> There are situations where fuse servers can become unresponsive or take
> too long to reply to a request. Currently there is no upper bound on
> how long a request may take, which may be frustrating to users who get
> stuck waiting for a request to complete.
>
> This patchset adds a timeout option for requests and two dynamically
> configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
> for controlling/enforcing timeout behavior system-wide.
>
> Existing fuse servers will not be affected unless they explicitly opt into the
> timeout.

I sort of understand the motivation, but do not clearly see why this
is required.

A well written server will be able to do request timeouts properly,
without the kernel having to cut off requests mid flight without the
knowledge of the server.  The latter could even be dangerous because
locking guarantees previously provided by the kernel do not apply
anymore.

Can you please explain why this needs to be done by the client
(kernel) instead of the server (userspace)?

Thanks,
Miklos




>
> v3: https://lore.kernel.org/linux-fsdevel/20240808190110.3188039-1-joannelkoong@gmail.com/
> Changes from v3 -> v4:
> - Fix wording on some comments to make it more clear
> - Use simpler logic for timer (eg remove extra if checks, use mod timer API) (Josef)
> - Sanity-check should be on FR_FINISHING not FR_FINISHED (Jingbo)
> - Fix comment for "processing queue", add req->fpq = NULL safeguard  (Bernd)
>
> v2: https://lore.kernel.org/linux-fsdevel/20240730002348.3431931-1-joannelkoong@gmail.com/
> Changes from v2 -> v3:
> - Disarm / rearm timer in dev_do_read to handle race conditions (Bernrd)
> - Disarm timer in error handling for fatal interrupt (Yafang)
> - Clean up do_fuse_request_end (Jingbo)
> - Add timer for notify retrieve requests
> - Fix kernel test robot errors for #define no-op functions
>
> v1: https://lore.kernel.org/linux-fsdevel/20240717213458.1613347-1-joannelkoong@gmail.com/
> Changes from v1 -> v2:
> - Add timeout for background requests
> - Handle resend race condition
> - Add sysctls
>
> Joanne Koong (2):
>   fuse: add optional kernel-enforced timeout for requests
>   fuse: add default_request_timeout and max_request_timeout sysctls
>
>  Documentation/admin-guide/sysctl/fs.rst |  17 +++
>  fs/fuse/Makefile                        |   2 +-
>  fs/fuse/dev.c                           | 192 +++++++++++++++++++++++-
>  fs/fuse/fuse_i.h                        |  30 ++++
>  fs/fuse/inode.c                         |  24 +++
>  fs/fuse/sysctl.c                        |  42 ++++++
>  6 files changed, 298 insertions(+), 9 deletions(-)
>  create mode 100644 fs/fuse/sysctl.c
>
> --
> 2.43.5
>

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Bernd Schubert]

On 8/21/24 15:47, Miklos Szeredi wrote:
> On Wed, 14 Aug 2024 at 01:23, Joanne Koong <joannelkoong@gmail.com> wrote:
>>
>> There are situations where fuse servers can become unresponsive or take
>> too long to reply to a request. Currently there is no upper bound on
>> how long a request may take, which may be frustrating to users who get
>> stuck waiting for a request to complete.
>>
>> This patchset adds a timeout option for requests and two dynamically
>> configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
>> for controlling/enforcing timeout behavior system-wide.
>>
>> Existing fuse servers will not be affected unless they explicitly opt into the
>> timeout.
> 
> I sort of understand the motivation, but do not clearly see why this
> is required.
> 
> A well written server will be able to do request timeouts properly,
> without the kernel having to cut off requests mid flight without the
> knowledge of the server.  The latter could even be dangerous because
> locking guarantees previously provided by the kernel do not apply
> anymore.
> 
> Can you please explain why this needs to be done by the client
> (kernel) instead of the server (userspace)?

I don't know about Joannes motivation, I see two use cases

- You suggested yourself that timeouts might be a (non-ideal) 
solution to avoid page copies on writeback
https://lore.kernel.org/linux-mm/CAJfpegt_mEYOeeTo2bWS3iJfC38t5bf29mzrxK68dhMptrgamg@mail.gmail.com/raw

- Would probably be a solution for this LXCFS issue
https://lore.kernel.org/lkml/a8d0c5da-6935-4d28-9380-68b84b8e6e72@shopee.com/
(although I don't fully understand the issue there yet)


Thanks,
Bernd

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Wed, 21 Aug 2024 at 16:15, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:

> - You suggested yourself that timeouts might be a (non-ideal)
> solution to avoid page copies on writeback
> https://lore.kernel.org/linux-mm/CAJfpegt_mEYOeeTo2bWS3iJfC38t5bf29mzrxK68dhMptrgamg@mail.gmail.com/raw

What I suggested is to copy the original writeback page to a tmp page
after a timeout.  That's not the same as aborting the request, though
the mechanism might be similar.

> - Would probably be a solution for this LXCFS issue
> https://lore.kernel.org/lkml/a8d0c5da-6935-4d28-9380-68b84b8e6e72@shopee.com/
> (although I don't fully understand the issue there yet)

Yeah, we first need to understand what's going on.

Thanks,
Miklos

Re: [PATCH v4 1/2] fuse: add optional kernel-enforced timeout for requests

[Joanne Koong]

On Wed, Aug 21, 2024 at 12:56 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> Hi, Joanne,
>
> On 8/14/24 7:22 AM, Joanne Koong wrote:
> > There are situations where fuse servers can become unresponsive or take
> > too long to reply to a request. Currently there is no upper bound on
> > how long a request may take, which may be frustrating to users who get
> > stuck waiting for a request to complete.
> >
> > This commit adds a timeout option (in seconds) for requests. If the
> > timeout elapses before the server replies to the request, the request
> > will fail with -ETIME.
> >
> > There are 3 possibilities for a request that times out:
> > a) The request times out before the request has been sent to userspace
> > b) The request times out after the request has been sent to userspace
> > and before it receives a reply from the server
> > c) The request times out after the request has been sent to userspace
> > and the server replies while the kernel is timing out the request
> >
> > While a request timeout is being handled, there may be other handlers
> > running at the same time if:
> > a) the kernel is forwarding the request to the server
> > b) the kernel is processing the server's reply to the request
> > c) the request is being re-sent
> > d) the connection is aborting
> > e) the device is getting released
> >
> > Proper synchronization must be added to ensure that the request is
> > handled correctly in all of these cases. To this effect, there is a new
> > FR_FINISHING bit added to the request flags, which is set atomically by
> > either the timeout handler (see fuse_request_timeout()) which is invoked
> > after the request timeout elapses or set by the request reply handler
> > (see dev_do_write()), whichever gets there first. If the reply handler
> > and the timeout handler are executing simultaneously and the reply handler
> > sets FR_FINISHING before the timeout handler, then the request will be
> > handled as if the timeout did not elapse. If the timeout handler sets
> > FR_FINISHING before the reply handler, then the request will fail with
> > -ETIME and the request will be cleaned up.
> >
> > Currently, this is the refcount lifecycle of a request:
> >
> > Synchronous request is created:
> > fuse_simple_request -> allocates request, sets refcount to 1
> >   __fuse_request_send -> acquires refcount
> >     queues request and waits for reply...
> > fuse_simple_request -> drops refcount
> >
> > Background request is created:
> > fuse_simple_background -> allocates request, sets refcount to 1
> >
> > Request is replied to:
> > fuse_dev_do_write
> >   fuse_request_end -> drops refcount on request
> >
> > Proper acquires on the request reference must be added to ensure that the
> > timeout handler does not drop the last refcount on the request while
> > other handlers may be operating on the request. Please note that the
> > timeout handler may get invoked at any phase of the request's
> > lifetime (eg before the request has been forwarded to userspace, etc).
> >
> > It is always guaranteed that there is a refcount on the request when the
> > timeout handler is executing. The timeout handler will be either
> > deactivated by the reply/abort/release handlers, or if the timeout
> > handler is concurrently executing on another CPU, the reply/abort/release
> > handlers will wait for the timeout handler to finish executing first before
> > it drops the final refcount on the request.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/dev.c    | 192 +++++++++++++++++++++++++++++++++++++++++++++--
> >  fs/fuse/fuse_i.h |  14 ++++
> >  fs/fuse/inode.c  |   7 ++
> >  3 files changed, 205 insertions(+), 8 deletions(-)
>
> > @@ -1951,9 +2105,10 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> >               goto copy_finish;
> >       }
> >
> > +     __fuse_get_request(req);
> > +
>
> While re-inspecting the patch, I doubt if acquiring an extra req->count
> here is really needed here.
>
> There are three conditions for concurrency between reply receiving and
> timeout handler:
>
> 1. timeout handler acquires fpq->lock first and delets the request from
> processing[] table.  In this case, fuse_dev_write() has no chance of
> accessing this request since it has previously already been removed from
> the processing[] table.  No concurrency and no extra refcount needed here.
>
> 2. fuse_dev_write() acquires fpq->lock first and sets FR_FINISHING.  In
> this case the timeout handler will be disactivated when seeing
> FR_FINISHING.  Also No concurrency and no extra refcount needed here.
>
> 2. fuse_dev_write() acquires fpq->lock first but timeout handler sets
> FR_FINISHING first.  In this case, fuse_dev_write() handler will return,
> leaving the request to the timeout hadler. The access to fuse_req from
> fuse_dev_write() is safe as long as fuse_dev_write() still holds
> fpq->lock, as the timeout handler may free the request only after
> acquiring and releasing fpq->lock.  Besides, as for fuse_dev_write(),
> the only operation after releasing fpq->lock is fuse_copy_finish(cs),
> which shall be safe even when the fuse_req may have been freed by the
> timeout handler (not seriously confirmed though)?
>
> Please correct me if I missed something.

Hi Jingbo,

Thanks for taking the time to reinspect this patch. I agree with your
analysis above. That all sounds right to me.
I don't remember why I added this line in v1. I think I had missed
that the fpq lock would prevent the request from being completely
freed out by the timeout handler so no extra refcount is necessary.
I'll remove this for v5.


Thanks,
Joanne

>
> >       /* Is it an interrupt reply ID? */
> >       if (oh.unique & FUSE_INT_REQ_BIT) {
> > -             __fuse_get_request(req);
> >               spin_unlock(&fpq->lock);
> >
> >               err = 0;
> > @@ -1969,6 +2124,18 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> >               goto copy_finish;
> >       }
> >
> > +     if (test_and_set_bit(FR_FINISHING, &req->flags)) {
> > +             /* timeout handler is already finishing the request */
> > +             spin_unlock(&fpq->lock);
> > +             fuse_put_request(req);
> > +             goto copy_finish;
> > +     }
> > +
> > +     /*
> > +      * FR_FINISHING ensures the timeout handler will be a no-op if it runs,
> > +      * but unset req->fpq here as an extra safeguard
> > +      */
> > +     req->fpq = NULL;
> >       clear_bit(FR_SENT, &req->flags);
> >       list_move(&req->list, &fpq->io);
> >       req->out.h = oh;
> > @@ -1995,6 +2162,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> >       spin_unlock(&fpq->lock);
> >
> >       fuse_request_end(req);
> > +     fuse_put_request(req);
> >  out:
> >       return err ? err : nbytes;
> >
>
>
> --
> Thanks,
> Jingbo

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Josef Bacik]

On Wed, Aug 21, 2024 at 03:47:53PM +0200, Miklos Szeredi wrote:
> On Wed, 14 Aug 2024 at 01:23, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > There are situations where fuse servers can become unresponsive or take
> > too long to reply to a request. Currently there is no upper bound on
> > how long a request may take, which may be frustrating to users who get
> > stuck waiting for a request to complete.
> >
> > This patchset adds a timeout option for requests and two dynamically
> > configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
> > for controlling/enforcing timeout behavior system-wide.
> >
> > Existing fuse servers will not be affected unless they explicitly opt into the
> > timeout.
> 
> I sort of understand the motivation, but do not clearly see why this
> is required.
> 
> A well written server will be able to do request timeouts properly,
> without the kernel having to cut off requests mid flight without the
> knowledge of the server.  The latter could even be dangerous because
> locking guarantees previously provided by the kernel do not apply
> anymore.
> 
> Can you please explain why this needs to be done by the client
> (kernel) instead of the server (userspace)?
> 

"A well written server" is the key part here ;).  In our case we had a "well
written server" that ended up having a deadlock and we had to run around with a
drgn script to find those hung mounts and kill them manually.  The usecase here
is specifically for bugs in the FUSE server to allow us to cleanup automatically
with EIO's rather than a drgn script to figure out if the mount is hung.

It also gives us the opportunity to do the things that Bernd points out,
specifically remove the double buffering downside as we can trust that
eventually writeback will either succeed or timeout.  Thanks,

Josef

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Wed, 21 Aug 2024 at 20:11, Josef Bacik <josef@toxicpanda.com> wrote:

> "A well written server" is the key part here ;).  In our case we had a "well
> written server" that ended up having a deadlock and we had to run around with a
> drgn script to find those hung mounts and kill them manually.  The usecase here
> is specifically for bugs in the FUSE server to allow us to cleanup automatically
> with EIO's rather than a drgn script to figure out if the mount is hung.

So you 'd like to automatically abort the connection to an
unresponsive server?  I'm okay with that.

What I'm worried about is the unintended side effects of timed out
request without the server's knowledge (i.e. VFS locks released, then
new request takes VFS lock).   If the connection to the server is
aborted, then that's not an issue.

It's also much simpler to just time out any response from the server
(either read or write on /dev/fuse) than having to do per-request
timeouts.

> It also gives us the opportunity to do the things that Bernd points out,
> specifically remove the double buffering downside as we can trust that
> eventually writeback will either succeed or timeout.  Thanks,

Well see this explanation for how this might deadlock on a memory
allocation by the server:

 https://lore.kernel.org/all/CAJfpegsfF77SV96wvaxn9VnRkNt5FKCnA4mJ0ieFsZtwFeRuYw@mail.gmail.com/

Having a timeout would fix the deadlock, but it doesn't seem to me a
proper solution.

Thanks,
Miklos

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Joanne Koong]

On Wed, Aug 21, 2024 at 11:54 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 21 Aug 2024 at 20:11, Josef Bacik <josef@toxicpanda.com> wrote:
>
> > "A well written server" is the key part here ;).  In our case we had a "well
> > written server" that ended up having a deadlock and we had to run around with a
> > drgn script to find those hung mounts and kill them manually.  The usecase here
> > is specifically for bugs in the FUSE server to allow us to cleanup automatically
> > with EIO's rather than a drgn script to figure out if the mount is hung.
>
> So you 'd like to automatically abort the connection to an
> unresponsive server?  I'm okay with that.
>
> What I'm worried about is the unintended side effects of timed out
> request without the server's knowledge (i.e. VFS locks released, then
> new request takes VFS lock).   If the connection to the server is
> aborted, then that's not an issue.
>
> It's also much simpler to just time out any response from the server
> (either read or write on /dev/fuse) than having to do per-request
> timeouts.

In our case, the deadlock was triggered by invalidating the inode in
the middle of handling the write request. The server becomes stuck
since the inode invalidation (eg fuse_reverse_inval_inode())  is
attempting to acquire the folio lock but the lock was acquired when
servicing the write request (eg fuse_fill_write_pages()) and only gets
released after the server has replied to the write request (eg in
fuse_send_write_pages()).

Without a kernel enforced timeout, the only way out of this is to
abort the connection. A userspace timeout wouldn't help in this case
with getting the server unstuck. With the kernel timeout, this forces
the kernel handling of the write request to proceed, whihc will drop
the folio lock and resume the server back to a functioning state.

I don't think situations like this are uncommon. For example, it's not
obvious or clear to developers that fuse_lowlevel_notify_inval_inode()
shouldn't be called inside of a write handler in their server code.

I believe Yafang had a use case for this as well in
https://lore.kernel.org/linux-fsdevel/20240724071156.97188-1-laoar.shao@gmail.com/
where they were seeing fuse connections becoming indefinitely stuck.

For your concern about potential unintended side effects of timed out
requests without the server's knowledge, could you elaborate more on
the VFS locking example? In my mind, a request that times out is the
same thing as a request that behaves normally and completes with an
error code, but perhaps not?

I think also, having some way for system admins to enforce request
timeouts across the board might be useful as well - for example, if a
malignant fuse server doesn't reply to any requests, the requests hog
memory until the server is killed.

Thanks,
Joanne

>
> > It also gives us the opportunity to do the things that Bernd points out,
> > specifically remove the double buffering downside as we can trust that
> > eventually writeback will either succeed or timeout.  Thanks,
>
> Well see this explanation for how this might deadlock on a memory
> allocation by the server:
>
>  https://lore.kernel.org/all/CAJfpegsfF77SV96wvaxn9VnRkNt5FKCnA4mJ0ieFsZtwFeRuYw@mail.gmail.com/
>
> Having a timeout would fix the deadlock, but it doesn't seem to me a
> proper solution.
>
> Thanks,
> Miklos

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Jingbo Xu]

On 8/14/24 7:22 AM, Joanne Koong wrote:
> Introduce two new sysctls, "default_request_timeout" and
> "max_request_timeout". These control timeouts on replies by the
> server to kernel-issued fuse requests.
> 
> "default_request_timeout" sets a timeout if no timeout is specified by
> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> 
> "max_request_timeout" sets a maximum timeout for fuse requests. If the
> fuse server attempts to set a timeout greater than max_request_timeout,
> the system will default to max_request_timeout. Similarly, if the max
> default timeout is greater than the max request timeout, the system will
> default to the max request timeout. 0 (default) indicates no timeout should
> be enforced.
> 
> $ sysctl -a | grep fuse
> fs.fuse.default_request_timeout = 0
> fs.fuse.max_request_timeout = 0
> 
> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> 
> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> 0xFFFFFFFF
> 
> $ sysctl -a | grep fuse
> fs.fuse.default_request_timeout = 4294967295
> fs.fuse.max_request_timeout = 0
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
>  fs/fuse/Makefile                        |  2 +-
>  fs/fuse/fuse_i.h                        | 16 ++++++++++
>  fs/fuse/inode.c                         | 19 ++++++++++-
>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
>  5 files changed, 94 insertions(+), 2 deletions(-)
>  create mode 100644 fs/fuse/sysctl.c
> 
> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> index 47499a1742bd..44fd495f69b4 100644
> --- a/Documentation/admin-guide/sysctl/fs.rst
> +++ b/Documentation/admin-guide/sysctl/fs.rst
> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
>  on a 64-bit one.
>  The current default value for ``max_user_watches`` is 4% of the
>  available low memory, divided by the "watch" cost in bytes.
> +
> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> +=====================================================================
> +
> +This directory contains the following configuration options for FUSE
> +filesystems:
> +
> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> +setting/getting the default timeout (in seconds) for a fuse server to
> +reply to a kernel-issued request in the event where the server did not
> +specify a timeout at mount. 0 indicates no timeout.
> +
> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
> +setting/getting the maximum timeout (in seconds) for a fuse server to
> +reply to a kernel-issued request. If the server attempts to set a
> +timeout greater than max_request_timeout, the system will use
> +max_request_timeout as the timeout. 0 indicates no timeout.

"0 indicates no timeout"

I think 0 max_request_timeout shall indicate that there's no explicit
maximum limitation for request_timeout.


> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> index 6e0228c6d0cb..cd4ef3e08ebf 100644
> --- a/fs/fuse/Makefile
> +++ b/fs/fuse/Makefile
> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
>  obj-$(CONFIG_CUSE) += cuse.o
>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
>  
> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
>  fuse-y += iomode.o
>  fuse-$(CONFIG_FUSE_DAX) += dax.o
>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 0a2fa487a3bf..dae9977fa050 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -47,6 +47,14 @@
>  /** Number of dentries for each connection in the control filesystem */
>  #define FUSE_CTL_NUM_DENTRIES 5
>  
> +/*
> + * Default timeout (in seconds) for the server to reply to a request
> + * if no timeout was specified on mount
> + */
> +extern u32 fuse_default_req_timeout;
> +/** Max timeout (in seconds) for the server to reply to a request */
> +extern u32 fuse_max_req_timeout;
> +
>  /** List of active connections */
>  extern struct list_head fuse_conn_list;
>  
> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>  				      size_t len, unsigned int flags);
>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
>  
> +#ifdef CONFIG_SYSCTL
> +int fuse_sysctl_register(void);
> +void fuse_sysctl_unregister(void);
> +#else
> +static inline int fuse_sysctl_register(void) { return 0; }
> +static inline void fuse_sysctl_unregister(void) { return; }
> +#endif /* CONFIG_SYSCTL */
> +
>  #endif /* _FS_FUSE_I_H */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 9e69006fc026..cf333448f2d3 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
>  
>  static int set_global_limit(const char *val, const struct kernel_param *kp);
>  
> +/* default is no timeout */
> +u32 fuse_default_req_timeout = 0;
> +u32 fuse_max_req_timeout = 0;
> +
>  unsigned max_user_bgreq;
>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
>  		  &max_user_bgreq, 0644);
> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	struct fuse_conn *fc = fm->fc;
>  	struct inode *root;
>  	struct dentry *root_dentry;
> +	u32 req_timeout;
>  	int err;
>  
>  	err = -EINVAL;
> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	fc->group_id = ctx->group_id;
>  	fc->legacy_opts_show = ctx->legacy_opts_show;
>  	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
> -	fc->req_timeout = ctx->req_timeout * HZ;
>  	fc->destroy = ctx->destroy;
>  	fc->no_control = ctx->no_control;
>  	fc->no_force_umount = ctx->no_force_umount;
> +	req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
> +	if (!fuse_max_req_timeout)
> +		fc->req_timeout = req_timeout * HZ;
> +	else if (!req_timeout)
> +		fc->req_timeout = fuse_max_req_timeout * HZ;

So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
0" mount option), the final request timeout is exactly
fuse_max_req_timeout, which is unexpected as I think 0
fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
timeout.

> +	else
> +		fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
>  
>  	err = -ENOMEM;
>  	root = fuse_get_root_inode(sb, ctx->rootmode);
> @@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
>  	if (err)
>  		goto out3;
>  
> +	err = fuse_sysctl_register();
> +	if (err)
> +		goto out4;
> +
>  	return 0;
>  
> + out4:
> +	unregister_filesystem(&fuse_fs_type);
>   out3:
>  	unregister_fuseblk();
>   out2:
> diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
> new file mode 100644
> index 000000000000..c87bb0ecbfa9
> --- /dev/null
> +++ b/fs/fuse/sysctl.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> +* linux/fs/fuse/fuse_sysctl.c
> +*
> +* Sysctl interface to fuse parameters
> +*/
> +#include <linux/sysctl.h>
> +
> +#include "fuse_i.h"
> +
> +static struct ctl_table_header *fuse_table_header;
> +
> +static struct ctl_table fuse_sysctl_table[] = {
> +	{
> +		.procname	= "default_request_timeout",
> +		.data		= &fuse_default_req_timeout,
> +		.maxlen		= sizeof(fuse_default_req_timeout),
> +		.mode		= 0644,
> +		.proc_handler	= proc_douintvec,
> +	},
> +	{
> +		.procname	= "max_request_timeout",
> +		.data		= &fuse_max_req_timeout,
> +		.maxlen		= sizeof(fuse_max_req_timeout),
> +		.mode		= 0644,
> +		.proc_handler	= proc_douintvec,
> +	},

Missing "{}" here?  The internal implementation of register_sysctl()
depends on an empty last element of the array as the sentinel.

> +};
> +
> +int fuse_sysctl_register(void)
> +{
> +	fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
> +	if (!fuse_table_header)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +void fuse_sysctl_unregister(void)
> +{
> +	unregister_sysctl_table(fuse_table_header);
> +	fuse_table_header = NULL;
> +}

-- 
Thanks,
Jingbo

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Wed, 21 Aug 2024 at 23:22, Joanne Koong <joannelkoong@gmail.com> wrote:

> Without a kernel enforced timeout, the only way out of this is to
> abort the connection. A userspace timeout wouldn't help in this case
> with getting the server unstuck. With the kernel timeout, this forces
> the kernel handling of the write request to proceed, whihc will drop
> the folio lock and resume the server back to a functioning state.
>
> I don't think situations like this are uncommon. For example, it's not
> obvious or clear to developers that fuse_lowlevel_notify_inval_inode()
> shouldn't be called inside of a write handler in their server code.

Documentation is definitely lacking.  In fact a simple rule is: never
call a notification function from within a request handling function.
Notifications are async events that should happen independently of
handling regular operations.  Anything else is an abuse of the
interface.

>
> For your concern about potential unintended side effects of timed out
> requests without the server's knowledge, could you elaborate more on
> the VFS locking example? In my mind, a request that times out is the
> same thing as a request that behaves normally and completes with an
> error code, but perhaps not?

- user calls mknod(2) on fuse directory
- VFS takes inode lock on parent directory
- calls into fuse to create the file
- fuse sends request to server
- file creation is slow and times out in the kernel
- fuse returns -ETIMEDOUT
- VFS releases inode lock
- meanwhile the server is still working on creating the file and has
no idea that something went wrong
- user calls the same mknod(2) again
- same things happen as last time
- server starts to create the file *again* knowing that the VFS takes
care of concurrency
- server crashes due to corruption


> I think also, having some way for system admins to enforce request
> timeouts across the board might be useful as well - for example, if a
> malignant fuse server doesn't reply to any requests, the requests hog
> memory until the server is killed.

As I said, I'm not against enforcing a response time for fuse servers,
as long as a timeout results in a complete abort and not just an error
on the timed out request.

Thanks,
Miklos

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Joanne Koong]

On Thu, Aug 22, 2024 at 3:52 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 21 Aug 2024 at 23:22, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> > Without a kernel enforced timeout, the only way out of this is to
> > abort the connection. A userspace timeout wouldn't help in this case
> > with getting the server unstuck. With the kernel timeout, this forces
> > the kernel handling of the write request to proceed, whihc will drop
> > the folio lock and resume the server back to a functioning state.
> >
> > I don't think situations like this are uncommon. For example, it's not
> > obvious or clear to developers that fuse_lowlevel_notify_inval_inode()
> > shouldn't be called inside of a write handler in their server code.
>
> Documentation is definitely lacking.  In fact a simple rule is: never
> call a notification function from within a request handling function.
> Notifications are async events that should happen independently of
> handling regular operations.  Anything else is an abuse of the
> interface.
>
> >
> > For your concern about potential unintended side effects of timed out
> > requests without the server's knowledge, could you elaborate more on
> > the VFS locking example? In my mind, a request that times out is the
> > same thing as a request that behaves normally and completes with an
> > error code, but perhaps not?
>
> - user calls mknod(2) on fuse directory
> - VFS takes inode lock on parent directory
> - calls into fuse to create the file
> - fuse sends request to server
> - file creation is slow and times out in the kernel
> - fuse returns -ETIMEDOUT
> - VFS releases inode lock
> - meanwhile the server is still working on creating the file and has
> no idea that something went wrong
> - user calls the same mknod(2) again
> - same things happen as last time
> - server starts to create the file *again* knowing that the VFS takes
> care of concurrency
> - server crashes due to corruption

Thanks for the details.

For cases like these though, isn't the server already responsible for
handling errors properly to avoid potential corruption if their reply
to the request fails? In your example above, it seems like the server
would already need to have the error handling in place to roll back
the file creation if their fuse_reply_create() call returned an error
(eg -EIO if copying out args in the kernel had an issue). If the
request timed out, then the server would get back -ENOENT to their
reply.


Thanks,
Joanne

>
>
> > I think also, having some way for system admins to enforce request
> > timeouts across the board might be useful as well - for example, if a
> > malignant fuse server doesn't reply to any requests, the requests hog
> > memory until the server is killed.
>
> As I said, I'm not against enforcing a response time for fuse servers,
> as long as a timeout results in a complete abort and not just an error
> on the timed out request.
>
> Thanks,
> Miklos

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 19:31, Joanne Koong <joannelkoong@gmail.com> wrote:

> For cases like these though, isn't the server already responsible for
> handling errors properly to avoid potential corruption if their reply
> to the request fails? In your example above, it seems like the server
> would already need to have the error handling in place to roll back
> the file creation if their fuse_reply_create() call returned an error
> (eg -EIO if copying out args in the kernel had an issue).

No, the server does not need to implement rollback, and does not in
fact need to check for the return value of the fuse_reply_create()
call unless it wants to mess with interrupts (not enabled by default).
See libfuse/lib/fuse.c where most of the fuse_replu_XXX() calls just
ignore the return value.

Thanks,
Miklos

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
>
>
> On 8/14/24 7:22 AM, Joanne Koong wrote:
> > Introduce two new sysctls, "default_request_timeout" and
> > "max_request_timeout". These control timeouts on replies by the
> > server to kernel-issued fuse requests.
> >
> > "default_request_timeout" sets a timeout if no timeout is specified by
> > the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> >
> > "max_request_timeout" sets a maximum timeout for fuse requests. If the
> > fuse server attempts to set a timeout greater than max_request_timeout,
> > the system will default to max_request_timeout. Similarly, if the max
> > default timeout is greater than the max request timeout, the system will
> > default to the max request timeout. 0 (default) indicates no timeout should
> > be enforced.
> >
> > $ sysctl -a | grep fuse
> > fs.fuse.default_request_timeout = 0
> > fs.fuse.max_request_timeout = 0
> >
> > $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> >
> > $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> > 0xFFFFFFFF
> >
> > $ sysctl -a | grep fuse
> > fs.fuse.default_request_timeout = 4294967295
> > fs.fuse.max_request_timeout = 0
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> > ---
> >  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
> >  fs/fuse/Makefile                        |  2 +-
> >  fs/fuse/fuse_i.h                        | 16 ++++++++++
> >  fs/fuse/inode.c                         | 19 ++++++++++-
> >  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
> >  5 files changed, 94 insertions(+), 2 deletions(-)
> >  create mode 100644 fs/fuse/sysctl.c
> >
> > diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> > index 47499a1742bd..44fd495f69b4 100644
> > --- a/Documentation/admin-guide/sysctl/fs.rst
> > +++ b/Documentation/admin-guide/sysctl/fs.rst
> > @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
> >  on a 64-bit one.
> >  The current default value for ``max_user_watches`` is 4% of the
> >  available low memory, divided by the "watch" cost in bytes.
> > +
> > +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> > +=====================================================================
> > +
> > +This directory contains the following configuration options for FUSE
> > +filesystems:
> > +
> > +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> > +setting/getting the default timeout (in seconds) for a fuse server to
> > +reply to a kernel-issued request in the event where the server did not
> > +specify a timeout at mount. 0 indicates no timeout.
> > +
> > +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
> > +setting/getting the maximum timeout (in seconds) for a fuse server to
> > +reply to a kernel-issued request. If the server attempts to set a
> > +timeout greater than max_request_timeout, the system will use
> > +max_request_timeout as the timeout. 0 indicates no timeout.
>
> "0 indicates no timeout"
>
> I think 0 max_request_timeout shall indicate that there's no explicit
> maximum limitation for request_timeout.

Hi Jingbo,

Ah I see where the confusion in the wording is (eg that "0 indicates
no timeout" could be interpreted to mean there is no timeout at all
for the connection, rather than no timeout as the max limit). Thanks
for pointing this out. I'll make this more explicit in v5. I'll change
the wording above for the "default_request_timeout" case too.

>
>
> > diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> > index 6e0228c6d0cb..cd4ef3e08ebf 100644
> > --- a/fs/fuse/Makefile
> > +++ b/fs/fuse/Makefile
> > @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
> >  obj-$(CONFIG_CUSE) += cuse.o
> >  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
> >
> > -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
> > +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
> >  fuse-y += iomode.o
> >  fuse-$(CONFIG_FUSE_DAX) += dax.o
> >  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 0a2fa487a3bf..dae9977fa050 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -47,6 +47,14 @@
> >  /** Number of dentries for each connection in the control filesystem */
> >  #define FUSE_CTL_NUM_DENTRIES 5
> >
> > +/*
> > + * Default timeout (in seconds) for the server to reply to a request
> > + * if no timeout was specified on mount
> > + */
> > +extern u32 fuse_default_req_timeout;
> > +/** Max timeout (in seconds) for the server to reply to a request */
> > +extern u32 fuse_max_req_timeout;
> > +
> >  /** List of active connections */
> >  extern struct list_head fuse_conn_list;
> >
> > @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
> >                                     size_t len, unsigned int flags);
> >  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
> >
> > +#ifdef CONFIG_SYSCTL
> > +int fuse_sysctl_register(void);
> > +void fuse_sysctl_unregister(void);
> > +#else
> > +static inline int fuse_sysctl_register(void) { return 0; }
> > +static inline void fuse_sysctl_unregister(void) { return; }
> > +#endif /* CONFIG_SYSCTL */
> > +
> >  #endif /* _FS_FUSE_I_H */
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 9e69006fc026..cf333448f2d3 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
> >
> >  static int set_global_limit(const char *val, const struct kernel_param *kp);
> >
> > +/* default is no timeout */
> > +u32 fuse_default_req_timeout = 0;
> > +u32 fuse_max_req_timeout = 0;
> > +
> >  unsigned max_user_bgreq;
> >  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
> >                 &max_user_bgreq, 0644);
> > @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >       struct fuse_conn *fc = fm->fc;
> >       struct inode *root;
> >       struct dentry *root_dentry;
> > +     u32 req_timeout;
> >       int err;
> >
> >       err = -EINVAL;
> > @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >       fc->group_id = ctx->group_id;
> >       fc->legacy_opts_show = ctx->legacy_opts_show;
> >       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
> > -     fc->req_timeout = ctx->req_timeout * HZ;
> >       fc->destroy = ctx->destroy;
> >       fc->no_control = ctx->no_control;
> >       fc->no_force_umount = ctx->no_force_umount;
> > +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
> > +     if (!fuse_max_req_timeout)
> > +             fc->req_timeout = req_timeout * HZ;
> > +     else if (!req_timeout)
> > +             fc->req_timeout = fuse_max_req_timeout * HZ;
>
> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
> 0" mount option), the final request timeout is exactly
> fuse_max_req_timeout, which is unexpected as I think 0
> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
> timeout.

fuse_max_req_timeout takes precedence over fuse_default_req_timeout
(eg if the system administrator wants to enforce a max limit on fuse
timeouts, that is imposed even if a specific fuse server didn't
indicate a timeout or indicated no timeout). Sorry, that wasn't made
clear in the documentation. I'll add that in for v5.

>
> > +     else
> > +             fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
> >
> >       err = -ENOMEM;
> >       root = fuse_get_root_inode(sb, ctx->rootmode);
> > @@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
> >       if (err)
> >               goto out3;
> >
> > +     err = fuse_sysctl_register();
> > +     if (err)
> > +             goto out4;
> > +
> >       return 0;
> >
> > + out4:
> > +     unregister_filesystem(&fuse_fs_type);
> >   out3:
> >       unregister_fuseblk();
> >   out2:
> > diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
> > new file mode 100644
> > index 000000000000..c87bb0ecbfa9
> > --- /dev/null
> > +++ b/fs/fuse/sysctl.c
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > +* linux/fs/fuse/fuse_sysctl.c
> > +*
> > +* Sysctl interface to fuse parameters
> > +*/
> > +#include <linux/sysctl.h>
> > +
> > +#include "fuse_i.h"
> > +
> > +static struct ctl_table_header *fuse_table_header;
> > +
> > +static struct ctl_table fuse_sysctl_table[] = {
> > +     {
> > +             .procname       = "default_request_timeout",
> > +             .data           = &fuse_default_req_timeout,
> > +             .maxlen         = sizeof(fuse_default_req_timeout),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_douintvec,
> > +     },
> > +     {
> > +             .procname       = "max_request_timeout",
> > +             .data           = &fuse_max_req_timeout,
> > +             .maxlen         = sizeof(fuse_max_req_timeout),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_douintvec,
> > +     },
>
> Missing "{}" here?  The internal implementation of register_sysctl()
> depends on an empty last element of the array as the sentinel.

Could you point me to where this is enforced? I admittedly copied this
formatting from how other "struct ctl_table"s are getting defined, and
I don't see them using an extra {} here as well? (eg pty_table,
inotify_table, epolll_table)


Thanks,
Joanne
>
> > +};
> > +
> > +int fuse_sysctl_register(void)
> > +{
> > +     fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
> > +     if (!fuse_table_header)
> > +             return -ENOMEM;
> > +     return 0;
> > +}
> > +
> > +void fuse_sysctl_unregister(void)
> > +{
> > +     unregister_sysctl_table(fuse_table_header);
> > +     fuse_table_header = NULL;
> > +}
>
> --
> Thanks,
> Jingbo

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Joanne Koong]

On Thu, Aug 22, 2024 at 10:43 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 22 Aug 2024 at 19:31, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> > For cases like these though, isn't the server already responsible for
> > handling errors properly to avoid potential corruption if their reply
> > to the request fails? In your example above, it seems like the server
> > would already need to have the error handling in place to roll back
> > the file creation if their fuse_reply_create() call returned an error
> > (eg -EIO if copying out args in the kernel had an issue).
>
> No, the server does not need to implement rollback, and does not in
> fact need to check for the return value of the fuse_reply_create()
> call unless it wants to mess with interrupts (not enabled by default).
> See libfuse/lib/fuse.c where most of the fuse_replu_XXX() calls just
> ignore the return value.

Ok, I see.

For v5, I will update this to abort the connection altogether if a
request times out.

Thanks,
Joanne
>
> Thanks,
> Miklos

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Jingbo Xu]

On 8/23/24 5:19 AM, Joanne Koong wrote:
> On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>>
>>
>> On 8/14/24 7:22 AM, Joanne Koong wrote:
>>> Introduce two new sysctls, "default_request_timeout" and
>>> "max_request_timeout". These control timeouts on replies by the
>>> server to kernel-issued fuse requests.
>>>
>>> "default_request_timeout" sets a timeout if no timeout is specified by
>>> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
>>>
>>> "max_request_timeout" sets a maximum timeout for fuse requests. If the
>>> fuse server attempts to set a timeout greater than max_request_timeout,
>>> the system will default to max_request_timeout. Similarly, if the max
>>> default timeout is greater than the max request timeout, the system will
>>> default to the max request timeout. 0 (default) indicates no timeout should
>>> be enforced.
>>>
>>> $ sysctl -a | grep fuse
>>> fs.fuse.default_request_timeout = 0
>>> fs.fuse.max_request_timeout = 0
>>>
>>> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
>>>
>>> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>> 0xFFFFFFFF
>>>
>>> $ sysctl -a | grep fuse
>>> fs.fuse.default_request_timeout = 4294967295
>>> fs.fuse.max_request_timeout = 0
>>>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
>>> ---
>>>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
>>>  fs/fuse/Makefile                        |  2 +-
>>>  fs/fuse/fuse_i.h                        | 16 ++++++++++
>>>  fs/fuse/inode.c                         | 19 ++++++++++-
>>>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
>>>  5 files changed, 94 insertions(+), 2 deletions(-)
>>>  create mode 100644 fs/fuse/sysctl.c
>>>
>>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
>>> index 47499a1742bd..44fd495f69b4 100644
>>> --- a/Documentation/admin-guide/sysctl/fs.rst
>>> +++ b/Documentation/admin-guide/sysctl/fs.rst
>>> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
>>>  on a 64-bit one.
>>>  The current default value for ``max_user_watches`` is 4% of the
>>>  available low memory, divided by the "watch" cost in bytes.
>>> +
>>> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
>>> +=====================================================================
>>> +
>>> +This directory contains the following configuration options for FUSE
>>> +filesystems:
>>> +
>>> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
>>> +setting/getting the default timeout (in seconds) for a fuse server to
>>> +reply to a kernel-issued request in the event where the server did not
>>> +specify a timeout at mount. 0 indicates no timeout.
>>> +
>>> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
>>> +setting/getting the maximum timeout (in seconds) for a fuse server to
>>> +reply to a kernel-issued request. If the server attempts to set a
>>> +timeout greater than max_request_timeout, the system will use
>>> +max_request_timeout as the timeout. 0 indicates no timeout.
>>
>> "0 indicates no timeout"
>>
>> I think 0 max_request_timeout shall indicate that there's no explicit
>> maximum limitation for request_timeout.
> 
> Hi Jingbo,
> 
> Ah I see where the confusion in the wording is (eg that "0 indicates
> no timeout" could be interpreted to mean there is no timeout at all
> for the connection, rather than no timeout as the max limit). Thanks
> for pointing this out. I'll make this more explicit in v5. I'll change
> the wording above for the "default_request_timeout" case too.
> 
>>
>>
>>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
>>> index 6e0228c6d0cb..cd4ef3e08ebf 100644
>>> --- a/fs/fuse/Makefile
>>> +++ b/fs/fuse/Makefile
>>> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
>>>  obj-$(CONFIG_CUSE) += cuse.o
>>>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
>>>
>>> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
>>> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
>>>  fuse-y += iomode.o
>>>  fuse-$(CONFIG_FUSE_DAX) += dax.o
>>>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 0a2fa487a3bf..dae9977fa050 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -47,6 +47,14 @@
>>>  /** Number of dentries for each connection in the control filesystem */
>>>  #define FUSE_CTL_NUM_DENTRIES 5
>>>
>>> +/*
>>> + * Default timeout (in seconds) for the server to reply to a request
>>> + * if no timeout was specified on mount
>>> + */
>>> +extern u32 fuse_default_req_timeout;
>>> +/** Max timeout (in seconds) for the server to reply to a request */
>>> +extern u32 fuse_max_req_timeout;
>>> +
>>>  /** List of active connections */
>>>  extern struct list_head fuse_conn_list;
>>>
>>> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>>>                                     size_t len, unsigned int flags);
>>>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
>>>
>>> +#ifdef CONFIG_SYSCTL
>>> +int fuse_sysctl_register(void);
>>> +void fuse_sysctl_unregister(void);
>>> +#else
>>> +static inline int fuse_sysctl_register(void) { return 0; }
>>> +static inline void fuse_sysctl_unregister(void) { return; }
>>> +#endif /* CONFIG_SYSCTL */
>>> +
>>>  #endif /* _FS_FUSE_I_H */
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 9e69006fc026..cf333448f2d3 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
>>>
>>>  static int set_global_limit(const char *val, const struct kernel_param *kp);
>>>
>>> +/* default is no timeout */
>>> +u32 fuse_default_req_timeout = 0;
>>> +u32 fuse_max_req_timeout = 0;
>>> +
>>>  unsigned max_user_bgreq;
>>>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
>>>                 &max_user_bgreq, 0644);
>>> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>       struct fuse_conn *fc = fm->fc;
>>>       struct inode *root;
>>>       struct dentry *root_dentry;
>>> +     u32 req_timeout;
>>>       int err;
>>>
>>>       err = -EINVAL;
>>> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>       fc->group_id = ctx->group_id;
>>>       fc->legacy_opts_show = ctx->legacy_opts_show;
>>>       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
>>> -     fc->req_timeout = ctx->req_timeout * HZ;
>>>       fc->destroy = ctx->destroy;
>>>       fc->no_control = ctx->no_control;
>>>       fc->no_force_umount = ctx->no_force_umount;
>>> +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
>>> +     if (!fuse_max_req_timeout)
>>> +             fc->req_timeout = req_timeout * HZ;
>>> +     else if (!req_timeout)
>>> +             fc->req_timeout = fuse_max_req_timeout * HZ;
>>
>> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
>> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
>> 0" mount option), the final request timeout is exactly
>> fuse_max_req_timeout, which is unexpected as I think 0
>> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
>> timeout.
> 
> fuse_max_req_timeout takes precedence over fuse_default_req_timeout
> (eg if the system administrator wants to enforce a max limit on fuse
> timeouts, that is imposed even if a specific fuse server didn't
> indicate a timeout or indicated no timeout). Sorry, that wasn't made
> clear in the documentation. I'll add that in for v5.

OK that is quite confusing.  If the system admin wants to enforce a
timeout, then a non-zero fuse_default_req_timeout is adequate.  What's
the case where fuse_default_req_timeout must be 0, and the aystem admin
has to impose the enforced timeout through fuse_max_req_timeout?

IMHO the semantics of fuse_max_req_timeout is not straightforward and
can be confusing if it implies an enforced timeout when no timeout is
specified, while at the same time it also imposes a maximum limitation
when timeout is specified.

> 
>>
>>> +     else
>>> +             fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
>>>
>>>       err = -ENOMEM;
>>>       root = fuse_get_root_inode(sb, ctx->rootmode);
>>> @@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
>>>       if (err)
>>>               goto out3;
>>>
>>> +     err = fuse_sysctl_register();
>>> +     if (err)
>>> +             goto out4;
>>> +
>>>       return 0;
>>>
>>> + out4:
>>> +     unregister_filesystem(&fuse_fs_type);
>>>   out3:
>>>       unregister_fuseblk();
>>>   out2:
>>> diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
>>> new file mode 100644
>>> index 000000000000..c87bb0ecbfa9
>>> --- /dev/null
>>> +++ b/fs/fuse/sysctl.c
>>> @@ -0,0 +1,42 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> +* linux/fs/fuse/fuse_sysctl.c
>>> +*
>>> +* Sysctl interface to fuse parameters
>>> +*/
>>> +#include <linux/sysctl.h>
>>> +
>>> +#include "fuse_i.h"
>>> +
>>> +static struct ctl_table_header *fuse_table_header;
>>> +
>>> +static struct ctl_table fuse_sysctl_table[] = {
>>> +     {
>>> +             .procname       = "default_request_timeout",
>>> +             .data           = &fuse_default_req_timeout,
>>> +             .maxlen         = sizeof(fuse_default_req_timeout),
>>> +             .mode           = 0644,
>>> +             .proc_handler   = proc_douintvec,
>>> +     },
>>> +     {
>>> +             .procname       = "max_request_timeout",
>>> +             .data           = &fuse_max_req_timeout,
>>> +             .maxlen         = sizeof(fuse_max_req_timeout),
>>> +             .mode           = 0644,
>>> +             .proc_handler   = proc_douintvec,
>>> +     },
>>
>> Missing "{}" here?  The internal implementation of register_sysctl()
>> depends on an empty last element of the array as the sentinel.
> 
> Could you point me to where this is enforced? I admittedly copied this
> formatting from how other "struct ctl_table"s are getting defined, and
> I don't see them using an extra {} here as well? (eg pty_table,
> inotify_table, epolll_table)

Alright.  When I pick this patch to an older kernel version. e.g. 5.10,
and use register_sysctl_table() API instead as register_sysctl() API is
not available yet at that time.  The register_sysctl_table() API depends
on an empty last element in the array as the sentinel as I said above,
while the new register_sysctl_sz() API, which is called by
register_sysctl(), actually takes the size of the ctl_table in to avoid
array out-of-bounds access.  So please ignore my noise.  Sorry for that.


-- 
Thanks,
Jingbo

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

On Thu, Aug 22, 2024 at 7:17 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> On 8/23/24 5:19 AM, Joanne Koong wrote:
> > On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> >>
> >>
> >>
> >> On 8/14/24 7:22 AM, Joanne Koong wrote:
> >>> Introduce two new sysctls, "default_request_timeout" and
> >>> "max_request_timeout". These control timeouts on replies by the
> >>> server to kernel-issued fuse requests.
> >>>
> >>> "default_request_timeout" sets a timeout if no timeout is specified by
> >>> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> >>>
> >>> "max_request_timeout" sets a maximum timeout for fuse requests. If the
> >>> fuse server attempts to set a timeout greater than max_request_timeout,
> >>> the system will default to max_request_timeout. Similarly, if the max
> >>> default timeout is greater than the max request timeout, the system will
> >>> default to the max request timeout. 0 (default) indicates no timeout should
> >>> be enforced.
> >>>
> >>> $ sysctl -a | grep fuse
> >>> fs.fuse.default_request_timeout = 0
> >>> fs.fuse.max_request_timeout = 0
> >>>
> >>> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> >>> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> >>>
> >>> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> >>> 0xFFFFFFFF
> >>>
> >>> $ sysctl -a | grep fuse
> >>> fs.fuse.default_request_timeout = 4294967295
> >>> fs.fuse.max_request_timeout = 0
> >>>
> >>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> >>> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> >>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> >>> ---
> >>>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
> >>>  fs/fuse/Makefile                        |  2 +-
> >>>  fs/fuse/fuse_i.h                        | 16 ++++++++++
> >>>  fs/fuse/inode.c                         | 19 ++++++++++-
> >>>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
> >>>  5 files changed, 94 insertions(+), 2 deletions(-)
> >>>  create mode 100644 fs/fuse/sysctl.c
> >>>
> >>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> >>> index 47499a1742bd..44fd495f69b4 100644
> >>> --- a/Documentation/admin-guide/sysctl/fs.rst
> >>> +++ b/Documentation/admin-guide/sysctl/fs.rst
> >>> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
> >>>  on a 64-bit one.
> >>>  The current default value for ``max_user_watches`` is 4% of the
> >>>  available low memory, divided by the "watch" cost in bytes.
> >>> +
> >>> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> >>> +=====================================================================
> >>> +
> >>> +This directory contains the following configuration options for FUSE
> >>> +filesystems:
> >>> +
> >>> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> >>> +setting/getting the default timeout (in seconds) for a fuse server to
> >>> +reply to a kernel-issued request in the event where the server did not
> >>> +specify a timeout at mount. 0 indicates no timeout.
> >>> +
> >>> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
> >>> +setting/getting the maximum timeout (in seconds) for a fuse server to
> >>> +reply to a kernel-issued request. If the server attempts to set a
> >>> +timeout greater than max_request_timeout, the system will use
> >>> +max_request_timeout as the timeout. 0 indicates no timeout.
> >>
> >> "0 indicates no timeout"
> >>
> >> I think 0 max_request_timeout shall indicate that there's no explicit
> >> maximum limitation for request_timeout.
> >
> > Hi Jingbo,
> >
> > Ah I see where the confusion in the wording is (eg that "0 indicates
> > no timeout" could be interpreted to mean there is no timeout at all
> > for the connection, rather than no timeout as the max limit). Thanks
> > for pointing this out. I'll make this more explicit in v5. I'll change
> > the wording above for the "default_request_timeout" case too.
> >
> >>
> >>
> >>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> >>> index 6e0228c6d0cb..cd4ef3e08ebf 100644
> >>> --- a/fs/fuse/Makefile
> >>> +++ b/fs/fuse/Makefile
> >>> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
> >>>  obj-$(CONFIG_CUSE) += cuse.o
> >>>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
> >>>
> >>> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
> >>> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
> >>>  fuse-y += iomode.o
> >>>  fuse-$(CONFIG_FUSE_DAX) += dax.o
> >>>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
> >>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> >>> index 0a2fa487a3bf..dae9977fa050 100644
> >>> --- a/fs/fuse/fuse_i.h
> >>> +++ b/fs/fuse/fuse_i.h
> >>> @@ -47,6 +47,14 @@
> >>>  /** Number of dentries for each connection in the control filesystem */
> >>>  #define FUSE_CTL_NUM_DENTRIES 5
> >>>
> >>> +/*
> >>> + * Default timeout (in seconds) for the server to reply to a request
> >>> + * if no timeout was specified on mount
> >>> + */
> >>> +extern u32 fuse_default_req_timeout;
> >>> +/** Max timeout (in seconds) for the server to reply to a request */
> >>> +extern u32 fuse_max_req_timeout;
> >>> +
> >>>  /** List of active connections */
> >>>  extern struct list_head fuse_conn_list;
> >>>
> >>> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
> >>>                                     size_t len, unsigned int flags);
> >>>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
> >>>
> >>> +#ifdef CONFIG_SYSCTL
> >>> +int fuse_sysctl_register(void);
> >>> +void fuse_sysctl_unregister(void);
> >>> +#else
> >>> +static inline int fuse_sysctl_register(void) { return 0; }
> >>> +static inline void fuse_sysctl_unregister(void) { return; }
> >>> +#endif /* CONFIG_SYSCTL */
> >>> +
> >>>  #endif /* _FS_FUSE_I_H */
> >>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> >>> index 9e69006fc026..cf333448f2d3 100644
> >>> --- a/fs/fuse/inode.c
> >>> +++ b/fs/fuse/inode.c
> >>> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
> >>>
> >>>  static int set_global_limit(const char *val, const struct kernel_param *kp);
> >>>
> >>> +/* default is no timeout */
> >>> +u32 fuse_default_req_timeout = 0;
> >>> +u32 fuse_max_req_timeout = 0;
> >>> +
> >>>  unsigned max_user_bgreq;
> >>>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
> >>>                 &max_user_bgreq, 0644);
> >>> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >>>       struct fuse_conn *fc = fm->fc;
> >>>       struct inode *root;
> >>>       struct dentry *root_dentry;
> >>> +     u32 req_timeout;
> >>>       int err;
> >>>
> >>>       err = -EINVAL;
> >>> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >>>       fc->group_id = ctx->group_id;
> >>>       fc->legacy_opts_show = ctx->legacy_opts_show;
> >>>       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
> >>> -     fc->req_timeout = ctx->req_timeout * HZ;
> >>>       fc->destroy = ctx->destroy;
> >>>       fc->no_control = ctx->no_control;
> >>>       fc->no_force_umount = ctx->no_force_umount;
> >>> +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
> >>> +     if (!fuse_max_req_timeout)
> >>> +             fc->req_timeout = req_timeout * HZ;
> >>> +     else if (!req_timeout)
> >>> +             fc->req_timeout = fuse_max_req_timeout * HZ;
> >>
> >> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
> >> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
> >> 0" mount option), the final request timeout is exactly
> >> fuse_max_req_timeout, which is unexpected as I think 0
> >> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
> >> timeout.
> >
> > fuse_max_req_timeout takes precedence over fuse_default_req_timeout
> > (eg if the system administrator wants to enforce a max limit on fuse
> > timeouts, that is imposed even if a specific fuse server didn't
> > indicate a timeout or indicated no timeout). Sorry, that wasn't made
> > clear in the documentation. I'll add that in for v5.
>
> OK that is quite confusing.  If the system admin wants to enforce a
> timeout, then a non-zero fuse_default_req_timeout is adequate.  What's
> the case where fuse_default_req_timeout must be 0, and the aystem admin
> has to impose the enforced timeout through fuse_max_req_timeout?
>
> IMHO the semantics of fuse_max_req_timeout is not straightforward and
> can be confusing if it implies an enforced timeout when no timeout is
> specified, while at the same time it also imposes a maximum limitation
> when timeout is specified.

In my point of view, max_req_timeout is the ultimate safeguard the
administrator can set to enforce a timeout on all fuse requests on the
system (eg to mitigate rogue servers). When this is set, this
guarantees that absolutely no request will take longer than
max_req_timeout for the server to respond.

My understanding of /proc/sys sysctls is that ACLs can be used to
grant certain users/groups write permission for specific sysctl
parameters. So if a user wants to enforce a default request timeout,
they can set that. If that timeout is shorter than what the max
request timeout has been set to, then the request should time out
earlier according to that desired default timeout. But if it's greater
than what the max request timeout allows, then the max request timeout
limits the timeout on the request (the max request timeout is the
absolute upper bound on how long a request reply can take). It doesn't
matter if the user set no timeout as the default req timeout - what
matters is that there is a max req timeout on the system, and that
takes precedence for enforcing how long request replies can take.


Thanks,
Joanne
>
> >
> >>
> >>> +     else
> >>> +             fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
> >>>
> >>>       err = -ENOMEM;
> >>>       root = fuse_get_root_inode(sb, ctx->rootmode);
> >>> @@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
> >>>       if (err)
> >>>               goto out3;
> >>>
> >>> +     err = fuse_sysctl_register();
> >>> +     if (err)
> >>> +             goto out4;
> >>> +
> >>>       return 0;
> >>>
> >>> + out4:
> >>> +     unregister_filesystem(&fuse_fs_type);
> >>>   out3:
> >>>       unregister_fuseblk();
> >>>   out2:
> >>> diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
> >>> new file mode 100644
> >>> index 000000000000..c87bb0ecbfa9
> >>> --- /dev/null
> >>> +++ b/fs/fuse/sysctl.c
> >>> @@ -0,0 +1,42 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> +* linux/fs/fuse/fuse_sysctl.c
> >>> +*
> >>> +* Sysctl interface to fuse parameters
> >>> +*/
> >>> +#include <linux/sysctl.h>
> >>> +
> >>> +#include "fuse_i.h"
> >>> +
> >>> +static struct ctl_table_header *fuse_table_header;
> >>> +
> >>> +static struct ctl_table fuse_sysctl_table[] = {
> >>> +     {
> >>> +             .procname       = "default_request_timeout",
> >>> +             .data           = &fuse_default_req_timeout,
> >>> +             .maxlen         = sizeof(fuse_default_req_timeout),
> >>> +             .mode           = 0644,
> >>> +             .proc_handler   = proc_douintvec,
> >>> +     },
> >>> +     {
> >>> +             .procname       = "max_request_timeout",
> >>> +             .data           = &fuse_max_req_timeout,
> >>> +             .maxlen         = sizeof(fuse_max_req_timeout),
> >>> +             .mode           = 0644,
> >>> +             .proc_handler   = proc_douintvec,
> >>> +     },
> >>
> >> Missing "{}" here?  The internal implementation of register_sysctl()
> >> depends on an empty last element of the array as the sentinel.
> >
> > Could you point me to where this is enforced? I admittedly copied this
> > formatting from how other "struct ctl_table"s are getting defined, and
> > I don't see them using an extra {} here as well? (eg pty_table,
> > inotify_table, epolll_table)
>
> Alright.  When I pick this patch to an older kernel version. e.g. 5.10,
> and use register_sysctl_table() API instead as register_sysctl() API is
> not available yet at that time.  The register_sysctl_table() API depends
> on an empty last element in the array as the sentinel as I said above,
> while the new register_sysctl_sz() API, which is called by
> register_sysctl(), actually takes the size of the ctl_table in to avoid
> array out-of-bounds access.  So please ignore my noise.  Sorry for that.
>
>
> --
> Thanks,
> Jingbo

Re: [PATCH v4 0/2] fuse: add timeout option for requests

[Joanne Koong]

On Tue, Aug 20, 2024 at 7:02 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, Aug 14, 2024 at 7:23 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > There are situations where fuse servers can become unresponsive or take
> > too long to reply to a request. Currently there is no upper bound on
> > how long a request may take, which may be frustrating to users who get
> > stuck waiting for a request to complete.
> >
> > This patchset adds a timeout option for requests and two dynamically
> > configurable fuse sysctls "default_request_timeout" and "max_request_timeout"
> > for controlling/enforcing timeout behavior system-wide.
> >
> > Existing fuse servers will not be affected unless they explicitly opt into the
> > timeout.
> >
> > v3: https://lore.kernel.org/linux-fsdevel/20240808190110.3188039-1-joannelkoong@gmail.com/
> > Changes from v3 -> v4:
> > - Fix wording on some comments to make it more clear
> > - Use simpler logic for timer (eg remove extra if checks, use mod timer API) (Josef)
> > - Sanity-check should be on FR_FINISHING not FR_FINISHED (Jingbo)
> > - Fix comment for "processing queue", add req->fpq = NULL safeguard  (Bernd)
> >
> > v2: https://lore.kernel.org/linux-fsdevel/20240730002348.3431931-1-joannelkoong@gmail.com/
> > Changes from v2 -> v3:
> > - Disarm / rearm timer in dev_do_read to handle race conditions (Bernrd)
> > - Disarm timer in error handling for fatal interrupt (Yafang)
> > - Clean up do_fuse_request_end (Jingbo)
> > - Add timer for notify retrieve requests
> > - Fix kernel test robot errors for #define no-op functions
> >
> > v1: https://lore.kernel.org/linux-fsdevel/20240717213458.1613347-1-joannelkoong@gmail.com/
> > Changes from v1 -> v2:
> > - Add timeout for background requests
> > - Handle resend race condition
> > - Add sysctls
> >
> > Joanne Koong (2):
> >   fuse: add optional kernel-enforced timeout for requests
> >   fuse: add default_request_timeout and max_request_timeout sysctls
> >
> >  Documentation/admin-guide/sysctl/fs.rst |  17 +++
> >  fs/fuse/Makefile                        |   2 +-
> >  fs/fuse/dev.c                           | 192 +++++++++++++++++++++++-
> >  fs/fuse/fuse_i.h                        |  30 ++++
> >  fs/fuse/inode.c                         |  24 +++
> >  fs/fuse/sysctl.c                        |  42 ++++++
> >  6 files changed, 298 insertions(+), 9 deletions(-)
> >  create mode 100644 fs/fuse/sysctl.c
> >
> > --
> > 2.43.5
> >
>
> For this series,
>
> Tested-by: Yafang Shao <laoar.shao@gmail.com>

Thanks for testing this version. For v5, the behavior will be modified
(if a request times out, the connection will be aborted instead of
just the request being aborted) so I'll hold off on adding your
Tested-by sign-off until you explicitly give the ok on the newer
version.

Thanks,
Joanne

>
> --
> Regards
> Yafang

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Jingbo Xu]

On 8/24/24 6:54 AM, Joanne Koong wrote:
> On Thu, Aug 22, 2024 at 7:17 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> On 8/23/24 5:19 AM, Joanne Koong wrote:
>>> On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>
>>>>
>>>>
>>>> On 8/14/24 7:22 AM, Joanne Koong wrote:
>>>>> Introduce two new sysctls, "default_request_timeout" and
>>>>> "max_request_timeout". These control timeouts on replies by the
>>>>> server to kernel-issued fuse requests.
>>>>>
>>>>> "default_request_timeout" sets a timeout if no timeout is specified by
>>>>> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
>>>>>
>>>>> "max_request_timeout" sets a maximum timeout for fuse requests. If the
>>>>> fuse server attempts to set a timeout greater than max_request_timeout,
>>>>> the system will default to max_request_timeout. Similarly, if the max
>>>>> default timeout is greater than the max request timeout, the system will
>>>>> default to the max request timeout. 0 (default) indicates no timeout should
>>>>> be enforced.
>>>>>
>>>>> $ sysctl -a | grep fuse
>>>>> fs.fuse.default_request_timeout = 0
>>>>> fs.fuse.max_request_timeout = 0
>>>>>
>>>>> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>>>> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
>>>>>
>>>>> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>>>> 0xFFFFFFFF
>>>>>
>>>>> $ sysctl -a | grep fuse
>>>>> fs.fuse.default_request_timeout = 4294967295
>>>>> fs.fuse.max_request_timeout = 0
>>>>>
>>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>>>> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>>>>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
>>>>> ---
>>>>>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
>>>>>  fs/fuse/Makefile                        |  2 +-
>>>>>  fs/fuse/fuse_i.h                        | 16 ++++++++++
>>>>>  fs/fuse/inode.c                         | 19 ++++++++++-
>>>>>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
>>>>>  5 files changed, 94 insertions(+), 2 deletions(-)
>>>>>  create mode 100644 fs/fuse/sysctl.c
>>>>>
>>>>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
>>>>> index 47499a1742bd..44fd495f69b4 100644
>>>>> --- a/Documentation/admin-guide/sysctl/fs.rst
>>>>> +++ b/Documentation/admin-guide/sysctl/fs.rst
>>>>> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
>>>>>  on a 64-bit one.
>>>>>  The current default value for ``max_user_watches`` is 4% of the
>>>>>  available low memory, divided by the "watch" cost in bytes.
>>>>> +
>>>>> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
>>>>> +=====================================================================
>>>>> +
>>>>> +This directory contains the following configuration options for FUSE
>>>>> +filesystems:
>>>>> +
>>>>> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
>>>>> +setting/getting the default timeout (in seconds) for a fuse server to
>>>>> +reply to a kernel-issued request in the event where the server did not
>>>>> +specify a timeout at mount. 0 indicates no timeout.
>>>>> +
>>>>> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
>>>>> +setting/getting the maximum timeout (in seconds) for a fuse server to
>>>>> +reply to a kernel-issued request. If the server attempts to set a
>>>>> +timeout greater than max_request_timeout, the system will use
>>>>> +max_request_timeout as the timeout. 0 indicates no timeout.
>>>>
>>>> "0 indicates no timeout"
>>>>
>>>> I think 0 max_request_timeout shall indicate that there's no explicit
>>>> maximum limitation for request_timeout.
>>>
>>> Hi Jingbo,
>>>
>>> Ah I see where the confusion in the wording is (eg that "0 indicates
>>> no timeout" could be interpreted to mean there is no timeout at all
>>> for the connection, rather than no timeout as the max limit). Thanks
>>> for pointing this out. I'll make this more explicit in v5. I'll change
>>> the wording above for the "default_request_timeout" case too.
>>>
>>>>
>>>>
>>>>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
>>>>> index 6e0228c6d0cb..cd4ef3e08ebf 100644
>>>>> --- a/fs/fuse/Makefile
>>>>> +++ b/fs/fuse/Makefile
>>>>> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
>>>>>  obj-$(CONFIG_CUSE) += cuse.o
>>>>>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
>>>>>
>>>>> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
>>>>> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
>>>>>  fuse-y += iomode.o
>>>>>  fuse-$(CONFIG_FUSE_DAX) += dax.o
>>>>>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
>>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>>>> index 0a2fa487a3bf..dae9977fa050 100644
>>>>> --- a/fs/fuse/fuse_i.h
>>>>> +++ b/fs/fuse/fuse_i.h
>>>>> @@ -47,6 +47,14 @@
>>>>>  /** Number of dentries for each connection in the control filesystem */
>>>>>  #define FUSE_CTL_NUM_DENTRIES 5
>>>>>
>>>>> +/*
>>>>> + * Default timeout (in seconds) for the server to reply to a request
>>>>> + * if no timeout was specified on mount
>>>>> + */
>>>>> +extern u32 fuse_default_req_timeout;
>>>>> +/** Max timeout (in seconds) for the server to reply to a request */
>>>>> +extern u32 fuse_max_req_timeout;
>>>>> +
>>>>>  /** List of active connections */
>>>>>  extern struct list_head fuse_conn_list;
>>>>>
>>>>> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>>>>>                                     size_t len, unsigned int flags);
>>>>>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
>>>>>
>>>>> +#ifdef CONFIG_SYSCTL
>>>>> +int fuse_sysctl_register(void);
>>>>> +void fuse_sysctl_unregister(void);
>>>>> +#else
>>>>> +static inline int fuse_sysctl_register(void) { return 0; }
>>>>> +static inline void fuse_sysctl_unregister(void) { return; }
>>>>> +#endif /* CONFIG_SYSCTL */
>>>>> +
>>>>>  #endif /* _FS_FUSE_I_H */
>>>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>>>> index 9e69006fc026..cf333448f2d3 100644
>>>>> --- a/fs/fuse/inode.c
>>>>> +++ b/fs/fuse/inode.c
>>>>> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
>>>>>
>>>>>  static int set_global_limit(const char *val, const struct kernel_param *kp);
>>>>>
>>>>> +/* default is no timeout */
>>>>> +u32 fuse_default_req_timeout = 0;
>>>>> +u32 fuse_max_req_timeout = 0;
>>>>> +
>>>>>  unsigned max_user_bgreq;
>>>>>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
>>>>>                 &max_user_bgreq, 0644);
>>>>> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>>>       struct fuse_conn *fc = fm->fc;
>>>>>       struct inode *root;
>>>>>       struct dentry *root_dentry;
>>>>> +     u32 req_timeout;
>>>>>       int err;
>>>>>
>>>>>       err = -EINVAL;
>>>>> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>>>       fc->group_id = ctx->group_id;
>>>>>       fc->legacy_opts_show = ctx->legacy_opts_show;
>>>>>       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
>>>>> -     fc->req_timeout = ctx->req_timeout * HZ;
>>>>>       fc->destroy = ctx->destroy;
>>>>>       fc->no_control = ctx->no_control;
>>>>>       fc->no_force_umount = ctx->no_force_umount;
>>>>> +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
>>>>> +     if (!fuse_max_req_timeout)
>>>>> +             fc->req_timeout = req_timeout * HZ;
>>>>> +     else if (!req_timeout)
>>>>> +             fc->req_timeout = fuse_max_req_timeout * HZ;
>>>>
>>>> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
>>>> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
>>>> 0" mount option), the final request timeout is exactly
>>>> fuse_max_req_timeout, which is unexpected as I think 0
>>>> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
>>>> timeout.
>>>
>>> fuse_max_req_timeout takes precedence over fuse_default_req_timeout
>>> (eg if the system administrator wants to enforce a max limit on fuse
>>> timeouts, that is imposed even if a specific fuse server didn't
>>> indicate a timeout or indicated no timeout). Sorry, that wasn't made
>>> clear in the documentation. I'll add that in for v5.
>>
>> OK that is quite confusing.  If the system admin wants to enforce a
>> timeout, then a non-zero fuse_default_req_timeout is adequate.  What's
>> the case where fuse_default_req_timeout must be 0, and the aystem admin
>> has to impose the enforced timeout through fuse_max_req_timeout?
>>
>> IMHO the semantics of fuse_max_req_timeout is not straightforward and
>> can be confusing if it implies an enforced timeout when no timeout is
>> specified, while at the same time it also imposes a maximum limitation
>> when timeout is specified.
> 
> In my point of view, max_req_timeout is the ultimate safeguard the
> administrator can set to enforce a timeout on all fuse requests on the
> system (eg to mitigate rogue servers). When this is set, this
> guarantees that absolutely no request will take longer than
> max_req_timeout for the server to respond.
> 
> My understanding of /proc/sys sysctls is that ACLs can be used to
> grant certain users/groups write permission for specific sysctl
> parameters. So if a user wants to enforce a default request timeout,
> they can set that. If that timeout is shorter than what the max
> request timeout has been set to, then the request should time out
> earlier according to that desired default timeout. But if it's greater
> than what the max request timeout allows, then the max request timeout
> limits the timeout on the request (the max request timeout is the
> absolute upper bound on how long a request reply can take). It doesn't
> matter if the user set no timeout as the default req timeout - what
> matters is that there is a max req timeout on the system, and that
> takes precedence for enforcing how long request replies can take.
> 

Sorry for the late reply, just back from vacation these days.

Anyway, if max_req_timeout enforces a maximum timeout no matter whether
the fuse server explicitly specifies a timeout or not, then the
semantics of fuse_default_req_timeout seems a little bit overlapped with
max_req_timeout, right?  The only place where fuse_default_req_timeout
plays a role is when ctx->req_timeout and fuse_max_req_timeout are both
zero, in which case we can get the same effect if we eliminate
fuse_default_req_timeout and configure a non-zero fuse_max_req_timeout.

-- 
Thanks,
Jingbo

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

On Tue, Aug 27, 2024 at 1:12 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> On 8/24/24 6:54 AM, Joanne Koong wrote:
> > On Thu, Aug 22, 2024 at 7:17 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> >>
> >> On 8/23/24 5:19 AM, Joanne Koong wrote:
> >>> On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 8/14/24 7:22 AM, Joanne Koong wrote:
> >>>>> Introduce two new sysctls, "default_request_timeout" and
> >>>>> "max_request_timeout". These control timeouts on replies by the
> >>>>> server to kernel-issued fuse requests.
> >>>>>
> >>>>> "default_request_timeout" sets a timeout if no timeout is specified by
> >>>>> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
> >>>>>
> >>>>> "max_request_timeout" sets a maximum timeout for fuse requests. If the
> >>>>> fuse server attempts to set a timeout greater than max_request_timeout,
> >>>>> the system will default to max_request_timeout. Similarly, if the max
> >>>>> default timeout is greater than the max request timeout, the system will
> >>>>> default to the max request timeout. 0 (default) indicates no timeout should
> >>>>> be enforced.
> >>>>>
> >>>>> $ sysctl -a | grep fuse
> >>>>> fs.fuse.default_request_timeout = 0
> >>>>> fs.fuse.max_request_timeout = 0
> >>>>>
> >>>>> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> >>>>> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> >>>>>
> >>>>> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
> >>>>> 0xFFFFFFFF
> >>>>>
> >>>>> $ sysctl -a | grep fuse
> >>>>> fs.fuse.default_request_timeout = 4294967295
> >>>>> fs.fuse.max_request_timeout = 0
> >>>>>
> >>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> >>>>> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> >>>>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> >>>>> ---
> >>>>>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
> >>>>>  fs/fuse/Makefile                        |  2 +-
> >>>>>  fs/fuse/fuse_i.h                        | 16 ++++++++++
> >>>>>  fs/fuse/inode.c                         | 19 ++++++++++-
> >>>>>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
> >>>>>  5 files changed, 94 insertions(+), 2 deletions(-)
> >>>>>  create mode 100644 fs/fuse/sysctl.c
> >>>>>
> >>>>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
> >>>>> index 47499a1742bd..44fd495f69b4 100644
> >>>>> --- a/Documentation/admin-guide/sysctl/fs.rst
> >>>>> +++ b/Documentation/admin-guide/sysctl/fs.rst
> >>>>> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
> >>>>>  on a 64-bit one.
> >>>>>  The current default value for ``max_user_watches`` is 4% of the
> >>>>>  available low memory, divided by the "watch" cost in bytes.
> >>>>> +
> >>>>> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
> >>>>> +=====================================================================
> >>>>> +
> >>>>> +This directory contains the following configuration options for FUSE
> >>>>> +filesystems:
> >>>>> +
> >>>>> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
> >>>>> +setting/getting the default timeout (in seconds) for a fuse server to
> >>>>> +reply to a kernel-issued request in the event where the server did not
> >>>>> +specify a timeout at mount. 0 indicates no timeout.
> >>>>> +
> >>>>> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
> >>>>> +setting/getting the maximum timeout (in seconds) for a fuse server to
> >>>>> +reply to a kernel-issued request. If the server attempts to set a
> >>>>> +timeout greater than max_request_timeout, the system will use
> >>>>> +max_request_timeout as the timeout. 0 indicates no timeout.
> >>>>
> >>>> "0 indicates no timeout"
> >>>>
> >>>> I think 0 max_request_timeout shall indicate that there's no explicit
> >>>> maximum limitation for request_timeout.
> >>>
> >>> Hi Jingbo,
> >>>
> >>> Ah I see where the confusion in the wording is (eg that "0 indicates
> >>> no timeout" could be interpreted to mean there is no timeout at all
> >>> for the connection, rather than no timeout as the max limit). Thanks
> >>> for pointing this out. I'll make this more explicit in v5. I'll change
> >>> the wording above for the "default_request_timeout" case too.
> >>>
> >>>>
> >>>>
> >>>>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> >>>>> index 6e0228c6d0cb..cd4ef3e08ebf 100644
> >>>>> --- a/fs/fuse/Makefile
> >>>>> +++ b/fs/fuse/Makefile
> >>>>> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
> >>>>>  obj-$(CONFIG_CUSE) += cuse.o
> >>>>>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
> >>>>>
> >>>>> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
> >>>>> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
> >>>>>  fuse-y += iomode.o
> >>>>>  fuse-$(CONFIG_FUSE_DAX) += dax.o
> >>>>>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
> >>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> >>>>> index 0a2fa487a3bf..dae9977fa050 100644
> >>>>> --- a/fs/fuse/fuse_i.h
> >>>>> +++ b/fs/fuse/fuse_i.h
> >>>>> @@ -47,6 +47,14 @@
> >>>>>  /** Number of dentries for each connection in the control filesystem */
> >>>>>  #define FUSE_CTL_NUM_DENTRIES 5
> >>>>>
> >>>>> +/*
> >>>>> + * Default timeout (in seconds) for the server to reply to a request
> >>>>> + * if no timeout was specified on mount
> >>>>> + */
> >>>>> +extern u32 fuse_default_req_timeout;
> >>>>> +/** Max timeout (in seconds) for the server to reply to a request */
> >>>>> +extern u32 fuse_max_req_timeout;
> >>>>> +
> >>>>>  /** List of active connections */
> >>>>>  extern struct list_head fuse_conn_list;
> >>>>>
> >>>>> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
> >>>>>                                     size_t len, unsigned int flags);
> >>>>>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
> >>>>>
> >>>>> +#ifdef CONFIG_SYSCTL
> >>>>> +int fuse_sysctl_register(void);
> >>>>> +void fuse_sysctl_unregister(void);
> >>>>> +#else
> >>>>> +static inline int fuse_sysctl_register(void) { return 0; }
> >>>>> +static inline void fuse_sysctl_unregister(void) { return; }
> >>>>> +#endif /* CONFIG_SYSCTL */
> >>>>> +
> >>>>>  #endif /* _FS_FUSE_I_H */
> >>>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> >>>>> index 9e69006fc026..cf333448f2d3 100644
> >>>>> --- a/fs/fuse/inode.c
> >>>>> +++ b/fs/fuse/inode.c
> >>>>> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
> >>>>>
> >>>>>  static int set_global_limit(const char *val, const struct kernel_param *kp);
> >>>>>
> >>>>> +/* default is no timeout */
> >>>>> +u32 fuse_default_req_timeout = 0;
> >>>>> +u32 fuse_max_req_timeout = 0;
> >>>>> +
> >>>>>  unsigned max_user_bgreq;
> >>>>>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
> >>>>>                 &max_user_bgreq, 0644);
> >>>>> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >>>>>       struct fuse_conn *fc = fm->fc;
> >>>>>       struct inode *root;
> >>>>>       struct dentry *root_dentry;
> >>>>> +     u32 req_timeout;
> >>>>>       int err;
> >>>>>
> >>>>>       err = -EINVAL;
> >>>>> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >>>>>       fc->group_id = ctx->group_id;
> >>>>>       fc->legacy_opts_show = ctx->legacy_opts_show;
> >>>>>       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
> >>>>> -     fc->req_timeout = ctx->req_timeout * HZ;
> >>>>>       fc->destroy = ctx->destroy;
> >>>>>       fc->no_control = ctx->no_control;
> >>>>>       fc->no_force_umount = ctx->no_force_umount;
> >>>>> +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
> >>>>> +     if (!fuse_max_req_timeout)
> >>>>> +             fc->req_timeout = req_timeout * HZ;
> >>>>> +     else if (!req_timeout)
> >>>>> +             fc->req_timeout = fuse_max_req_timeout * HZ;
> >>>>
> >>>> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
> >>>> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
> >>>> 0" mount option), the final request timeout is exactly
> >>>> fuse_max_req_timeout, which is unexpected as I think 0
> >>>> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
> >>>> timeout.
> >>>
> >>> fuse_max_req_timeout takes precedence over fuse_default_req_timeout
> >>> (eg if the system administrator wants to enforce a max limit on fuse
> >>> timeouts, that is imposed even if a specific fuse server didn't
> >>> indicate a timeout or indicated no timeout). Sorry, that wasn't made
> >>> clear in the documentation. I'll add that in for v5.
> >>
> >> OK that is quite confusing.  If the system admin wants to enforce a
> >> timeout, then a non-zero fuse_default_req_timeout is adequate.  What's
> >> the case where fuse_default_req_timeout must be 0, and the aystem admin
> >> has to impose the enforced timeout through fuse_max_req_timeout?
> >>
> >> IMHO the semantics of fuse_max_req_timeout is not straightforward and
> >> can be confusing if it implies an enforced timeout when no timeout is
> >> specified, while at the same time it also imposes a maximum limitation
> >> when timeout is specified.
> >
> > In my point of view, max_req_timeout is the ultimate safeguard the
> > administrator can set to enforce a timeout on all fuse requests on the
> > system (eg to mitigate rogue servers). When this is set, this
> > guarantees that absolutely no request will take longer than
> > max_req_timeout for the server to respond.
> >
> > My understanding of /proc/sys sysctls is that ACLs can be used to
> > grant certain users/groups write permission for specific sysctl
> > parameters. So if a user wants to enforce a default request timeout,
> > they can set that. If that timeout is shorter than what the max
> > request timeout has been set to, then the request should time out
> > earlier according to that desired default timeout. But if it's greater
> > than what the max request timeout allows, then the max request timeout
> > limits the timeout on the request (the max request timeout is the
> > absolute upper bound on how long a request reply can take). It doesn't
> > matter if the user set no timeout as the default req timeout - what
> > matters is that there is a max req timeout on the system, and that
> > takes precedence for enforcing how long request replies can take.
> >
>
> Sorry for the late reply, just back from vacation these days.
>
> Anyway, if max_req_timeout enforces a maximum timeout no matter whether
> the fuse server explicitly specifies a timeout or not, then the
> semantics of fuse_default_req_timeout seems a little bit overlapped with
> max_req_timeout, right?  The only place where fuse_default_req_timeout
> plays a role is when ctx->req_timeout and fuse_max_req_timeout are both
> zero, in which case we can get the same effect if we eliminate
> fuse_default_req_timeout and configure a non-zero fuse_max_req_timeout.
>

The behavior would not be the same if we eliminated
fuse_default_request_timeout and configured a non-zero
fuse_max_request_timeout instead. For example, say we want a default
timeout of 10 secs. With fuse_default_request_timeout set to 10 secs,
if a server specifies a timeout that is 15 secs, that is perfectly ok.
If we get rid of fuse_default_request_timeout and just use
fuse_max_request_timeout of 10 secs, that will limit servers to 10
secs even if they specified 15 secs.

Thanks,
Joanne

> --
> Thanks,
> Jingbo

Re: [PATCH v4 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Jingbo Xu]

On 8/28/24 2:13 AM, Joanne Koong wrote:
> On Tue, Aug 27, 2024 at 1:12 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> On 8/24/24 6:54 AM, Joanne Koong wrote:
>>> On Thu, Aug 22, 2024 at 7:17 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>
>>>> On 8/23/24 5:19 AM, Joanne Koong wrote:
>>>>> On Thu, Aug 22, 2024 at 12:06 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/14/24 7:22 AM, Joanne Koong wrote:
>>>>>>> Introduce two new sysctls, "default_request_timeout" and
>>>>>>> "max_request_timeout". These control timeouts on replies by the
>>>>>>> server to kernel-issued fuse requests.
>>>>>>>
>>>>>>> "default_request_timeout" sets a timeout if no timeout is specified by
>>>>>>> the fuse server on mount. 0 (default) indicates no timeout should be enforced.
>>>>>>>
>>>>>>> "max_request_timeout" sets a maximum timeout for fuse requests. If the
>>>>>>> fuse server attempts to set a timeout greater than max_request_timeout,
>>>>>>> the system will default to max_request_timeout. Similarly, if the max
>>>>>>> default timeout is greater than the max request timeout, the system will
>>>>>>> default to the max request timeout. 0 (default) indicates no timeout should
>>>>>>> be enforced.
>>>>>>>
>>>>>>> $ sysctl -a | grep fuse
>>>>>>> fs.fuse.default_request_timeout = 0
>>>>>>> fs.fuse.max_request_timeout = 0
>>>>>>>
>>>>>>> $ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>>>>>> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
>>>>>>>
>>>>>>> $ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
>>>>>>> 0xFFFFFFFF
>>>>>>>
>>>>>>> $ sysctl -a | grep fuse
>>>>>>> fs.fuse.default_request_timeout = 4294967295
>>>>>>> fs.fuse.max_request_timeout = 0
>>>>>>>
>>>>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>>>>>> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>>>>>>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
>>>>>>> ---
>>>>>>>  Documentation/admin-guide/sysctl/fs.rst | 17 ++++++++++
>>>>>>>  fs/fuse/Makefile                        |  2 +-
>>>>>>>  fs/fuse/fuse_i.h                        | 16 ++++++++++
>>>>>>>  fs/fuse/inode.c                         | 19 ++++++++++-
>>>>>>>  fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
>>>>>>>  5 files changed, 94 insertions(+), 2 deletions(-)
>>>>>>>  create mode 100644 fs/fuse/sysctl.c
>>>>>>>
>>>>>>> diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
>>>>>>> index 47499a1742bd..44fd495f69b4 100644
>>>>>>> --- a/Documentation/admin-guide/sysctl/fs.rst
>>>>>>> +++ b/Documentation/admin-guide/sysctl/fs.rst
>>>>>>> @@ -332,3 +332,20 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
>>>>>>>  on a 64-bit one.
>>>>>>>  The current default value for ``max_user_watches`` is 4% of the
>>>>>>>  available low memory, divided by the "watch" cost in bytes.
>>>>>>> +
>>>>>>> +5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
>>>>>>> +=====================================================================
>>>>>>> +
>>>>>>> +This directory contains the following configuration options for FUSE
>>>>>>> +filesystems:
>>>>>>> +
>>>>>>> +``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
>>>>>>> +setting/getting the default timeout (in seconds) for a fuse server to
>>>>>>> +reply to a kernel-issued request in the event where the server did not
>>>>>>> +specify a timeout at mount. 0 indicates no timeout.
>>>>>>> +
>>>>>>> +``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
>>>>>>> +setting/getting the maximum timeout (in seconds) for a fuse server to
>>>>>>> +reply to a kernel-issued request. If the server attempts to set a
>>>>>>> +timeout greater than max_request_timeout, the system will use
>>>>>>> +max_request_timeout as the timeout. 0 indicates no timeout.
>>>>>>
>>>>>> "0 indicates no timeout"
>>>>>>
>>>>>> I think 0 max_request_timeout shall indicate that there's no explicit
>>>>>> maximum limitation for request_timeout.
>>>>>
>>>>> Hi Jingbo,
>>>>>
>>>>> Ah I see where the confusion in the wording is (eg that "0 indicates
>>>>> no timeout" could be interpreted to mean there is no timeout at all
>>>>> for the connection, rather than no timeout as the max limit). Thanks
>>>>> for pointing this out. I'll make this more explicit in v5. I'll change
>>>>> the wording above for the "default_request_timeout" case too.
>>>>>
>>>>>>
>>>>>>
>>>>>>> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
>>>>>>> index 6e0228c6d0cb..cd4ef3e08ebf 100644
>>>>>>> --- a/fs/fuse/Makefile
>>>>>>> +++ b/fs/fuse/Makefile
>>>>>>> @@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
>>>>>>>  obj-$(CONFIG_CUSE) += cuse.o
>>>>>>>  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
>>>>>>>
>>>>>>> -fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
>>>>>>> +fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
>>>>>>>  fuse-y += iomode.o
>>>>>>>  fuse-$(CONFIG_FUSE_DAX) += dax.o
>>>>>>>  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
>>>>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>>>>>> index 0a2fa487a3bf..dae9977fa050 100644
>>>>>>> --- a/fs/fuse/fuse_i.h
>>>>>>> +++ b/fs/fuse/fuse_i.h
>>>>>>> @@ -47,6 +47,14 @@
>>>>>>>  /** Number of dentries for each connection in the control filesystem */
>>>>>>>  #define FUSE_CTL_NUM_DENTRIES 5
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Default timeout (in seconds) for the server to reply to a request
>>>>>>> + * if no timeout was specified on mount
>>>>>>> + */
>>>>>>> +extern u32 fuse_default_req_timeout;
>>>>>>> +/** Max timeout (in seconds) for the server to reply to a request */
>>>>>>> +extern u32 fuse_max_req_timeout;
>>>>>>> +
>>>>>>>  /** List of active connections */
>>>>>>>  extern struct list_head fuse_conn_list;
>>>>>>>
>>>>>>> @@ -1486,4 +1494,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>>>>>>>                                     size_t len, unsigned int flags);
>>>>>>>  ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
>>>>>>>
>>>>>>> +#ifdef CONFIG_SYSCTL
>>>>>>> +int fuse_sysctl_register(void);
>>>>>>> +void fuse_sysctl_unregister(void);
>>>>>>> +#else
>>>>>>> +static inline int fuse_sysctl_register(void) { return 0; }
>>>>>>> +static inline void fuse_sysctl_unregister(void) { return; }
>>>>>>> +#endif /* CONFIG_SYSCTL */
>>>>>>> +
>>>>>>>  #endif /* _FS_FUSE_I_H */
>>>>>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>>>>>> index 9e69006fc026..cf333448f2d3 100644
>>>>>>> --- a/fs/fuse/inode.c
>>>>>>> +++ b/fs/fuse/inode.c
>>>>>>> @@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
>>>>>>>
>>>>>>>  static int set_global_limit(const char *val, const struct kernel_param *kp);
>>>>>>>
>>>>>>> +/* default is no timeout */
>>>>>>> +u32 fuse_default_req_timeout = 0;
>>>>>>> +u32 fuse_max_req_timeout = 0;
>>>>>>> +
>>>>>>>  unsigned max_user_bgreq;
>>>>>>>  module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
>>>>>>>                 &max_user_bgreq, 0644);
>>>>>>> @@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>>>>>       struct fuse_conn *fc = fm->fc;
>>>>>>>       struct inode *root;
>>>>>>>       struct dentry *root_dentry;
>>>>>>> +     u32 req_timeout;
>>>>>>>       int err;
>>>>>>>
>>>>>>>       err = -EINVAL;
>>>>>>> @@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>>>>>>       fc->group_id = ctx->group_id;
>>>>>>>       fc->legacy_opts_show = ctx->legacy_opts_show;
>>>>>>>       fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
>>>>>>> -     fc->req_timeout = ctx->req_timeout * HZ;
>>>>>>>       fc->destroy = ctx->destroy;
>>>>>>>       fc->no_control = ctx->no_control;
>>>>>>>       fc->no_force_umount = ctx->no_force_umount;
>>>>>>> +     req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
>>>>>>> +     if (!fuse_max_req_timeout)
>>>>>>> +             fc->req_timeout = req_timeout * HZ;
>>>>>>> +     else if (!req_timeout)
>>>>>>> +             fc->req_timeout = fuse_max_req_timeout * HZ;
>>>>>>
>>>>>> So if fuse_max_req_timeout is non-zero and req_timeout is zero (either
>>>>>> because of 0 fuse_default_req_timeout, or explicit "-o request_timeout =
>>>>>> 0" mount option), the final request timeout is exactly
>>>>>> fuse_max_req_timeout, which is unexpected as I think 0
>>>>>> fuse_default_req_timeout, or "-o request_timeout=0" shall indicate no
>>>>>> timeout.
>>>>>
>>>>> fuse_max_req_timeout takes precedence over fuse_default_req_timeout
>>>>> (eg if the system administrator wants to enforce a max limit on fuse
>>>>> timeouts, that is imposed even if a specific fuse server didn't
>>>>> indicate a timeout or indicated no timeout). Sorry, that wasn't made
>>>>> clear in the documentation. I'll add that in for v5.
>>>>
>>>> OK that is quite confusing.  If the system admin wants to enforce a
>>>> timeout, then a non-zero fuse_default_req_timeout is adequate.  What's
>>>> the case where fuse_default_req_timeout must be 0, and the aystem admin
>>>> has to impose the enforced timeout through fuse_max_req_timeout?
>>>>
>>>> IMHO the semantics of fuse_max_req_timeout is not straightforward and
>>>> can be confusing if it implies an enforced timeout when no timeout is
>>>> specified, while at the same time it also imposes a maximum limitation
>>>> when timeout is specified.
>>>
>>> In my point of view, max_req_timeout is the ultimate safeguard the
>>> administrator can set to enforce a timeout on all fuse requests on the
>>> system (eg to mitigate rogue servers). When this is set, this
>>> guarantees that absolutely no request will take longer than
>>> max_req_timeout for the server to respond.
>>>
>>> My understanding of /proc/sys sysctls is that ACLs can be used to
>>> grant certain users/groups write permission for specific sysctl
>>> parameters. So if a user wants to enforce a default request timeout,
>>> they can set that. If that timeout is shorter than what the max
>>> request timeout has been set to, then the request should time out
>>> earlier according to that desired default timeout. But if it's greater
>>> than what the max request timeout allows, then the max request timeout
>>> limits the timeout on the request (the max request timeout is the
>>> absolute upper bound on how long a request reply can take). It doesn't
>>> matter if the user set no timeout as the default req timeout - what
>>> matters is that there is a max req timeout on the system, and that
>>> takes precedence for enforcing how long request replies can take.
>>>
>>
>> Sorry for the late reply, just back from vacation these days.
>>
>> Anyway, if max_req_timeout enforces a maximum timeout no matter whether
>> the fuse server explicitly specifies a timeout or not, then the
>> semantics of fuse_default_req_timeout seems a little bit overlapped with
>> max_req_timeout, right?  The only place where fuse_default_req_timeout
>> plays a role is when ctx->req_timeout and fuse_max_req_timeout are both
>> zero, in which case we can get the same effect if we eliminate
>> fuse_default_req_timeout and configure a non-zero fuse_max_req_timeout.
>>
> 
> The behavior would not be the same if we eliminated
> fuse_default_request_timeout and configured a non-zero
> fuse_max_request_timeout instead. For example, say we want a default
> timeout of 10 secs. With fuse_default_request_timeout set to 10 secs,
> if a server specifies a timeout that is 15 secs, that is perfectly ok.
> If we get rid of fuse_default_request_timeout and just use
> fuse_max_request_timeout of 10 secs, that will limit servers to 10
> secs even if they specified 15 secs.
> 

Alright, make sense to me.

I suddenly realized that zero timeout (either the fuse server explicitly
specifies "-o request_timeout=0" or fuse_default_req_timeout is 0)
actually indicates an infinite timeout。  In this perspective, capping
the (infinite) timeout to fuse_max_request_timeout indeed makes sense.


-- 
Thanks,
Jingbo