| Message ID | 20210211153024.32502-2-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] quota: Add mountpath based quota support | expand |
> + if (!mountpoint) > + return -ENODEV; > + > + ret = user_path_at(AT_FDCWD, mountpoint, > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); user_path_at handles an empty path, although you'll get EFAULT instead. Do we care about the -ENODEV here? Otherwise this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi Sascha,
I love your patch! Yet something to improve:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on tip/x86/asm m68k/for-next hp-parisc/for-next powerpc/next s390/features linus/master v5.11-rc7 next-20210211]
[cannot apply to 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/Sascha-Hauer/quota-Add-mountpath-based-quota-support/20210211-233912
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: x86_64-randconfig-a012-20210209 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
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 x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/4b7a3df11dd2ca215a6e9b24d81c98d6951476b6
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sascha-Hauer/quota-Add-mountpath-based-quota-support/20210211-233912
git checkout 4b7a3df11dd2ca215a6e9b24d81c98d6951476b6
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> fs/quota/quota.c:995:6: error: implicit declaration of function 'quotactl_cmd_write' [-Werror,-Wimplicit-function-declaration]
if (quotactl_cmd_write(cmds)) {
^
fs/quota/quota.c:995:6: note: did you mean 'quotactl_cmd_onoff'?
fs/quota/quota.c:857:13: note: 'quotactl_cmd_onoff' declared here
static bool quotactl_cmd_onoff(int cmd)
^
1 error generated.
vim +/quotactl_cmd_write +995 fs/quota/quota.c
972
973 SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
974 mountpoint, qid_t, id, void __user *, addr)
975 {
976 struct super_block *sb;
977 struct path mountpath;
978 unsigned int cmds = cmd >> SUBCMDSHIFT;
979 unsigned int type = cmd & SUBCMDMASK;
980 int ret;
981
982 if (type >= MAXQUOTAS)
983 return -EINVAL;
984
985 if (!mountpoint)
986 return -ENODEV;
987
988 ret = user_path_at(AT_FDCWD, mountpoint,
989 LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
990 if (ret)
991 return ret;
992
993 sb = mountpath.dentry->d_inode->i_sb;
994
> 995 if (quotactl_cmd_write(cmds)) {
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Feb 11, 2021 at 03:38:13PM +0000, Christoph Hellwig wrote: > > + if (!mountpoint) > > + return -ENODEV; > > + > > + ret = user_path_at(AT_FDCWD, mountpoint, > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > > user_path_at handles an empty path, although you'll get EFAULT instead. > Do we care about the -ENODEV here? The quotactl manpage documents EFAULT as error code for invalid addr or special argument, so we really should return -EFAULT here. Existing quotactl gets this wrong as well: if (!special) { if (cmds == Q_SYNC) return quota_sync_all(type); return -ENODEV; } Should we fix this or is there userspace code that is confused by a changed return value? Sascha
On Fri 12-02-21 09:38:35, Sascha Hauer wrote: > On Thu, Feb 11, 2021 at 03:38:13PM +0000, Christoph Hellwig wrote: > > > + if (!mountpoint) > > > + return -ENODEV; > > > + > > > + ret = user_path_at(AT_FDCWD, mountpoint, > > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > > > > user_path_at handles an empty path, although you'll get EFAULT instead. > > Do we care about the -ENODEV here? > > The quotactl manpage documents EFAULT as error code for invalid addr or > special argument, so we really should return -EFAULT here. > > Existing quotactl gets this wrong as well: > > if (!special) { > if (cmds == Q_SYNC) > return quota_sync_all(type); > return -ENODEV; > } > > Should we fix this or is there userspace code that is confused by a changed > return value? I'd leave the original quotactl(2) as is. There's no strong reason to risk breaking some userspace. For the new syscall, I agree we can just standardize the return value, there ENODEV makes even less sense since there's no device in that call. Honza
On Fri, Feb 12, 2021 at 11:05:05AM +0100, Jan Kara wrote: > On Fri 12-02-21 09:38:35, Sascha Hauer wrote: > > On Thu, Feb 11, 2021 at 03:38:13PM +0000, Christoph Hellwig wrote: > > > > + if (!mountpoint) > > > > + return -ENODEV; > > > > + > > > > + ret = user_path_at(AT_FDCWD, mountpoint, > > > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > > > > > > user_path_at handles an empty path, although you'll get EFAULT instead. > > > Do we care about the -ENODEV here? > > > > The quotactl manpage documents EFAULT as error code for invalid addr or > > special argument, so we really should return -EFAULT here. > > > > Existing quotactl gets this wrong as well: > > > > if (!special) { > > if (cmds == Q_SYNC) > > return quota_sync_all(type); > > return -ENODEV; > > } > > > > Should we fix this or is there userspace code that is confused by a changed > > return value? > > I'd leave the original quotactl(2) as is. There's no strong reason to risk > breaking some userspace. For the new syscall, I agree we can just > standardize the return value, there ENODEV makes even less sense since > there's no device in that call. Ok, will do. Who can pick this series up? Anyone else I need to Cc next round? Sascha
On Fri 12-02-21 11:29:00, Sascha Hauer wrote: > On Fri, Feb 12, 2021 at 11:05:05AM +0100, Jan Kara wrote: > > On Fri 12-02-21 09:38:35, Sascha Hauer wrote: > > > On Thu, Feb 11, 2021 at 03:38:13PM +0000, Christoph Hellwig wrote: > > > > > + if (!mountpoint) > > > > > + return -ENODEV; > > > > > + > > > > > + ret = user_path_at(AT_FDCWD, mountpoint, > > > > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); > > > > > > > > user_path_at handles an empty path, although you'll get EFAULT instead. > > > > Do we care about the -ENODEV here? > > > > > > The quotactl manpage documents EFAULT as error code for invalid addr or > > > special argument, so we really should return -EFAULT here. > > > > > > Existing quotactl gets this wrong as well: > > > > > > if (!special) { > > > if (cmds == Q_SYNC) > > > return quota_sync_all(type); > > > return -ENODEV; > > > } > > > > > > Should we fix this or is there userspace code that is confused by a changed > > > return value? > > > > I'd leave the original quotactl(2) as is. There's no strong reason to risk > > breaking some userspace. For the new syscall, I agree we can just > > standardize the return value, there ENODEV makes even less sense since > > there's no device in that call. > > Ok, will do. Who can pick this series up? Anyone else I need to Cc next > round? I guess I can pick up both kernel patches (the manpage patch needs to be submitted to the manpage list) but please CC linux-api@vger as well so that interested people are aware of the new syscall. Honza
On Thu, Feb 11, 2021 at 04:30:22PM +0100, Sascha Hauer wrote:
> + sb = mountpath.dentry->d_inode->i_sb;
Minor nit: mountpath.mnt->mnt_sb, please.
diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 6d16b2be5ac4..6f1df32abeea 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -17,6 +17,7 @@ #include <linux/capability.h> #include <linux/quotaops.h> #include <linux/types.h> +#include <linux/mount.h> #include <linux/writeback.h> #include <linux/nospec.h> #include "compat.h" @@ -968,3 +969,51 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, path_put(pathp); return ret; } + +SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, + mountpoint, qid_t, id, void __user *, addr) +{ + struct super_block *sb; + struct path mountpath; + unsigned int cmds = cmd >> SUBCMDSHIFT; + unsigned int type = cmd & SUBCMDMASK; + int ret; + + if (type >= MAXQUOTAS) + return -EINVAL; + + if (!mountpoint) + return -ENODEV; + + ret = user_path_at(AT_FDCWD, mountpoint, + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath); + if (ret) + return ret; + + sb = mountpath.dentry->d_inode->i_sb; + + if (quotactl_cmd_write(cmds)) { + ret = mnt_want_write(mountpath.mnt); + if (ret) + goto out; + } + + if (quotactl_cmd_onoff(cmds)) + down_write(&sb->s_umount); + else + down_read(&sb->s_umount); + + ret = do_quotactl(sb, type, cmds, id, addr, ERR_PTR(-EINVAL)); + + if (quotactl_cmd_onoff(cmds)) + up_write(&sb->s_umount); + else + up_read(&sb->s_umount); + + if (quotactl_cmd_write(cmds)) + mnt_drop_write(mountpath.mnt); +out: + path_put(&mountpath); + + return ret; +}
Add syscall quotactl_path, a variant of quotactl which allows to specify the mountpath instead of a path of to a block device. The quotactl syscall expects a path to the mounted block device to specify the filesystem to work on. This limits usage to filesystems which actually have a block device. quotactl_path replaces the path to the block device with a path where the filesystem is mounted at. The global Q_SYNC command to sync all filesystems is not supported for this new syscall, otherwise quotactl_path behaves like quotactl. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- fs/quota/quota.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)