On Fri, Nov 01, 2019 at 05:07:06PM +0100, Jan Kara wrote:
> On Wed 30-10-19 16:26:56, Sascha Hauer wrote:
> > This patch introduces the Q_PATH flag to the quotactl cmd argument.
> > When given, the path given in the special argument to quotactl will
> > be the mount path where the filesystem is mounted, instead of a path
> > to the block device.
> > This is necessary for filesystems which do not have a block device as
> > backing store. Particularly this is done for upcoming UBIFS support.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>
> Thanks for the patch! Some smaller comments below...
>
> > ---
> > fs/quota/quota.c | 37 ++++++++++++++++++++++++++++---------
> > include/uapi/linux/quota.h | 1 +
> > 2 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> > index cb13fb76dbee..035cdd1b022b 100644
> > --- a/fs/quota/quota.c
> > +++ b/fs/quota/quota.c
> > @@ -19,6 +19,7 @@
> > #include <linux/types.h>
> > #include <linux/writeback.h>
> > #include <linux/nospec.h>
> > +#include <linux/mount.h>
> >
> > static int check_quotactl_permission(struct super_block *sb, int type, int cmd,
> > qid_t id)
> > @@ -825,12 +826,16 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
> > {
> > uint cmds, type;
> > struct super_block *sb = NULL;
> > - struct path path, *pathp = NULL;
> > + struct path path, *pathp = NULL, qpath;
>
> Maybe call these two 'file_path', 'file_pathp', and 'sb_path' to make it
> clearer which path is which?
>
> > int ret;
> > + bool q_path;
> >
> > cmds = cmd >> SUBCMDSHIFT;
> > type = cmd & SUBCMDMASK;
> >
> > + q_path = cmds & Q_PATH;
> > + cmds &= ~Q_PATH;
> > +
> > /*
> > * As a special case Q_SYNC can be called without a specific device.
> > * It will iterate all superblocks that have quota enabled and call
> > @@ -855,19 +860,33 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
> > pathp = &path;
> > }
> >
> > - sb = quotactl_block(special, cmds);
> > - if (IS_ERR(sb)) {
> > - ret = PTR_ERR(sb);
> > - goto out;
> > + if (q_path) {
> > + ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT,
> > + &qpath);
> > + if (ret)
> > + goto out1;
> > +
> > + sb = qpath.mnt->mnt_sb;
> > + } else {
> > + sb = quotactl_block(special, cmds);
> > + if (IS_ERR(sb)) {
> > + ret = PTR_ERR(sb);
> > + goto out;
> > + }
> > }
> >
> > ret = do_quotactl(sb, type, cmds, id, addr, pathp);
> >
> > - if (!quotactl_cmd_onoff(cmds))
> > - drop_super(sb);
> > - else
> > - drop_super_exclusive(sb);
> > + if (!q_path) {
> > + if (!quotactl_cmd_onoff(cmds))
> > + drop_super(sb);
> > + else
> > + drop_super_exclusive(sb);
> > + }
> > out:
> > + if (q_path)
> > + path_put(&qpath);
> > +out1:
>
> Why do you need out1? If you leave 'out' as is, things should just work.
> And you can also combine the above if like:
See above where out1 is used. In this case qpath is not valid and I
cannot call path_put() on it.
I see that having q_path and qpath as different variables is confusing,
but as you say I will rename qpath to sb_path, so hopefully this
already makes it clearer.
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-5555 |
