| Message ID | 20230920173445.3943581-3-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: full atomic open and atomic-open-revalidate | expand |
On 10/2/23 03:57, Yuan Yao wrote: > Hi, I’m Yuan, particularly interested in this patch set, and I've > noticed some ambiguity regarding the behavior of atomic_open for > symbolic links. > > > I think this part may cause a problem if we atomic_open a symbolic > link.The previous behavior for fuse_create_open() will only do lookup > but not open the symbolic link. But, with the full atomic open kernel > patch. My understanding is that when the kernel performs an atomic_open > operation on a symbolic link, the dentry returned from the FUSE server > contains the inode pointing to the opened symbolic link. However, after > atomic_open() is called, the may_open() function in namei.c checks the > node's i_mode and identifies it as a symbolic link, resulting in an > ELOOP error. Thanks for testing it, I didn't manage to look into it yet, but will for sure do during the next days (I hope tomorrow). > > > My concernn is: what is the expected behavior for opening a symbolic > link, both on the kernel side and the server side? Is it possible for > the fuse server to return the dentry containing the inode of the link > destination instead of the inode of the symbolic link itself? nfs_atomic_open handles -ELOOP. Possibly fuse server needs to check for O_NOFOLLOW, but I'm not sure. Will look into that in the morning. Thanks, Bernd
Sorry for confusing, I’m resending the mail as the previous mail had a formatting issue(not plain text) and was not sent to linux-fsdevel. Hi, I’m Yuan, particularly interested in this patch set, and I've noticed some ambiguity regarding the behavior of atomic_open for symbolic links. I think this part may cause a problem if we atomic_open a symbolic link.The previous behavior for fuse_create_open() will only do lookup but not open the symbolic link. But, with the full atomic open kernel patch. My understanding is that when the kernel performs an atomic_open operation on a symbolic link, the dentry returned from the FUSE server contains the inode pointing to the opened symbolic link. However, after atomic_open() is called, the may_open() function in namei.c checks the node's i_mode and identifies it as a symbolic link, resulting in an ELOOP error. My concernn is: what is the expected behavior for opening a symbolic link, both on the kernel side and the server side? Is it possible for the fuse server to return the dentry containing the inode of the link destination instead of the inode of the symbolic link itself? > On Sep 21, 2023, at 2:34, Bernd Schubert <bschubert@ddn.com> wrote: > > From: Dharmendra Singh <dsingh@ddn.com> > > This adds full atomic open support, to avoid lookup before open/create. > If the implementation (fuse server/daemon) does not support atomic open > it falls back to non-atomic open. > > Co-developed-by: Bernd Schubert <bschubert@ddn.com> > Signed-off-by: Dharmendra Singh <dsingh@ddn.com> > Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com> > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Dharmendra Singh <dsingh@ddn.com> > Cc: linux-fsdevel@vger.kernel.org > --- > fs/fuse/dir.c | 214 +++++++++++++++++++++++++++++++++++++- > fs/fuse/fuse_i.h | 3 + > include/uapi/linux/fuse.h | 3 + > 3 files changed, 219 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 542086140781..4cb2809a852d 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -722,7 +722,7 @@ static int _fuse_create_open(struct inode *dir, struct dentry *entry, > > static int fuse_mknod(struct mnt_idmap *, struct inode *, struct dentry *, > umode_t, dev_t); > -static int fuse_atomic_open(struct inode *dir, struct dentry *entry, > +static int fuse_create_open(struct inode *dir, struct dentry *entry, > struct file *file, unsigned flags, > umode_t mode) > { > @@ -768,6 +768,218 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry, > return finish_no_open(file, res); > } > > +static int _fuse_atomic_open(struct inode *dir, struct dentry *entry, > + struct file *file, unsigned flags, > + umode_t mode) > +{ > + int err; > + struct inode *inode; > + struct fuse_mount *fm = get_fuse_mount(dir); > + struct fuse_conn *fc = fm->fc; > + FUSE_ARGS(args); > + struct fuse_forget_link *forget; > + struct fuse_create_in inarg; > + struct fuse_open_out outopen; > + struct fuse_entry_out outentry; > + struct fuse_inode *fi; > + struct fuse_file *ff; > + struct dentry *switched_entry = NULL, *alias = NULL; > + DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); > + > + /* Expect a negative dentry */ > + if (unlikely(d_inode(entry))) > + goto fallback; > + > + /* Userspace expects S_IFREG in create mode */ > + if ((flags & O_CREAT) && (mode & S_IFMT) != S_IFREG) > + goto fallback; > + > + forget = fuse_alloc_forget(); > + err = -ENOMEM; > + if (!forget) > + goto out_err; > + > + err = -ENOMEM; > + ff = fuse_file_alloc(fm); > + if (!ff) > + goto out_put_forget_req; > + > + if (!fc->dont_mask) > + mode &= ~current_umask(); > + > + flags &= ~O_NOCTTY; > + memset(&inarg, 0, sizeof(inarg)); > + memset(&outentry, 0, sizeof(outentry)); > + inarg.flags = flags; > + inarg.mode = mode; > + inarg.umask = current_umask(); > + > + if (fc->handle_killpriv_v2 && (flags & O_TRUNC) && > + !(flags & O_EXCL) && !capable(CAP_FSETID)) { > + inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID; > + } > + > + args.opcode = FUSE_OPEN_ATOMIC; > + args.nodeid = get_node_id(dir); > + args.in_numargs = 2; > + args.in_args[0].size = sizeof(inarg); > + args.in_args[0].value = &inarg; > + args.in_args[1].size = entry->d_name.len + 1; > + args.in_args[1].value = entry->d_name.name; > + args.out_numargs = 2; > + args.out_args[0].size = sizeof(outentry); > + args.out_args[0].value = &outentry; > + args.out_args[1].size = sizeof(outopen); > + args.out_args[1].value = &outopen; > + > + if (flags & O_CREAT) { > + err = get_create_ext(&args, dir, entry, mode); > + if (err) > + goto out_free_ff; > + } > + > + err = fuse_simple_request(fm, &args); > + free_ext_value(&args); > + if (err == -ENOSYS) { > + fc->no_open_atomic = 1; > + fuse_file_free(ff); > + kfree(forget); > + goto fallback; > + } > + > + if (!err && !outentry.nodeid) > + err = -ENOENT; > + > + if (err) > + goto out_free_ff; > + > + err = -EIO; > + if (invalid_nodeid(outentry.nodeid) || fuse_invalid_attr(&outentry.attr)) > + goto out_free_ff; > + > + ff->fh = outopen.fh; > + ff->nodeid = outentry.nodeid; > + ff->open_flags = outopen.open_flags; > + inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation, > + &outentry.attr, entry_attr_timeout(&outentry), 0); > + if (!inode) { > + flags &= ~(O_CREAT | O_EXCL | O_TRUNC); > + fuse_sync_release(NULL, ff, flags); > + fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1); > + err = -ENOMEM; > + goto out_err; > + } > + > + /* prevent racing/parallel lookup on a negative hashed */ > + if (!(flags & O_CREAT) && !d_in_lookup(entry)) { > + d_drop(entry); > + switched_entry = d_alloc_parallel(entry->d_parent, > + &entry->d_name, &wq); > + if (IS_ERR(switched_entry)) { > + err = PTR_ERR(switched_entry); > + goto out_free_ff; > + } > + > + if (unlikely(!d_in_lookup(switched_entry))) { > + /* fall back */ > + dput(switched_entry); > + switched_entry = NULL; > + goto free_and_fallback; > + } > + > + entry = switched_entry; > + } > + > + if (d_really_is_negative(entry)) { > + d_drop(entry); > + alias = d_exact_alias(entry, inode); > + if (!alias) { > + alias = d_splice_alias(inode, entry); > + if (IS_ERR(alias)) { > + /* > + * Close the file in user space, but do not unlink it, > + * if it was created - with network file systems other > + * clients might have already accessed it. > + */ > + fi = get_fuse_inode(inode); > + fuse_sync_release(fi, ff, flags); > + fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1); > + err = PTR_ERR(alias); > + goto out_err; > + } > + } > + > + if (alias) > + entry = alias; > + } > + > + fuse_change_entry_timeout(entry, &outentry); > + > + /* File was indeed created */ > + if (outopen.open_flags & FOPEN_FILE_CREATED) { > + if (!(flags & O_CREAT)) { > + pr_debug("Server side bug, FOPEN_FILE_CREATED set " > + "without O_CREAT, ignoring."); > + } else { > + /* This should be always set when the file is created */ > + fuse_dir_changed(dir); > + file->f_mode |= FMODE_CREATED; > + } > + } > + > + if (S_ISDIR(mode)) > + ff->open_flags &= ~FOPEN_DIRECT_IO; > + err = finish_open(file, entry, generic_file_open); > + if (err) { > + fi = get_fuse_inode(inode); > + fuse_sync_release(fi, ff, flags); > + } else { > + file->private_data = ff; > + fuse_finish_open(inode, file); > + } > + > + kfree(forget); > + > + if (switched_entry) { > + d_lookup_done(switched_entry); > + dput(switched_entry); > + } > + > + dput(alias); > + > + return err; > + > +out_free_ff: > + fuse_file_free(ff); > +out_put_forget_req: > + kfree(forget); > +out_err: > + if (switched_entry) { > + d_lookup_done(switched_entry); > + dput(switched_entry); > + } > + > + return err; > + > +free_and_fallback: > + fuse_file_free(ff); > + kfree(forget); > +fallback: > + return fuse_create_open(dir, entry, file, flags, mode); > +} > + > +static int fuse_atomic_open(struct inode *dir, struct dentry *entry, > + struct file *file, unsigned int flags, > + umode_t mode) > +{ > + struct fuse_conn *fc = get_fuse_conn(dir); > + > + if (fc->no_open_atomic) > + return fuse_create_open(dir, entry, file, flags, mode); > + else > + return _fuse_atomic_open(dir, entry, file, flags, mode); > +} > + > /* > * Code shared between mknod, mkdir, symlink and link > */ > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 9b7fc7d3c7f1..c838708cfa2b 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -672,6 +672,9 @@ struct fuse_conn { > /** Is open/release not implemented by fs? */ > unsigned no_open:1; > > + /** Is open atomic not implemented by fs? */ > + unsigned no_open_atomic:1; > + > /** Is opendir/releasedir not implemented by fs? */ > unsigned no_opendir:1; > > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > index b3fcab13fcd3..33fefee42697 100644 > --- a/include/uapi/linux/fuse.h > +++ b/include/uapi/linux/fuse.h > @@ -315,6 +315,7 @@ struct fuse_file_lock { > * FOPEN_STREAM: the file is stream-like (no file position at all) > * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE) > * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on the same inode > + * FOPEN_FILE_CREATED: the file was indeed created > */ > #define FOPEN_DIRECT_IO (1 << 0) > #define FOPEN_KEEP_CACHE (1 << 1) > @@ -323,6 +324,7 @@ struct fuse_file_lock { > #define FOPEN_STREAM (1 << 4) > #define FOPEN_NOFLUSH (1 << 5) > #define FOPEN_PARALLEL_DIRECT_WRITES (1 << 6) > +#define FOPEN_FILE_CREATED (1 << 7) > > /** > * INIT request/reply flags > @@ -575,6 +577,7 @@ enum fuse_opcode { > FUSE_REMOVEMAPPING = 49, > FUSE_SYNCFS = 50, > FUSE_TMPFILE = 51, > + FUSE_OPEN_ATOMIC = 52, > > /* CUSE specific operations */ > CUSE_INIT = 4096, > -- > 2.39.2 > >
Hi Yuan, >> >> My concernn is: what is the expected behavior for opening a symbolic >> link, both on the kernel side and the server side? Is it possible for >> the fuse server to return the dentry containing the inode of the link >> destination instead of the inode of the symbolic link itself? > > nfs_atomic_open handles -ELOOP. Possibly fuse server needs to check for > O_NOFOLLOW, but I'm not sure. Will look into that in the morning. > thanks again for testing atomic open and reporting the symlink issue. And sorry for the delay. Actually interesting that it was not caught by xfstests (or I somehow didn't notice) Horst (in CC) is currently adding fuse specific tests - we will catch it in the future. Will also check for existing symlink tests - this is actually not fuse specific... I just pushed an initial fix to https://github.com/bsbernd/linux/tree/atomic-open-for-6.5-v10 https://github.com/libfuse/libfuse/pull/813 I'm going to add further changes for symlinks, though. Right now atomic open just reports -ELOOP and then falls back to fuse_create_open, which sends a lookup - atomic_open now adds in overhead - the opposite of what I want. I had tried to return the attributes and the error for atomic_open, but that is not accepted in fuse_dev_do_write(). It would be possible to hack that function with checks for FUSE_OPEN_ATOMIC, but it would be rather hackish. So we need to extend fuse_entry_out with the error code, or add a new struct. Thanks, Bernd
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 542086140781..4cb2809a852d 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -722,7 +722,7 @@ static int _fuse_create_open(struct inode *dir, struct dentry *entry, static int fuse_mknod(struct mnt_idmap *, struct inode *, struct dentry *, umode_t, dev_t); -static int fuse_atomic_open(struct inode *dir, struct dentry *entry, +static int fuse_create_open(struct inode *dir, struct dentry *entry, struct file *file, unsigned flags, umode_t mode) { @@ -768,6 +768,218 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry, return finish_no_open(file, res); } +static int _fuse_atomic_open(struct inode *dir, struct dentry *entry, + struct file *file, unsigned flags, + umode_t mode) +{ + int err; + struct inode *inode; + struct fuse_mount *fm = get_fuse_mount(dir); + struct fuse_conn *fc = fm->fc; + FUSE_ARGS(args); + struct fuse_forget_link *forget; + struct fuse_create_in inarg; + struct fuse_open_out outopen; + struct fuse_entry_out outentry; + struct fuse_inode *fi; + struct fuse_file *ff; + struct dentry *switched_entry = NULL, *alias = NULL; + DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); + + /* Expect a negative dentry */ + if (unlikely(d_inode(entry))) + goto fallback; + + /* Userspace expects S_IFREG in create mode */ + if ((flags & O_CREAT) && (mode & S_IFMT) != S_IFREG) + goto fallback; + + forget = fuse_alloc_forget(); + err = -ENOMEM; + if (!forget) + goto out_err; + + err = -ENOMEM; + ff = fuse_file_alloc(fm); + if (!ff) + goto out_put_forget_req; + + if (!fc->dont_mask) + mode &= ~current_umask(); + + flags &= ~O_NOCTTY; + memset(&inarg, 0, sizeof(inarg)); + memset(&outentry, 0, sizeof(outentry)); + inarg.flags = flags; + inarg.mode = mode; + inarg.umask = current_umask(); + + if (fc->handle_killpriv_v2 && (flags & O_TRUNC) && + !(flags & O_EXCL) && !capable(CAP_FSETID)) { + inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID; + } + + args.opcode = FUSE_OPEN_ATOMIC; + args.nodeid = get_node_id(dir); + args.in_numargs = 2; + args.in_args[0].size = sizeof(inarg); + args.in_args[0].value = &inarg; + args.in_args[1].size = entry->d_name.len + 1; + args.in_args[1].value = entry->d_name.name; + args.out_numargs = 2; + args.out_args[0].size = sizeof(outentry); + args.out_args[0].value = &outentry; + args.out_args[1].size = sizeof(outopen); + args.out_args[1].value = &outopen; + + if (flags & O_CREAT) { + err = get_create_ext(&args, dir, entry, mode); + if (err) + goto out_free_ff; + } + + err = fuse_simple_request(fm, &args); + free_ext_value(&args); + if (err == -ENOSYS) { + fc->no_open_atomic = 1; + fuse_file_free(ff); + kfree(forget); + goto fallback; + } + + if (!err && !outentry.nodeid) + err = -ENOENT; + + if (err) + goto out_free_ff; + + err = -EIO; + if (invalid_nodeid(outentry.nodeid) || fuse_invalid_attr(&outentry.attr)) + goto out_free_ff; + + ff->fh = outopen.fh; + ff->nodeid = outentry.nodeid; + ff->open_flags = outopen.open_flags; + inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation, + &outentry.attr, entry_attr_timeout(&outentry), 0); + if (!inode) { + flags &= ~(O_CREAT | O_EXCL | O_TRUNC); + fuse_sync_release(NULL, ff, flags); + fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1); + err = -ENOMEM; + goto out_err; + } + + /* prevent racing/parallel lookup on a negative hashed */ + if (!(flags & O_CREAT) && !d_in_lookup(entry)) { + d_drop(entry); + switched_entry = d_alloc_parallel(entry->d_parent, + &entry->d_name, &wq); + if (IS_ERR(switched_entry)) { + err = PTR_ERR(switched_entry); + goto out_free_ff; + } + + if (unlikely(!d_in_lookup(switched_entry))) { + /* fall back */ + dput(switched_entry); + switched_entry = NULL; + goto free_and_fallback; + } + + entry = switched_entry; + } + + if (d_really_is_negative(entry)) { + d_drop(entry); + alias = d_exact_alias(entry, inode); + if (!alias) { + alias = d_splice_alias(inode, entry); + if (IS_ERR(alias)) { + /* + * Close the file in user space, but do not unlink it, + * if it was created - with network file systems other + * clients might have already accessed it. + */ + fi = get_fuse_inode(inode); + fuse_sync_release(fi, ff, flags); + fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1); + err = PTR_ERR(alias); + goto out_err; + } + } + + if (alias) + entry = alias; + } + + fuse_change_entry_timeout(entry, &outentry); + + /* File was indeed created */ + if (outopen.open_flags & FOPEN_FILE_CREATED) { + if (!(flags & O_CREAT)) { + pr_debug("Server side bug, FOPEN_FILE_CREATED set " + "without O_CREAT, ignoring."); + } else { + /* This should be always set when the file is created */ + fuse_dir_changed(dir); + file->f_mode |= FMODE_CREATED; + } + } + + if (S_ISDIR(mode)) + ff->open_flags &= ~FOPEN_DIRECT_IO; + err = finish_open(file, entry, generic_file_open); + if (err) { + fi = get_fuse_inode(inode); + fuse_sync_release(fi, ff, flags); + } else { + file->private_data = ff; + fuse_finish_open(inode, file); + } + + kfree(forget); + + if (switched_entry) { + d_lookup_done(switched_entry); + dput(switched_entry); + } + + dput(alias); + + return err; + +out_free_ff: + fuse_file_free(ff); +out_put_forget_req: + kfree(forget); +out_err: + if (switched_entry) { + d_lookup_done(switched_entry); + dput(switched_entry); + } + + return err; + +free_and_fallback: + fuse_file_free(ff); + kfree(forget); +fallback: + return fuse_create_open(dir, entry, file, flags, mode); +} + +static int fuse_atomic_open(struct inode *dir, struct dentry *entry, + struct file *file, unsigned int flags, + umode_t mode) +{ + struct fuse_conn *fc = get_fuse_conn(dir); + + if (fc->no_open_atomic) + return fuse_create_open(dir, entry, file, flags, mode); + else + return _fuse_atomic_open(dir, entry, file, flags, mode); +} + /* * Code shared between mknod, mkdir, symlink and link */ diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 9b7fc7d3c7f1..c838708cfa2b 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -672,6 +672,9 @@ struct fuse_conn { /** Is open/release not implemented by fs? */ unsigned no_open:1; + /** Is open atomic not implemented by fs? */ + unsigned no_open_atomic:1; + /** Is opendir/releasedir not implemented by fs? */ unsigned no_opendir:1; diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index b3fcab13fcd3..33fefee42697 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -315,6 +315,7 @@ struct fuse_file_lock { * FOPEN_STREAM: the file is stream-like (no file position at all) * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE) * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on the same inode + * FOPEN_FILE_CREATED: the file was indeed created */ #define FOPEN_DIRECT_IO (1 << 0) #define FOPEN_KEEP_CACHE (1 << 1) @@ -323,6 +324,7 @@ struct fuse_file_lock { #define FOPEN_STREAM (1 << 4) #define FOPEN_NOFLUSH (1 << 5) #define FOPEN_PARALLEL_DIRECT_WRITES (1 << 6) +#define FOPEN_FILE_CREATED (1 << 7) /** * INIT request/reply flags @@ -575,6 +577,7 @@ enum fuse_opcode { FUSE_REMOVEMAPPING = 49, FUSE_SYNCFS = 50, FUSE_TMPFILE = 51, + FUSE_OPEN_ATOMIC = 52, /* CUSE specific operations */ CUSE_INIT = 4096,