[PATCH 1/2] quota: Add mountpath based quota support
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
Reviewed-by: Christoph Hellwig
---
fs/quota/quota.c | 49 +---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 6d16b2be5ac4..f7b4b66491fc 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include "compat.h"
@@ -827,8 +828,6 @@ static int do_quotactl(struct super_block *sb, int type,
int cmd, qid_t id,
}
}
-#ifdef CONFIG_BLOCK
-
/* Return 1 if 'cmd' will block on frozen filesystem */
static int quotactl_cmd_write(int cmd)
{
@@ -850,7 +849,6 @@ static int quotactl_cmd_write(int cmd)
}
return 1;
}
-#endif /* CONFIG_BLOCK */
/* Return true if quotactl command is manipulating quota on/off state */
static bool quotactl_cmd_onoff(int cmd)
@@ -968,3 +966,48 @@ 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;
+
+ ret = user_path_at(AT_FDCWD, mountpoint,
+LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
+ if (ret)
+ return ret;
+
+ sb = mountpath.mnt->mnt_sb;
+
+ if (quotactl_cmd_write(cmds)) {
+ ret = mnt_want_write(mountpath.mnt);
+ if (ret)
+ goto out;
+ }
+
+ if (quotactl_cmd_onoff(cmds))
+ down_write(>s_umount);
+ else
+ down_read(>s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, ERR_PTR(-EINVAL));
+
+ if (quotactl_cmd_onoff(cmds))
+ up_write(>s_umount);
+ else
+ up_read(>s_umount);
+
+ if (quotactl_cmd_write(cmds))
+ mnt_drop_write(mountpath.mnt);
+out:
+ path_put();
+
+ return ret;
+}
--
2.29.2
Re: [PATCH 1/2] quota: Add mountpath based quota support
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.
Re: [PATCH 1/2] quota: Add mountpath based quota support
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 +, Christoph Hellwig wrote:
> > > > > + if (!mountpoint)
> > > > > + return -ENODEV;
> > > > > +
> > > > > + ret = user_path_at(AT_FDCWD, mountpoint,
> > > > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > > > );
> > > >
> > > > 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
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 1/2] quota: Add mountpath based quota support
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 +, Christoph Hellwig wrote:
> > > > + if (!mountpoint)
> > > > + return -ENODEV;
> > > > +
> > > > + ret = user_path_at(AT_FDCWD, mountpoint,
> > > > +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > > );
> > >
> > > 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Fri 12-02-21 09:38:35, Sascha Hauer wrote:
> On Thu, Feb 11, 2021 at 03:38:13PM +, Christoph Hellwig wrote:
> > > + if (!mountpoint)
> > > + return -ENODEV;
> > > +
> > > + ret = user_path_at(AT_FDCWD, mountpoint,
> > > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
> >
> > 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
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Thu, Feb 11, 2021 at 03:38:13PM +, Christoph Hellwig wrote:
> > + if (!mountpoint)
> > + return -ENODEV;
> > +
> > + ret = user_path_at(AT_FDCWD, mountpoint,
> > +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
>
> 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] quota: Add mountpath based quota support
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
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,
);
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-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH 1/2] quota: Add mountpath based quota support
> + if (!mountpoint) > + return -ENODEV; > + > + ret = user_path_at(AT_FDCWD, mountpoint, > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, ); 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
[PATCH 1/2] quota: Add mountpath based quota support
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
---
fs/quota/quota.c | 49
1 file changed, 49 insertions(+)
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
#include
#include
+#include
#include
#include
#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, );
+ 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(>s_umount);
+ else
+ down_read(>s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, ERR_PTR(-EINVAL));
+
+ if (quotactl_cmd_onoff(cmds))
+ up_write(>s_umount);
+ else
+ up_read(>s_umount);
+
+ if (quotactl_cmd_write(cmds))
+ mnt_drop_write(mountpath.mnt);
+out:
+ path_put();
+
+ return ret;
+}
--
2.20.1
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Tue 09-02-21 08:51:01, Christoph Hellwig wrote: > On Thu, Feb 04, 2021 at 01:53:50PM +0100, Jan Kara wrote: > > Now quota data stored in a normal file is a setup we try to deprecate > > anyway so another option is to just leave quotactl_path() only for those > > setups where quota metadata is managed by the filesystem so we don't need > > to pass quota files to Q_QUOTAON? > > I'd be perfectly fine with that. OK, then this looks like the best way forward to me. We can always extend the quotactl_path() syscall later if we find this is problematic for some real usecases. Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Thu, Feb 04, 2021 at 01:53:50PM +0100, Jan Kara wrote: > Now quota data stored in a normal file is a setup we try to deprecate > anyway so another option is to just leave quotactl_path() only for those > setups where quota metadata is managed by the filesystem so we don't need > to pass quota files to Q_QUOTAON? I'd be perfectly fine with that.
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Thu 04-02-21 07:34:14, Christoph Hellwig wrote:
> On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote:
> > Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
> > quota file - unless the filesystem stores quota in hidden files in which
> > case this argument is just ignored. You're right we could require that
> > specifically for Q_QUOTAON, the mountpoint path would actually need to
> > point to the quota file if it is relevant, otherwise anywhere in the
> > appropriate filesystem. We don't allow quota file to reside on a different
> > filesystem (for a past decade or so) so it should work fine.
> >
> > So the only problem I have is whether requiring the mountpoint argument to
> > point quota file for Q_QUOTAON isn't going to be somewhat confusing to
> > users. At the very least it would require some careful explanation in the
> > manpage to explain the difference between quotactl_path() and quotactl()
> > in this regard. But is saving the second path for Q_QUOTAON really worth the
> > bother?
>
> I find the doubled path argument a really horrible API, so I'd pretty
> strongly prefer to avoid that.
Honestly, I don't understand why is it so horrible. The paths point to
different things... The first path identifies the filesystem to operate on,
the second path identifies the file which contains quota accounting data.
In the ancient times, the file with quota accounting data could even be
stored on a different filesystem (these were still times when filesystem
metadata journalling was a new thing - like late 90's). But later I just
disallowed that because it was not very useful (and luckily even used) and
just complicated matters.
Anyway, back to 2021 :). What I find somewhat confusing about a single path
for Q_QUOTAON is that for any other quotactl, any path on the filesystem is
fine. Similarly if quota data is stored in the hidden file, any path on the
filesystem is fine. It is only for Q_QUOTAON on a filesystem where quota
data is stored in a normal file, where we suddently require that the path
has to point to it.
Now quota data stored in a normal file is a setup we try to deprecate
anyway so another option is to just leave quotactl_path() only for those
setups where quota metadata is managed by the filesystem so we don't need
to pass quota files to Q_QUOTAON?
> > > Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
> > > could probably simplify this down do:
> > >
> > > if (quotactl_cmd_write(cmd)) {
> >
> > This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
> > Otherwise I agree what you suggest is somewhat more readable given how
> > small the function is.
>
> The way I read quotactl_cmd_write, it only special cases a few commands
> and returns 0 there, so we should not need the extra quotactl_cmd_onoff
> call, as all those commands are not explicitly listed.
Right, sorry, I was mistaken.
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote:
> Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
> quota file - unless the filesystem stores quota in hidden files in which
> case this argument is just ignored. You're right we could require that
> specifically for Q_QUOTAON, the mountpoint path would actually need to
> point to the quota file if it is relevant, otherwise anywhere in the
> appropriate filesystem. We don't allow quota file to reside on a different
> filesystem (for a past decade or so) so it should work fine.
>
> So the only problem I have is whether requiring the mountpoint argument to
> point quota file for Q_QUOTAON isn't going to be somewhat confusing to
> users. At the very least it would require some careful explanation in the
> manpage to explain the difference between quotactl_path() and quotactl()
> in this regard. But is saving the second path for Q_QUOTAON really worth the
> bother?
I find the doubled path argument a really horrible API, so I'd pretty
strongly prefer to avoid that.
> > Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
> > could probably simplify this down do:
> >
> > if (quotactl_cmd_write(cmd)) {
>
> This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
> Otherwise I agree what you suggest is somewhat more readable given how
> small the function is.
The way I read quotactl_cmd_write, it only special cases a few commands
and returns 0 there, so we should not need the extra quotactl_cmd_onoff
call, as all those commands are not explicitly listed.
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Thu 28-01-21 14:35:52, Christoph Hellwig wrote:
> > + struct path path, *pathp = NULL;
> > + struct path mountpath;
> > + bool excl = false, thawed = false;
> > + int ret;
> > +
> > + cmds = cmd >> SUBCMDSHIFT;
> > + type = cmd & SUBCMDMASK;
>
> Personal pet peeve: it would be nice to just initialize cmds and
> type on their declaration line, or while we're at it declutter
> this a bit and remove the separate cmds variable:
>
> unsigned int type = cmd & SUBCMDMASK;
>
> cmd >>= SUBCMDSHIFT;
Yeah, whatever :)
> > + /*
> > +* Path for quotaon has to be resolved before grabbing superblock
> > +* because that gets s_umount sem which is also possibly needed by path
> > +* resolution (think about autofs) and thus deadlocks could arise.
> > +*/
> > + if (cmds == Q_QUOTAON) {
> > + ret = user_path_at(AT_FDCWD, addr,
> > + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
> > + if (ret)
> > + pathp = ERR_PTR(ret);
> > + else
> > + pathp =
> > + }
> > +
> > + ret = user_path_at(AT_FDCWD, mountpoint,
> > +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
> > + if (ret)
> > + goto out;
>
> I don't think we need two path lookups here, we can path the same path
> to the command for quotaon.
Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
quota file - unless the filesystem stores quota in hidden files in which
case this argument is just ignored. You're right we could require that
specifically for Q_QUOTAON, the mountpoint path would actually need to
point to the quota file if it is relevant, otherwise anywhere in the
appropriate filesystem. We don't allow quota file to reside on a different
filesystem (for a past decade or so) so it should work fine.
So the only problem I have is whether requiring the mountpoint argument to
point quota file for Q_QUOTAON isn't going to be somewhat confusing to
users. At the very least it would require some careful explanation in the
manpage to explain the difference between quotactl_path() and quotactl()
in this regard. But is saving the second path for Q_QUOTAON really worth the
bother?
> > + if (quotactl_cmd_onoff(cmds)) {
> > + excl = true;
> > + thawed = true;
> > + } else if (quotactl_cmd_write(cmds)) {
> > + thawed = true;
> > + }
> > +
> > + if (thawed) {
> > + ret = mnt_want_write(mountpath.mnt);
> > + if (ret)
> > + goto out1;
> > + }
> > +
> > + sb = mountpath.dentry->d_inode->i_sb;
> > +
> > + if (excl)
> > + down_write(>s_umount);
> > + else
> > + down_read(>s_umount);
>
> Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
> could probably simplify this down do:
>
> if (quotactl_cmd_write(cmd)) {
This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
Otherwise I agree what you suggest is somewhat more readable given how
small the function is.
> ret = mnt_want_write(path.mnt);
> if (ret)
> goto out1;
> }
> if (quotactl_cmd_onoff(cmd))
> down_write(>s_umount);
> else
> down_read(>s_umount);
>
> and duplicate the checks after the do_quotactl call.
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 1/2] quota: Add mountpath based quota support
> + uint cmds, type;
> + struct super_block *sb = NULL;
I don't think sb needs the NULL initialization.
> + struct path path, *pathp = NULL;
> + struct path mountpath;
> + bool excl = false, thawed = false;
> + int ret;
> +
> + cmds = cmd >> SUBCMDSHIFT;
> + type = cmd & SUBCMDMASK;
Personal pet peeve: it would be nice to just initialize cmds and
type on their declaration line, or while we're at it declutter
this a bit and remove the separate cmds variable:
unsigned int type = cmd & SUBCMDMASK;
cmd >>= SUBCMDSHIFT;
> + /*
> + * Path for quotaon has to be resolved before grabbing superblock
> + * because that gets s_umount sem which is also possibly needed by path
> + * resolution (think about autofs) and thus deadlocks could arise.
> + */
> + if (cmds == Q_QUOTAON) {
> + ret = user_path_at(AT_FDCWD, addr,
> +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
> + if (ret)
> + pathp = ERR_PTR(ret);
> + else
> + pathp =
> + }
> +
> + ret = user_path_at(AT_FDCWD, mountpoint,
> + LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
> + if (ret)
> + goto out;
I don't think we need two path lookups here, we can path the same path
to the command for quotaon.
> + if (quotactl_cmd_onoff(cmds)) {
> + excl = true;
> + thawed = true;
> + } else if (quotactl_cmd_write(cmds)) {
> + thawed = true;
> + }
> +
> + if (thawed) {
> + ret = mnt_want_write(mountpath.mnt);
> + if (ret)
> + goto out1;
> + }
> +
> + sb = mountpath.dentry->d_inode->i_sb;
> +
> + if (excl)
> + down_write(>s_umount);
> + else
> + down_read(>s_umount);
Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
could probably simplify this down do:
if (quotactl_cmd_write(cmd)) {
ret = mnt_want_write(path.mnt);
if (ret)
goto out1;
}
if (quotactl_cmd_onoff(cmd))
down_write(>s_umount);
else
down_read(>s_umount);
and duplicate the checks after the do_quotactl call.
[PATCH 1/2] quota: Add mountpath based quota support
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
---
fs/quota/quota.c | 77
1 file changed, 77 insertions(+)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 6d16b2be5ac4..9ac09e128686 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include "compat.h"
@@ -968,3 +969,79 @@ 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)
+{
+ uint cmds, type;
+ struct super_block *sb = NULL;
+ struct path path, *pathp = NULL;
+ struct path mountpath;
+ bool excl = false, thawed = false;
+ int ret;
+
+ cmds = cmd >> SUBCMDSHIFT;
+ type = cmd & SUBCMDMASK;
+
+ if (type >= MAXQUOTAS)
+ return -EINVAL;
+
+ if (!mountpoint)
+ return -ENODEV;
+
+ /*
+* Path for quotaon has to be resolved before grabbing superblock
+* because that gets s_umount sem which is also possibly needed by path
+* resolution (think about autofs) and thus deadlocks could arise.
+*/
+ if (cmds == Q_QUOTAON) {
+ ret = user_path_at(AT_FDCWD, addr,
+ LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
+ if (ret)
+ pathp = ERR_PTR(ret);
+ else
+ pathp =
+ }
+
+ ret = user_path_at(AT_FDCWD, mountpoint,
+LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, );
+ if (ret)
+ goto out;
+
+ if (quotactl_cmd_onoff(cmds)) {
+ excl = true;
+ thawed = true;
+ } else if (quotactl_cmd_write(cmds)) {
+ thawed = true;
+ }
+
+ if (thawed) {
+ ret = mnt_want_write(mountpath.mnt);
+ if (ret)
+ goto out1;
+ }
+
+ sb = mountpath.dentry->d_inode->i_sb;
+
+ if (excl)
+ down_write(>s_umount);
+ else
+ down_read(>s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, pathp);
+
+ if (excl)
+ up_write(>s_umount);
+ else
+ up_read(>s_umount);
+
+ if (thawed)
+ mnt_drop_write(mountpath.mnt);
+out1:
+ path_put();
+
+out:
+ if (pathp && !IS_ERR(pathp))
+ path_put(pathp);
+ return ret;
+}
--
2.20.1
