From a06f77a0ddc8107ba6a41b4e3ca9c8acf9ffd751 Mon Sep 17 00:00:00 2001 From: Dmitrii Merkurev Date: Fri, 8 Jul 2022 23:38:18 +0000 Subject: [PATCH] ANDROID: fuse-bpf: Fix revalidate error path and backing handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we have 2 different problems 1. Every revalidate considered as a error because of added args->out_argvar = true; inside fuse_lookup_init which makes fuse_simple_request return out argument size which is considered as an error by revalidate code. 2. We’re ignoring backing_fd and bpf_program set by daemon lookup code called by revalidate. Problem 1 makes any revalidate (lookup to userspace) useless and any result lead us to the full lookup because it was interpreted as an error. This CL fixes both and introducing revalidate test case which makes sure: 1. We’re receiving only one lookup as a part of revalidate 2. We’re setting backing_fd as a part of revalidate’s lookup result Test is failed before the fix and passed after. Bug: 219958836 Test: Booted device 5 times to make sure we’re not receiving redundant lookups anymore. Test: selftests Signed-off-by: Dmitrii Merkurev Change-Id: Ifa62e56b42ca5580b25682eb5f16b5c91826cf49 --- fs/fuse/backing.c | 177 +++++++++++------- fs/fuse/dir.c | 67 +++---- fs/fuse/fuse_i.h | 7 + .../selftests/filesystems/fuse/fuse_test.c | 114 ++++++++++- 4 files changed, 254 insertions(+), 111 deletions(-) diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c index 3fea31bec40a..d21ab1a076ec 100644 --- a/fs/fuse/backing.c +++ b/fs/fuse/backing.c @@ -1172,6 +1172,105 @@ int fuse_lookup_backing(struct fuse_bpf_args *fa, struct inode *dir, return 0; } +int handle_inode_backing_fd(struct inode *inode, struct dentry *entry, + struct fuse_entry_bpf_out *febo, + struct fuse_entry_bpf *feb) +{ + struct fuse_inode *fi = get_fuse_inode(inode); + struct fuse_dentry *fd = get_fuse_dentry(entry); + int ret = 0; + + switch (febo->backing_action) { + case FUSE_ACTION_KEEP: + /* backing inode/path are added in fuse_lookup_backing */ + break; + + case FUSE_ACTION_REMOVE: + iput(fi->backing_inode); + fi->backing_inode = NULL; + path_put_init(&fd->backing_path); + break; + + case FUSE_ACTION_REPLACE: { + struct file *backing_file = feb->backing_file; + + if (!backing_file) + return -EINVAL; + if (IS_ERR(backing_file)) + return PTR_ERR(backing_file); + + if (fi->backing_inode) + iput(fi->backing_inode); + fi->backing_inode = backing_file->f_inode; + ihold(fi->backing_inode); + + path_put(&fd->backing_path); + fd->backing_path = backing_file->f_path; + path_get(&fd->backing_path); + + fput(backing_file); + break; + } + + default: + return -EINVAL; + } + + return ret; +} + +int handle_inode_bpf(struct inode *inode, struct inode *parent, + struct fuse_entry_bpf_out *febo, + struct fuse_entry_bpf *feb) +{ + struct fuse_inode *fi = get_fuse_inode(inode); + struct fuse_inode *pi; + int ret = 0; + + // Parent isn't presented, but we want to keep + // Don't touch bpf program at all in this case + if (febo->bpf_action == FUSE_ACTION_KEEP && !parent) { + goto out; + } + + if (fi->bpf) { + bpf_prog_put(fi->bpf); + fi->bpf = NULL; + } + + switch (febo->bpf_action) { + case FUSE_ACTION_KEEP: + pi = get_fuse_inode(parent); + fi->bpf = pi->bpf; + if (fi->bpf) + bpf_prog_inc(fi->bpf); + break; + + case FUSE_ACTION_REMOVE: + break; + + case FUSE_ACTION_REPLACE: { + struct file *bpf_file = feb->bpf_file; + 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 PTR_ERR(bpf_prog); + + fi->bpf = bpf_prog; + break; + } + + default: + return -EINVAL; + } + +out: + return ret; +} + struct dentry *fuse_lookup_finalize(struct fuse_bpf_args *fa, struct inode *dir, struct dentry *entry, unsigned int flags) { @@ -1182,6 +1281,7 @@ struct dentry *fuse_lookup_finalize(struct fuse_bpf_args *fa, struct inode *dir, struct fuse_entry_out *feo = fa->out_args[0].value; struct fuse_entry_bpf_out *febo = fa->out_args[1].value; struct fuse_entry_bpf *feb = container_of(febo, struct fuse_entry_bpf, out); + int error = -1; u64 target_nodeid = 0; fd = get_fuse_dentry(entry); @@ -1202,78 +1302,13 @@ struct dentry *fuse_lookup_finalize(struct fuse_bpf_args *fa, struct inode *dir, if (IS_ERR(inode)) return ERR_PTR(PTR_ERR(inode)); - /* TODO Make sure this handles invalid handles */ - /* TODO Do we need the same code in revalidate */ - if (get_fuse_inode(inode)->bpf) { - bpf_prog_put(get_fuse_inode(inode)->bpf); - get_fuse_inode(inode)->bpf = NULL; - } + error = handle_inode_bpf(inode, dir, febo, feb); + if (error) + return ERR_PTR(error); - switch (febo->bpf_action) { - case FUSE_ACTION_KEEP: - get_fuse_inode(inode)->bpf = get_fuse_inode(dir)->bpf; - if (get_fuse_inode(inode)->bpf) - bpf_prog_inc(get_fuse_inode(inode)->bpf); - break; - - case FUSE_ACTION_REMOVE: - get_fuse_inode(inode)->bpf = NULL; - break; - - case FUSE_ACTION_REPLACE: { - struct file *bpf_file = feb->bpf_file; - 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)); - - get_fuse_inode(inode)->bpf = bpf_prog; - break; - } - - default: - return ERR_PTR(-EIO); - } - - switch (febo->backing_action) { - case FUSE_ACTION_KEEP: - /* backing inode/path are added in fuse_lookup_backing */ - break; - - case FUSE_ACTION_REMOVE: - iput(get_fuse_inode(inode)->backing_inode); - get_fuse_inode(inode)->backing_inode = NULL; - path_put_init(&get_fuse_dentry(entry)->backing_path); - break; - - case FUSE_ACTION_REPLACE: { - struct fuse_conn *fc; - struct file *backing_file; - - fc = get_fuse_mount(dir)->fc; - backing_file = feb->backing_file; - if (!backing_file || IS_ERR(backing_file)) - return ERR_PTR(-EIO); - - iput(get_fuse_inode(inode)->backing_inode); - get_fuse_inode(inode)->backing_inode = - backing_file->f_inode; - ihold(get_fuse_inode(inode)->backing_inode); - - path_put(&get_fuse_dentry(entry)->backing_path); - get_fuse_dentry(entry)->backing_path = backing_file->f_path; - path_get(&get_fuse_dentry(entry)->backing_path); - - fput(backing_file); - break; - } - - default: - return ERR_PTR(-EIO); - } + error = handle_inode_backing_fd(inode, entry, febo, feb); + if (error) + return ERR_PTR(error); get_fuse_inode(inode)->nodeid = feo->nodeid; diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index b8debd29573c..b679b60a5d9e 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -238,25 +238,23 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags) ret = fuse_simple_request(fm, &args); dput(parent); - /* - * TODO This doesn't seem sufficient, though we don't plan to - * 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_arg.out.backing_action == FUSE_ACTION_REPLACE) { - struct file *file = bpf_arg.backing_file; +#ifdef CONFIG_FUSE_BPF + if (ret == sizeof(bpf_arg.out)) { + ret = -ENOENT; + if (!entry) + goto out; - if (file && !IS_ERR(file)) - fput(file); + ret = handle_inode_backing_fd(inode, entry, + &bpf_arg.out, &bpf_arg); + if (ret) + goto out; + + ret = handle_inode_bpf(inode, entry->d_parent->d_inode, + &bpf_arg.out, &bpf_arg); + if (ret) + goto out; } - - if (bpf_arg.out.bpf_action == FUSE_ACTION_REPLACE) { - struct file *file = bpf_arg.bpf_file; - - if (file && !IS_ERR(file)) - fput(file); - } - +#endif /* Zero nodeid is same as -ENOENT */ if (!ret && !outarg.nodeid) ret = -ENOENT; @@ -528,7 +526,6 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name #ifdef CONFIG_FUSE_BPF if (err == sizeof(bpf_arg.out)) { /* TODO Make sure this handles invalid handles */ - /* TODO Do we need the same code in revalidate */ struct file *backing_file; struct inode *backing_inode; @@ -537,37 +534,29 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name goto out_queue_forget; err = -EINVAL; - if (bpf_arg.out.backing_action != FUSE_ACTION_REPLACE) + backing_file = bpf_arg.backing_file; + if (!backing_file) goto out_queue_forget; - backing_file = bpf_arg.backing_file; - if (!backing_file || IS_ERR(backing_file)) + if (IS_ERR(backing_file)) { + err = PTR_ERR(backing_file); goto out_queue_forget; + } backing_inode = backing_file->f_inode; *inode = fuse_iget_backing(sb, outarg->nodeid, backing_inode); if (!*inode) goto bpf_arg_out; - if (bpf_arg.out.bpf_action == FUSE_ACTION_REPLACE) { - struct file *bpf_file = bpf_arg.bpf_file; - 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); - *inode = NULL; - err = PTR_ERR(bpf_prog); - goto bpf_arg_out; - } - get_fuse_inode(*inode)->bpf = bpf_prog; - } - - get_fuse_dentry(entry)->backing_path = backing_file->f_path; - path_get(&get_fuse_dentry(entry)->backing_path); + err = handle_inode_backing_fd(*inode, entry, + &bpf_arg.out, &bpf_arg); + if (err) + goto out; + err = handle_inode_bpf(*inode, NULL, + &bpf_arg.out, &bpf_arg); + if (err) + goto out; bpf_arg_out: fput(backing_file); } else diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 38bdf467ee64..03c5ab29997a 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1597,6 +1597,13 @@ struct fuse_lookup_io { struct fuse_entry_bpf feb; }; +int handle_inode_backing_fd(struct inode *inode, struct dentry *entry, + struct fuse_entry_bpf_out *febo, + struct fuse_entry_bpf *feb); +int handle_inode_bpf(struct inode *inode, struct inode *parent, + struct fuse_entry_bpf_out *febo, + struct fuse_entry_bpf *feb); + int fuse_lookup_initialize(struct fuse_bpf_args *fa, struct fuse_lookup_io *feo, struct inode *dir, struct dentry *entry, unsigned int flags); int fuse_lookup_backing(struct fuse_bpf_args *fa, struct inode *dir, diff --git a/tools/testing/selftests/filesystems/fuse/fuse_test.c b/tools/testing/selftests/filesystems/fuse/fuse_test.c index a30648ece91b..cc47e7f5e3fd 100644 --- a/tools/testing/selftests/filesystems/fuse/fuse_test.c +++ b/tools/testing/selftests/filesystems/fuse/fuse_test.c @@ -1779,6 +1779,117 @@ out: return result; } +/* + * State: + * Original: dst/folder1/content.txt + * ^ + * | + * | + * Backing: src/folder1/content.txt + * + * Step 1: open(folder1) - lookup folder1 with entry_timeout set to 0 + * Step 2: open(folder1) - lookup folder1 again to trigger revalidate wich will + * set backing fd + * + * Check 1: cat(content.txt) - check not receiving call on the fuse daemon + * and content is the same + */ +static int bpf_test_revalidate_handle_backing_fd(const char *mount_dir) +{ + const char *folder1 = "folder1"; + const char *content_file = "content.txt"; + const char *content = "hello world"; + int result = TEST_FAILURE; + int fuse_dev = -1; + int src_fd = -1; + int content_fd = -1; + int pid = -1; + int status; + TESTSYSCALL(s_mkdir(s_path(s(ft_src), s(folder1)), 0777)); + TEST(content_fd = s_creat(s_pathn(3, s(ft_src), s(folder1), s(content_file)), 0777), + content_fd != -1); + TESTEQUAL(write(content_fd, content, strlen(content)), strlen(content)); + TESTSYSCALL(close(content_fd)); + content_fd = -1; + TESTEQUAL(mount_fuse_no_init(mount_dir, -1, -1, &fuse_dev), 0); + FUSE_ACTION + int dst_folder1_fd = -1; + int dst_content_fd = -1; + int dst_content_read_size = -1; + char content_buffer[9] = {0}; + // Step 1: Lookup folder1 + TESTERR(dst_folder1_fd = s_open(s_path(s(mount_dir), s(folder1)), + O_RDONLY | O_CLOEXEC), dst_folder1_fd != -1); + TESTSYSCALL(close(dst_folder1_fd)); + dst_folder1_fd = -1; + // Step 2: Lookup folder1 again + TESTERR(dst_folder1_fd = s_open(s_path(s(mount_dir), s(folder1)), + O_RDONLY | O_CLOEXEC), dst_folder1_fd != -1); + TESTSYSCALL(close(dst_folder1_fd)); + dst_folder1_fd = -1; + // Check 1: Read content file (must be backed) + TESTERR(dst_content_fd = + s_open(s_pathn(3, s(mount_dir), s(folder1), s(content_file)), + O_RDONLY | O_CLOEXEC), dst_content_fd != -1); + TEST(dst_content_read_size = + read(dst_content_fd, content_buffer, strlen(content)), + dst_content_read_size == strlen(content) && + strcmp(content, content_buffer) == 0); + TESTSYSCALL(close(dst_content_fd)); + dst_content_fd = -1; + FUSE_DAEMON + struct fuse_attr attr = {}; + int backing_fd = -1; + TESTFUSEINITFLAGS(FUSE_DO_READDIRPLUS | FUSE_READDIRPLUS_AUTO); + // Step 1: Lookup folder1 set entry_timeout to 0 to trigger + // revalidate later + TESTFUSELOOKUP(folder1, 0); + TESTSYSCALL(s_fuse_attr(s_path(s(ft_src), s(folder1)), &attr)); + TEST(backing_fd = s_open(s_path(s(ft_src), s(folder1)), + O_DIRECTORY | O_RDONLY | O_CLOEXEC), + backing_fd != -1); + TESTFUSEOUT2(fuse_entry_out, ((struct fuse_entry_out) { + .nodeid = attr.ino, + .generation = 0, + .entry_valid = 0, + .attr_valid = UINT64_MAX, + .entry_valid_nsec = 0, + .attr_valid_nsec = UINT32_MAX, + .attr = attr, + }), fuse_entry_bpf_out, ((struct fuse_entry_bpf_out) { + .backing_action = FUSE_ACTION_REPLACE, + .backing_fd = backing_fd, + })); + TESTSYSCALL(close(backing_fd)); + // Step 1: Lookup folder1 as a reaction to revalidate call + TESTFUSELOOKUP(folder1, 0); + TESTSYSCALL(s_fuse_attr(s_path(s(ft_src), s(folder1)), &attr)); + TEST(backing_fd = s_open(s_path(s(ft_src), s(folder1)), + O_DIRECTORY | O_RDONLY | O_CLOEXEC), + backing_fd != -1); + TESTFUSEOUT2(fuse_entry_out, ((struct fuse_entry_out) { + .nodeid = attr.ino, + .generation = 0, + .entry_valid = UINT64_MAX, + .attr_valid = UINT64_MAX, + .entry_valid_nsec = UINT32_MAX, + .attr_valid_nsec = UINT32_MAX, + .attr = attr, + }), fuse_entry_bpf_out, ((struct fuse_entry_bpf_out) { + .backing_action = FUSE_ACTION_REPLACE, + .backing_fd = backing_fd, + })); + TESTSYSCALL(close(backing_fd)); + FUSE_DONE + result = TEST_SUCCESS; +out: + close(fuse_dev); + close(content_fd); + close(src_fd); + umount(mount_dir); + return result; +} + static int parse_options(int argc, char *const *argv) { signed char c; @@ -1884,7 +1995,8 @@ int main(int argc, char *argv[]) MAKE_TEST(bpf_test_statfs), MAKE_TEST(bpf_test_lseek), MAKE_TEST(bpf_test_readdirplus_not_overriding_backing), - MAKE_TEST(bpf_test_no_readdirplus_without_nodeid) + MAKE_TEST(bpf_test_no_readdirplus_without_nodeid), + MAKE_TEST(bpf_test_revalidate_handle_backing_fd) }; #undef MAKE_TEST