From 8efdff35e3052e44d519ccfb9d50594f5df0240b Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Fri, 4 Mar 2022 20:11:59 +0000 Subject: [PATCH] ANDROID: fuse-bpf: Move fd operations to be synchronous Bug: 222619123 Test: fuse_test passes, on cuttlefish CtsCameraTestCases passes Signed-off-by: Paul Lawrence Change-Id: I54c148206b5ad5ae5737939bcb076cbe6c40129c --- fs/fuse/backing.c | 78 +++++-------------- fs/fuse/dev.c | 12 +++ fs/fuse/dir.c | 34 ++++---- fs/fuse/fuse_i.h | 6 +- fs/fuse/inode.c | 3 - .../selftests/filesystems/fuse/fuse_test.c | 4 +- 6 files changed, 55 insertions(+), 82 deletions(-) diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c index 3c38a3c6cd43..7cb9a17eb71c 100644 --- a/fs/fuse/backing.c +++ b/fs/fuse/backing.c @@ -13,53 +13,12 @@ #include "../internal.h" -/* Reimplement these functions since fget_task is not exported */ -static struct file *fuse__fget_files(struct files_struct *files, - unsigned int fd, fmode_t mask, unsigned int refs) +struct bpf_prog *fuse_get_bpf_prog(struct file *file) { - struct file *file; - - rcu_read_lock(); -loop: - file = fcheck_files(files, fd); - if (file) { - /* File object ref couldn't be taken. - * dup2() atomicity guarantee is the reason - * we loop to catch the new file (or NULL pointer) - */ - if (file->f_mode & mask) - file = NULL; - else if (!get_file_rcu_many(file, refs)) - goto loop; - } - rcu_read_unlock(); - return file; -} - -static struct file *fuse_fget_task(struct task_struct *task, unsigned int fd) -{ - struct file *file = NULL; - - task_lock(task); - if (task->files) - file = fuse__fget_files(task->files, fd, 0, 1); - task_unlock(task); - - return file; -} - -struct file *fuse_fget(struct fuse_conn *fc, unsigned int fd) -{ - return fuse_fget_task(fc->task, fd); -} - -struct bpf_prog *fuse_get_bpf_prog(struct fuse_conn *fc, unsigned int fd) -{ - struct file *bpf_file = fuse_fget(fc, fd); struct bpf_prog *bpf_prog = ERR_PTR(-EINVAL); - if (!bpf_file) - goto out; + if (!file || IS_ERR(file)) + return bpf_prog; /** * Two ways of getting a bpf prog from another task's fd, since * bpf_prog_get_type_dev only works with an fd @@ -75,10 +34,10 @@ struct bpf_prog *fuse_get_bpf_prog(struct fuse_conn *fc, unsigned int fd) * compilable as a module. */ #if 0 - if (bpf_file->f_op != &bpf_prog_fops) + if (file->f_op != &bpf_prog_fops) goto out; - bpf_prog = bpf_file->private_data; + bpf_prog = file->private_data; if (bpf_prog->type == BPF_PROG_TYPE_FUSE) bpf_prog_inc(bpf_prog); else @@ -86,24 +45,25 @@ struct bpf_prog *fuse_get_bpf_prog(struct fuse_conn *fc, unsigned int fd) #else { - int task_fd = get_unused_fd_flags(bpf_file->f_flags); + int task_fd = get_unused_fd_flags(file->f_flags); if (task_fd < 0) goto out; - fd_install(task_fd, bpf_file); + + fd_install(task_fd, file); bpf_prog = bpf_prog_get_type_dev(task_fd, BPF_PROG_TYPE_FUSE, false); - __close_fd(current->files, task_fd); - /* TODO I think this file is probably being leaked */ - bpf_file = NULL; + /* Close the fd, which also closes the file */ + __close_fd(current->files, task_fd); + file = NULL; } #endif out: - if (bpf_file) - fput(bpf_file); + if (file) + fput(file); return bpf_prog; } @@ -992,8 +952,11 @@ struct dentry *fuse_lookup_finalize(struct fuse_args *fa, struct inode *dir, break; case FUSE_ACTION_REPLACE: { - struct fuse_conn *fc = get_fuse_mount(dir)->fc; - struct bpf_prog *bpf_prog = fuse_get_bpf_prog(fc, febo->bpf_fd); + struct file *bpf_file = (struct file*) febo->bpf_fd; + struct bpf_prog *bpf_prog = ERR_PTR(-EINVAL); + + if (bpf_file && !IS_ERR(bpf_file)) + bpf_prog = fuse_get_bpf_prog(bpf_file); if (IS_ERR(bpf_prog)) return ERR_PTR(PTR_ERR(bpf_prog)); @@ -1022,9 +985,8 @@ struct dentry *fuse_lookup_finalize(struct fuse_args *fa, struct inode *dir, struct file *backing_file; fc = get_fuse_mount(dir)->fc; - backing_file = fuse_fget(fc, febo->backing_fd); - __close_fd(fc->task->files, febo->backing_fd); - if (!backing_file) + backing_file = (struct file *) febo->backing_fd; + if (!backing_file || IS_ERR(backing_file)) return ERR_PTR(-EIO); iput(get_fuse_inode(inode)->backing_inode); diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 0085728fe64a..2b0e9f16fe24 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1943,6 +1943,18 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud, kern_path(path, 0, req->args->canonical_path); } + if (!err && (req->in.h.opcode == FUSE_LOOKUP || + req->in.h.opcode == (FUSE_LOOKUP | FUSE_POSTFILTER)) && + req->args->out_args[1].size == sizeof(struct fuse_entry_bpf_out)) { + struct fuse_entry_bpf_out *febo = (struct fuse_entry_bpf_out *) + req->args->out_args[1].value; + + if (febo->backing_action == FUSE_ACTION_REPLACE) + febo->backing_fd = (uint64_t) fget(febo->backing_fd); + if (febo->bpf_action == FUSE_ACTION_REPLACE) + febo->bpf_fd = (uint64_t) fget(febo->bpf_fd); + } + spin_lock(&fpq->lock); clear_bit(FR_LOCKED, &req->flags); if (!fpq->connected) diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 64b3b8a2718c..d123058e89a7 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -243,8 +243,19 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags) * change the backing file ever, so not sure what is correct * here yet, especially as we can't return an error to user */ - if (bpf_outarg.backing_action == FUSE_ACTION_REPLACE) - __close_fd(fm->fc->task->files, bpf_outarg.backing_fd); + if (bpf_outarg.backing_action == FUSE_ACTION_REPLACE) { + struct file *file = (struct file *) bpf_outarg.backing_fd; + + if (file && !IS_ERR(file)) + fput(file); + } + + if (bpf_outarg.bpf_action == FUSE_ACTION_REPLACE) { + struct file *file = (struct file *) bpf_outarg.bpf_fd; + + if (file && !IS_ERR(file)) + fput(file); + } /* Zero nodeid is same as -ENOENT */ if (!ret && !outarg.nodeid) @@ -529,26 +540,21 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name if (bpf_outarg->backing_action != FUSE_ACTION_REPLACE) goto out_queue_forget; - backing_file = fuse_fget(fm->fc, bpf_outarg->backing_fd); - if (!backing_file) + backing_file = (struct file *) bpf_outarg->backing_fd; + if (!backing_file || IS_ERR(backing_file)) goto out_queue_forget; - /* TODO userspace doesn't really know when the right time to - * close the passed fd is. This because after replying to the - * driver request, so assume that after a lookup with bpf_args, - * the daemon passes the fd ownership to the kernel, which also - * takes care of closing it at the right time. - */ - __close_fd(fm->fc->task->files, bpf_outarg->backing_fd); - backing_inode = backing_file->f_inode; *inode = fuse_iget_backing(sb, backing_inode); if (!*inode) goto bpf_outarg_out; if (bpf_outarg->bpf_action == FUSE_ACTION_REPLACE) { - struct bpf_prog *bpf_prog = fuse_get_bpf_prog(fm->fc, - bpf_outarg->bpf_fd); + struct file *bpf_file = (struct file*) bpf_outarg->bpf_fd; + struct bpf_prog *bpf_prog = ERR_PTR(-EINVAL); + + if (bpf_file && !IS_ERR(bpf_file)) + bpf_prog = fuse_get_bpf_prog(bpf_file);; if (IS_ERR(bpf_prog)) { iput(*inode); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 675db33e9c0c..a75d82539ac7 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -845,9 +845,6 @@ struct fuse_conn { /** Protects passthrough_req */ spinlock_t passthrough_req_lock; - - /** task_struct for fd lookups in fuse-bpf */ - struct task_struct *task; }; /* @@ -1310,8 +1307,7 @@ ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma); /* backing.c */ -struct file *fuse_fget(struct fuse_conn *fc, unsigned int fd); -struct bpf_prog *fuse_get_bpf_prog(struct fuse_conn *fc, unsigned int fd); +struct bpf_prog *fuse_get_bpf_prog(struct file *file); /* * Dummy io passed to fuse_bpf_backing when io operation needs no scratch space diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index e529fe0e147f..e9e445d7959d 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1218,7 +1218,6 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, fc->minor = arg->minor; fc->max_write = arg->minor < 5 ? 4096 : arg->max_write; fc->max_write = max_t(unsigned, 4096, fc->max_write); - fc->task = get_task_struct(current); fc->conn_init = 1; } kfree(ia); @@ -1295,8 +1294,6 @@ void fuse_free_conn(struct fuse_conn *fc) idr_for_each(&fc->passthrough_req, free_fuse_passthrough, NULL); idr_destroy(&fc->passthrough_req); kfree_rcu(fc, rcu); - if (fc->task) - put_task_struct(fc->task); } EXPORT_SYMBOL_GPL(fuse_free_conn); diff --git a/tools/testing/selftests/filesystems/fuse/fuse_test.c b/tools/testing/selftests/filesystems/fuse/fuse_test.c index 96178eea67f8..9117b21cc461 100644 --- a/tools/testing/selftests/filesystems/fuse/fuse_test.c +++ b/tools/testing/selftests/filesystems/fuse/fuse_test.c @@ -1145,6 +1145,7 @@ static int bpf_test_set_backing(const char *mount_dir) })); read(fuse_dev, bytes_in, sizeof(bytes_in)); TESTSYSCALL(close(bpf_fd)); + TESTSYSCALL(close(backing_fd)); FUSE_DONE result = TEST_SUCCESS; @@ -1234,8 +1235,7 @@ static int bpf_test_remove_backing(const char *mount_dir) while (read(fuse_dev, bytes_in, sizeof(bytes_in)) != -1) ; - TESTEQUAL(close(backing_fd), -1); - TESTEQUAL(errno, EBADF); + TESTSYSCALL(close(backing_fd)); FUSE_DONE result = TEST_SUCCESS;