| Message ID | 20200505095915.11275-6-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs patch queue | expand |
Needs a Cc to linux-api and linux-man. On Tue, May 05, 2020 at 11:59:08AM +0200, Miklos Szeredi wrote: > This allows xattr ops on symlink/special files referenced by an O_PATH > descriptor without having to play games with /proc/self/fd/NN (which > doesn't work for symlinks anyway). Do we even intent to support xattrs on say links? They never wire up ->listxattr and would only get them through s_xattr. I'm defintively worried that this could break things without a very careful audit.
On Wed, May 13, 2020 at 12:04 PM Christoph Hellwig <hch@infradead.org> wrote: > > Needs a Cc to linux-api and linux-man. > > On Tue, May 05, 2020 at 11:59:08AM +0200, Miklos Szeredi wrote: > > This allows xattr ops on symlink/special files referenced by an O_PATH > > descriptor without having to play games with /proc/self/fd/NN (which > > doesn't work for symlinks anyway). > > Do we even intent to support xattrs on say links? They never wire up > ->listxattr and would only get them through s_xattr. I'm defintively > worried that this could break things without a very careful audit. Why do you think listxattr is not wired up for symlinks? Xfs and ext4 definitely do have it, and it seems most others too: $ git grep -A10 "struct inode_operations.*symlink" | grep listxattr | wc -l 29 Thanks, Miklos
On Thu, May 14, 2020 at 10:02 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Wed, May 13, 2020 at 12:04 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > Needs a Cc to linux-api and linux-man. > > > > On Tue, May 05, 2020 at 11:59:08AM +0200, Miklos Szeredi wrote: > > > This allows xattr ops on symlink/special files referenced by an O_PATH > > > descriptor without having to play games with /proc/self/fd/NN (which > > > doesn't work for symlinks anyway). > > > > Do we even intent to support xattrs on say links? They never wire up > > ->listxattr and would only get them through s_xattr. I'm defintively > > worried that this could break things without a very careful audit. > > Why do you think listxattr is not wired up for symlinks? > > Xfs and ext4 definitely do have it, and it seems most others too: > > $ git grep -A10 "struct inode_operations.*symlink" | grep listxattr | wc -l > 29 In any case, I'm dropping this patch for now. The comment about /proc/self/fd/NN not working is actually wrong; it does work despite the target being a symlink: LOOKUP_FOLLOW only follows the magic symlink in this case, not the symlink that is the target. So it's possible to get (set, remove, list) the xattr on an O_PATH descriptor using sprintf("/proc/self/fd/%i", procpath, sizeof(procpath)); getxattr(procpath, ...); Thanks, Miklos
diff --git a/fs/xattr.c b/fs/xattr.c index e13265e65871..7080bb4f3f14 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -495,7 +495,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname, SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name, const void __user *,value, size_t, size, int, flags) { - struct fd f = fdget(fd); + struct fd f = fdget_raw(fd); int error = -EBADF; if (!f.file) @@ -587,7 +587,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname, SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name, void __user *, value, size_t, size) { - struct fd f = fdget(fd); + struct fd f = fdget_raw(fd); ssize_t error = -EBADF; if (!f.file) @@ -662,7 +662,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list, SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size) { - struct fd f = fdget(fd); + struct fd f = fdget_raw(fd); ssize_t error = -EBADF; if (!f.file) @@ -727,7 +727,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname, SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name) { - struct fd f = fdget(fd); + struct fd f = fdget_raw(fd); int error = -EBADF; if (!f.file)
This allows xattr ops on symlink/special files referenced by an O_PATH descriptor without having to play games with /proc/self/fd/NN (which doesn't work for symlinks anyway). This capability is the same as would be given by introducing ...at() variants with an AT_EMPTY_PATH argument. Looking at getattr/setattr type syscalls, this is allowed for fstatat() and fchownat(), but not for fchmodat() and utimensat(). What's the logic? While this carries a minute risk of someone relying on the property of xattr syscalls rejecting O_PATH descriptors, it saves the trouble of introducing another set of syscalls. Only file->f_path and file->f_inode are accessed in these functions. Current versions return EBADF, hence easy to detect the presense of this feature and fall back in case it's missing. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/xattr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)