Re: strange interaction between fuse + pidns

[Miklos Szeredi]

On Thu, 23 Jun 2022 at 19:21, Tycho Andersen <tycho@tycho.pizza> wrote:

>         /*
> -        * Either request is already in userspace, or it was forced.
> -        * Wait it out.
> +        * Womp womp. We sent a request to userspace and now we're getting
> +        * killed.
>          */
> -       wait_event(req->waitq, test_bit(FR_FINISHED, &req->flags));

You can't remove this, it's a crucial part of fuse request handling.
Yes, it causes pain, but making *sent* requests killable is a lot more
work.

For one: need to duplicate caller's locking state (i_rwsem, ...) and
move the request into a backround queue instead of just finishing it
off immediately so that the shadow locking can be torn down when the
reply actually arrives.  This affects a lot of requests.

Or we could special case FUSE_FLUSH, which doesn't have any locking.

The reason force=true is needed for FUSE_FLUSH is because it affects
posix lock state.   Not waiting for the reply if the task is killed
could have observable consequences, but my guess is that it's an
uninteresting corner case and would not cause regressions in real
life.

Can you try the attached untested patch?

Thanks,
Miklos
---
 fs/fuse/dev.c    |    9 ++++++++-
 fs/fuse/file.c   |    1 +
 fs/fuse/fuse_i.h |    1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -397,6 +397,12 @@ static void request_wait_answer(struct f
 			req->out.h.error = -EINTR;
 			return;
 		}
+		if (req->args->killable) {
+			req->out.h.error = -EINTR;
+			/* fuse_request_end() will drop final ref */
+			spin_unlock(&fiq->lock);
+			return;
+		}
 		spin_unlock(&fiq->lock);
 	}
 
@@ -494,7 +500,8 @@ ssize_t fuse_simple_request(struct fuse_
 			fuse_force_creds(req);
 
 		__set_bit(FR_WAITING, &req->flags);
-		__set_bit(FR_FORCE, &req->flags);
+		if (!args->killable)
+			__set_bit(FR_FORCE, &req->flags);
 	} else {
 		WARN_ON(args->nocreds);
 		req = fuse_get_req(fm, false);
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -504,6 +504,7 @@ static int fuse_flush(struct file *file,
 	args.in_args[0].size = sizeof(inarg);
 	args.in_args[0].value = &inarg;
 	args.force = true;
+	args.killable = true;
 
 	err = fuse_simple_request(fm, &args);
 	if (err == -ENOSYS) {
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -261,6 +261,7 @@ struct fuse_args {
 	bool page_zeroing:1;
 	bool page_replace:1;
 	bool may_block:1;
+	bool killable:1;
 	struct fuse_in_arg in_args[3];
 	struct fuse_arg out_args[2];
 	void (*end)(struct fuse_mount *fm, struct fuse_args *args, int error);