| Message ID | 20210602151553.30090-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change quotactl_path() to an fd-based syscall | expand |
Hello Honza, On Wed, Jun 02, 2021 at 05:15:52PM +0200, Jan Kara wrote: > Some users have pointed out that path-based syscalls are problematic in > some environments and at least directory fd argument and possibly also > resolve flags are desirable for such syscalls. Rather than > reimplementing all details of pathname lookup and following where it may > eventually evolve, let's go for full file descriptor based syscall > similar to how ioctl(2) works since the beginning. Managing of quotas > isn't performance sensitive so the extra overhead of open does not > matter and we are able to consume O_PATH descriptors as well which makes > open cheap anyway. Also for frequent operations (such as retrieving > usage information for all users) we can reuse single fd and in fact get > even better performance as well as avoiding races with possible remounts > etc. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/quota/quota.c | 27 ++++++++++++--------------- > include/linux/syscalls.h | 4 ++-- > include/uapi/asm-generic/unistd.h | 4 ++-- > kernel/sys_ni.c | 2 +- > 4 files changed, 17 insertions(+), 20 deletions(-) Thanks for taking care of this. I also gave this some testing and it's working ok for me, so at least I can give you my: Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Sascha
On Thu 03-06-21 11:41:02, Sascha Hauer wrote: > Hello Honza, > > On Wed, Jun 02, 2021 at 05:15:52PM +0200, Jan Kara wrote: > > Some users have pointed out that path-based syscalls are problematic in > > some environments and at least directory fd argument and possibly also > > resolve flags are desirable for such syscalls. Rather than > > reimplementing all details of pathname lookup and following where it may > > eventually evolve, let's go for full file descriptor based syscall > > similar to how ioctl(2) works since the beginning. Managing of quotas > > isn't performance sensitive so the extra overhead of open does not > > matter and we are able to consume O_PATH descriptors as well which makes > > open cheap anyway. Also for frequent operations (such as retrieving > > usage information for all users) we can reuse single fd and in fact get > > even better performance as well as avoiding races with possible remounts > > etc. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/quota/quota.c | 27 ++++++++++++--------------- > > include/linux/syscalls.h | 4 ++-- > > include/uapi/asm-generic/unistd.h | 4 ++-- > > kernel/sys_ni.c | 2 +- > > 4 files changed, 17 insertions(+), 20 deletions(-) > > Thanks for taking care of this. > > I also gave this some testing and it's working ok for me, so at least I > can give you my: > > Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Thanks! Honza
On Wed, Jun 02, 2021 at 05:15:52PM +0200, Jan Kara wrote: > Some users have pointed out that path-based syscalls are problematic in > some environments and at least directory fd argument and possibly also > resolve flags are desirable for such syscalls. Rather than > reimplementing all details of pathname lookup and following where it may > eventually evolve, let's go for full file descriptor based syscall Fair, I can accept that. > similar to how ioctl(2) works since the beginning. Managing of quotas > isn't performance sensitive so the extra overhead of open does not > matter and we are able to consume O_PATH descriptors as well which makes > open cheap anyway. Also for frequent operations (such as retrieving > usage information for all users) we can reuse single fd and in fact get > even better performance as well as avoiding races with possible remounts > etc. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/quota/quota.c | 27 ++++++++++++--------------- > include/linux/syscalls.h | 4 ++-- > include/uapi/asm-generic/unistd.h | 4 ++-- > kernel/sys_ni.c | 2 +- > 4 files changed, 17 insertions(+), 20 deletions(-) > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index 05e4bd9ab6d6..8450bb6186f4 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -968,31 +968,29 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, > return ret; > } > > -SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, > - mountpoint, qid_t, id, void __user *, addr) > +SYSCALL_DEFINE4(quotactl_fd, unsigned int, fd, unsigned int, cmd, > + qid_t, id, void __user *, addr) > { > struct super_block *sb; > - struct path mountpath; > unsigned int cmds = cmd >> SUBCMDSHIFT; > unsigned int type = cmd & SUBCMDMASK; > + struct fd f = fdget_raw(fd); > int ret; > > - if (type >= MAXQUOTAS) > - return -EINVAL; > + if (!f.file) > + return -EBADF; I would maybe change this to f = fdget_raw(fd); if (!f.file) return -EBADF; instead of directly assigning when declaring the variable. (And it might make sense to fold the second commit into this one.) But other than these nits this looks good, Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > > - ret = user_path_at(AT_FDCWD, mountpoint, > - LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > - if (ret) > - return ret; > - > - sb = mountpath.mnt->mnt_sb; > + ret = -EINVAL; > + if (type >= MAXQUOTAS) > + goto out; > > if (quotactl_cmd_write(cmds)) { > - ret = mnt_want_write(mountpath.mnt); > + ret = mnt_want_write(f.file->f_path.mnt); > if (ret) > goto out; > } > > + sb = f.file->f_path.mnt->mnt_sb; > if (quotactl_cmd_onoff(cmds)) > down_write(&sb->s_umount); > else > @@ -1006,9 +1004,8 @@ SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, > up_read(&sb->s_umount); > > if (quotactl_cmd_write(cmds)) > - mnt_drop_write(mountpath.mnt); > + mnt_drop_write(f.file->f_path.mnt); > out: > - path_put(&mountpath); > - > + fdput(f); > return ret; > } > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h > index 050511e8f1f8..586128d5c3b8 100644 > --- a/include/linux/syscalls.h > +++ b/include/linux/syscalls.h > @@ -485,8 +485,8 @@ asmlinkage long sys_pipe2(int __user *fildes, int flags); > /* fs/quota.c */ > asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, > qid_t id, void __user *addr); > -asmlinkage long sys_quotactl_path(unsigned int cmd, const char __user *mountpoint, > - qid_t id, void __user *addr); > +asmlinkage long sys_quotactl_fd(unsigned int fd, unsigned int cmd, qid_t id, > + void __user *addr); > > /* fs/readdir.c */ > asmlinkage long sys_getdents64(unsigned int fd, > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h > index 6de5a7fc066b..f211961ce1da 100644 > --- a/include/uapi/asm-generic/unistd.h > +++ b/include/uapi/asm-generic/unistd.h > @@ -863,8 +863,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise) > __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2) > #define __NR_mount_setattr 442 > __SYSCALL(__NR_mount_setattr, sys_mount_setattr) > -#define __NR_quotactl_path 443 > -__SYSCALL(__NR_quotactl_path, sys_quotactl_path) > +#define __NR_quotactl_fd 443 > +__SYSCALL(__NR_quotactl_fd, sys_quotactl_fd) > > #define __NR_landlock_create_ruleset 444 > __SYSCALL(__NR_landlock_create_ruleset, sys_landlock_create_ruleset) > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > index 0ea8128468c3..dad4d994641e 100644 > --- a/kernel/sys_ni.c > +++ b/kernel/sys_ni.c > @@ -99,7 +99,7 @@ COND_SYSCALL(flock); > > /* fs/quota.c */ > COND_SYSCALL(quotactl); > -COND_SYSCALL(quotactl_path); > +COND_SYSCALL(quotactl_fd); > > /* fs/readdir.c */ > > -- > 2.26.2 >
Hi Jan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on m68k/for-next powerpc/next s390/features linus/master v5.13-rc4 next-20210603]
[cannot apply to tip/x86/asm asm-generic/master hp-parisc/for-next sparc-next/master sparc/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jan-Kara/Change-quotactl_path-to-an-fd-based-syscall/20210602-232203
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm64-randconfig-r002-20210603 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d8e0ae9a76a62bdc6117630d59bf9967ac9bb4ea)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/2f0109eecacfbf0ade367f8c7631ad18e4368ab5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jan-Kara/Change-quotactl_path-to-an-fd-based-syscall/20210602-232203
git checkout 2f0109eecacfbf0ade367f8c7631ad18e4368ab5
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
asmlinkage long __weak __arm64_compat_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:41:1: note: expanded from here
__arm64_compat_sys_epoll_pwait2
^
kernel/sys_ni.c:72:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:41:13: note: expanded from macro 'COND_SYSCALL_COMPAT'
asmlinkage long __weak __arm64_compat_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:77:1: warning: no previous prototype for function '__arm64_sys_inotify_init1' [-Wmissing-prototypes]
COND_SYSCALL(inotify_init1);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:42:1: note: expanded from here
__arm64_sys_inotify_init1
^
kernel/sys_ni.c:77:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:78:1: warning: no previous prototype for function '__arm64_sys_inotify_add_watch' [-Wmissing-prototypes]
COND_SYSCALL(inotify_add_watch);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:43:1: note: expanded from here
__arm64_sys_inotify_add_watch
^
kernel/sys_ni.c:78:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:79:1: warning: no previous prototype for function '__arm64_sys_inotify_rm_watch' [-Wmissing-prototypes]
COND_SYSCALL(inotify_rm_watch);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:44:1: note: expanded from here
__arm64_sys_inotify_rm_watch
^
kernel/sys_ni.c:79:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:84:1: warning: no previous prototype for function '__arm64_sys_ioprio_set' [-Wmissing-prototypes]
COND_SYSCALL(ioprio_set);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:45:1: note: expanded from here
__arm64_sys_ioprio_set
^
kernel/sys_ni.c:84:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:85:1: warning: no previous prototype for function '__arm64_sys_ioprio_get' [-Wmissing-prototypes]
COND_SYSCALL(ioprio_get);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:46:1: note: expanded from here
__arm64_sys_ioprio_get
^
kernel/sys_ni.c:85:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:88:1: warning: no previous prototype for function '__arm64_sys_flock' [-Wmissing-prototypes]
COND_SYSCALL(flock);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:47:1: note: expanded from here
__arm64_sys_flock
^
kernel/sys_ni.c:88:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:101:1: warning: no previous prototype for function '__arm64_sys_quotactl' [-Wmissing-prototypes]
COND_SYSCALL(quotactl);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:48:1: note: expanded from here
__arm64_sys_quotactl
^
kernel/sys_ni.c:101:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
>> kernel/sys_ni.c:102:1: warning: no previous prototype for function '__arm64_sys_quotactl_fd' [-Wmissing-prototypes]
COND_SYSCALL(quotactl_fd);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:49:1: note: expanded from here
__arm64_sys_quotactl_fd
^
kernel/sys_ni.c:102:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:113:1: warning: no previous prototype for function '__arm64_sys_signalfd4' [-Wmissing-prototypes]
COND_SYSCALL(signalfd4);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:50:1: note: expanded from here
__arm64_sys_signalfd4
^
kernel/sys_ni.c:113:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:114:1: warning: no previous prototype for function '__arm64_compat_sys_signalfd4' [-Wmissing-prototypes]
COND_SYSCALL_COMPAT(signalfd4);
^
arch/arm64/include/asm/syscall_wrapper.h:41:25: note: expanded from macro 'COND_SYSCALL_COMPAT'
asmlinkage long __weak __arm64_compat_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:51:1: note: expanded from here
__arm64_compat_sys_signalfd4
^
kernel/sys_ni.c:114:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:41:13: note: expanded from macro 'COND_SYSCALL_COMPAT'
asmlinkage long __weak __arm64_compat_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:123:1: warning: no previous prototype for function '__arm64_sys_timerfd_create' [-Wmissing-prototypes]
COND_SYSCALL(timerfd_create);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:52:1: note: expanded from here
__arm64_sys_timerfd_create
^
kernel/sys_ni.c:123:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:124:1: warning: no previous prototype for function '__arm64_sys_timerfd_settime' [-Wmissing-prototypes]
COND_SYSCALL(timerfd_settime);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:53:1: note: expanded from here
__arm64_sys_timerfd_settime
^
kernel/sys_ni.c:124:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:125:1: warning: no previous prototype for function '__arm64_sys_timerfd_settime32' [-Wmissing-prototypes]
COND_SYSCALL(timerfd_settime32);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:54:1: note: expanded from here
__arm64_sys_timerfd_settime32
^
kernel/sys_ni.c:125:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:126:1: warning: no previous prototype for function '__arm64_sys_timerfd_gettime' [-Wmissing-prototypes]
COND_SYSCALL(timerfd_gettime);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:55:1: note: expanded from here
__arm64_sys_timerfd_gettime
^
kernel/sys_ni.c:126:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
arch/arm64/include/asm/syscall_wrapper.h:76:13: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
kernel/sys_ni.c:127:1: warning: no previous prototype for function '__arm64_sys_timerfd_gettime32' [-Wmissing-prototypes]
COND_SYSCALL(timerfd_gettime32);
^
arch/arm64/include/asm/syscall_wrapper.h:76:25: note: expanded from macro 'COND_SYSCALL'
asmlinkage long __weak __arm64_sys_##name(const struct pt_regs *regs) \
^
<scratch space>:56:1: note: expanded from here
__arm64_sys_timerfd_gettime32
^
kernel/sys_ni.c:127:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
vim +/__arm64_sys_quotactl_fd +102 kernel/sys_ni.c
99
100 /* fs/quota.c */
101 COND_SYSCALL(quotactl);
> 102 COND_SYSCALL(quotactl_fd);
103
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu 03-06-21 14:25:04, Christian Brauner wrote: > On Wed, Jun 02, 2021 at 05:15:52PM +0200, Jan Kara wrote: > > Some users have pointed out that path-based syscalls are problematic in > > some environments and at least directory fd argument and possibly also > > resolve flags are desirable for such syscalls. Rather than > > reimplementing all details of pathname lookup and following where it may > > eventually evolve, let's go for full file descriptor based syscall > > Fair, I can accept that. > > > similar to how ioctl(2) works since the beginning. Managing of quotas > > isn't performance sensitive so the extra overhead of open does not > > matter and we are able to consume O_PATH descriptors as well which makes > > open cheap anyway. Also for frequent operations (such as retrieving > > usage information for all users) we can reuse single fd and in fact get > > even better performance as well as avoiding races with possible remounts > > etc. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/quota/quota.c | 27 ++++++++++++--------------- > > include/linux/syscalls.h | 4 ++-- > > include/uapi/asm-generic/unistd.h | 4 ++-- > > kernel/sys_ni.c | 2 +- > > 4 files changed, 17 insertions(+), 20 deletions(-) > > > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > > index 05e4bd9ab6d6..8450bb6186f4 100644 > > --- a/fs/quota/quota.c > > +++ b/fs/quota/quota.c > > @@ -968,31 +968,29 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, > > return ret; > > } > > > > -SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, > > - mountpoint, qid_t, id, void __user *, addr) > > +SYSCALL_DEFINE4(quotactl_fd, unsigned int, fd, unsigned int, cmd, > > + qid_t, id, void __user *, addr) > > { > > struct super_block *sb; > > - struct path mountpath; > > unsigned int cmds = cmd >> SUBCMDSHIFT; > > unsigned int type = cmd & SUBCMDMASK; > > + struct fd f = fdget_raw(fd); > > int ret; > > > > - if (type >= MAXQUOTAS) > > - return -EINVAL; > > + if (!f.file) > > + return -EBADF; > > I would maybe change this to > > f = fdget_raw(fd); > if (!f.file) > return -EBADF; > > instead of directly assigning when declaring the variable. OK, I'll do this. > (And it might make sense to fold the second commit into this one.) Well, I wanted to keep mostly mechanical (and partly autogenerated ;)) changes separately from the "real" stuff that needs serious review... So I prefer to keep the split. > But other than these nits this looks good, > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> Thanks for review! Honza > > > > > - ret = user_path_at(AT_FDCWD, mountpoint, > > - LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > > - if (ret) > > - return ret; > > - > > - sb = mountpath.mnt->mnt_sb; > > + ret = -EINVAL; > > + if (type >= MAXQUOTAS) > > + goto out; > > > > if (quotactl_cmd_write(cmds)) { > > - ret = mnt_want_write(mountpath.mnt); > > + ret = mnt_want_write(f.file->f_path.mnt); > > if (ret) > > goto out; > > } > > > > + sb = f.file->f_path.mnt->mnt_sb; > > if (quotactl_cmd_onoff(cmds)) > > down_write(&sb->s_umount); > > else > > @@ -1006,9 +1004,8 @@ SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, > > up_read(&sb->s_umount); > > > > if (quotactl_cmd_write(cmds)) > > - mnt_drop_write(mountpath.mnt); > > + mnt_drop_write(f.file->f_path.mnt); > > out: > > - path_put(&mountpath); > > - > > + fdput(f); > > return ret; > > } > > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h > > index 050511e8f1f8..586128d5c3b8 100644 > > --- a/include/linux/syscalls.h > > +++ b/include/linux/syscalls.h > > @@ -485,8 +485,8 @@ asmlinkage long sys_pipe2(int __user *fildes, int flags); > > /* fs/quota.c */ > > asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, > > qid_t id, void __user *addr); > > -asmlinkage long sys_quotactl_path(unsigned int cmd, const char __user *mountpoint, > > - qid_t id, void __user *addr); > > +asmlinkage long sys_quotactl_fd(unsigned int fd, unsigned int cmd, qid_t id, > > + void __user *addr); > > > > /* fs/readdir.c */ > > asmlinkage long sys_getdents64(unsigned int fd, > > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h > > index 6de5a7fc066b..f211961ce1da 100644 > > --- a/include/uapi/asm-generic/unistd.h > > +++ b/include/uapi/asm-generic/unistd.h > > @@ -863,8 +863,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise) > > __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2) > > #define __NR_mount_setattr 442 > > __SYSCALL(__NR_mount_setattr, sys_mount_setattr) > > -#define __NR_quotactl_path 443 > > -__SYSCALL(__NR_quotactl_path, sys_quotactl_path) > > +#define __NR_quotactl_fd 443 > > +__SYSCALL(__NR_quotactl_fd, sys_quotactl_fd) > > > > #define __NR_landlock_create_ruleset 444 > > __SYSCALL(__NR_landlock_create_ruleset, sys_landlock_create_ruleset) > > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > > index 0ea8128468c3..dad4d994641e 100644 > > --- a/kernel/sys_ni.c > > +++ b/kernel/sys_ni.c > > @@ -99,7 +99,7 @@ COND_SYSCALL(flock); > > > > /* fs/quota.c */ > > COND_SYSCALL(quotactl); > > -COND_SYSCALL(quotactl_path); > > +COND_SYSCALL(quotactl_fd); > > > > /* fs/readdir.c */ > > > > -- > > 2.26.2 > >
diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 05e4bd9ab6d6..8450bb6186f4 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -968,31 +968,29 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, return ret; } -SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, - mountpoint, qid_t, id, void __user *, addr) +SYSCALL_DEFINE4(quotactl_fd, unsigned int, fd, unsigned int, cmd, + qid_t, id, void __user *, addr) { struct super_block *sb; - struct path mountpath; unsigned int cmds = cmd >> SUBCMDSHIFT; unsigned int type = cmd & SUBCMDMASK; + struct fd f = fdget_raw(fd); int ret; - if (type >= MAXQUOTAS) - return -EINVAL; + if (!f.file) + return -EBADF; - ret = user_path_at(AT_FDCWD, mountpoint, - LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); - if (ret) - return ret; - - sb = mountpath.mnt->mnt_sb; + ret = -EINVAL; + if (type >= MAXQUOTAS) + goto out; if (quotactl_cmd_write(cmds)) { - ret = mnt_want_write(mountpath.mnt); + ret = mnt_want_write(f.file->f_path.mnt); if (ret) goto out; } + sb = f.file->f_path.mnt->mnt_sb; if (quotactl_cmd_onoff(cmds)) down_write(&sb->s_umount); else @@ -1006,9 +1004,8 @@ SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, up_read(&sb->s_umount); if (quotactl_cmd_write(cmds)) - mnt_drop_write(mountpath.mnt); + mnt_drop_write(f.file->f_path.mnt); out: - path_put(&mountpath); - + fdput(f); return ret; } diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index 050511e8f1f8..586128d5c3b8 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -485,8 +485,8 @@ asmlinkage long sys_pipe2(int __user *fildes, int flags); /* fs/quota.c */ asmlinkage long sys_quotactl(unsigned int cmd, const char __user *special, qid_t id, void __user *addr); -asmlinkage long sys_quotactl_path(unsigned int cmd, const char __user *mountpoint, - qid_t id, void __user *addr); +asmlinkage long sys_quotactl_fd(unsigned int fd, unsigned int cmd, qid_t id, + void __user *addr); /* fs/readdir.c */ asmlinkage long sys_getdents64(unsigned int fd, diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h index 6de5a7fc066b..f211961ce1da 100644 --- a/include/uapi/asm-generic/unistd.h +++ b/include/uapi/asm-generic/unistd.h @@ -863,8 +863,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise) __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2) #define __NR_mount_setattr 442 __SYSCALL(__NR_mount_setattr, sys_mount_setattr) -#define __NR_quotactl_path 443 -__SYSCALL(__NR_quotactl_path, sys_quotactl_path) +#define __NR_quotactl_fd 443 +__SYSCALL(__NR_quotactl_fd, sys_quotactl_fd) #define __NR_landlock_create_ruleset 444 __SYSCALL(__NR_landlock_create_ruleset, sys_landlock_create_ruleset) diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c index 0ea8128468c3..dad4d994641e 100644 --- a/kernel/sys_ni.c +++ b/kernel/sys_ni.c @@ -99,7 +99,7 @@ COND_SYSCALL(flock); /* fs/quota.c */ COND_SYSCALL(quotactl); -COND_SYSCALL(quotactl_path); +COND_SYSCALL(quotactl_fd); /* fs/readdir.c */
Some users have pointed out that path-based syscalls are problematic in some environments and at least directory fd argument and possibly also resolve flags are desirable for such syscalls. Rather than reimplementing all details of pathname lookup and following where it may eventually evolve, let's go for full file descriptor based syscall similar to how ioctl(2) works since the beginning. Managing of quotas isn't performance sensitive so the extra overhead of open does not matter and we are able to consume O_PATH descriptors as well which makes open cheap anyway. Also for frequent operations (such as retrieving usage information for all users) we can reuse single fd and in fact get even better performance as well as avoiding races with possible remounts etc. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/quota/quota.c | 27 ++++++++++++--------------- include/linux/syscalls.h | 4 ++-- include/uapi/asm-generic/unistd.h | 4 ++-- kernel/sys_ni.c | 2 +- 4 files changed, 17 insertions(+), 20 deletions(-)