| Message ID | 20191030152702.14269-5-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add quota support to UBIFS | expand |
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@pengutronix.de> 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: if (q_path) { path_put(&qpath); } else { if (!quotactl_cmd_onoff(cmds)) drop_super(sb); else drop_super_exclusive(sb); } which would then make it more obvious, what's actually going on... Otherwise the patch looks good to me. Honza
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@pengutronix.de> > > 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
On Mon 11-11-19 11:08:07, Sascha Hauer wrote: > 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@pengutronix.de> > > > > 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. Right. But you also don't need to do path_put(&qpath) in case quotactl_block() failed. So you can just jump to out1 there as well and then 'out' is unused... Honza
On Mon, Nov 11, 2019 at 12:05:59PM +0100, Jan Kara wrote: > On Mon 11-11-19 11:08:07, Sascha Hauer wrote: > > 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@pengutronix.de> > > > > > > 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(-) > > > > > > > > - 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. > > Right. But you also don't need to do path_put(&qpath) in case > quotactl_block() failed. So you can just jump to out1 there as well and > then 'out' is unused... Ah, I see. Ok, will do this. Sascha
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; 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: if (pathp && !IS_ERR(pathp)) path_put(pathp); return ret; diff --git a/include/uapi/linux/quota.h b/include/uapi/linux/quota.h index f17c9636a859..e1787c0df601 100644 --- a/include/uapi/linux/quota.h +++ b/include/uapi/linux/quota.h @@ -71,6 +71,7 @@ #define Q_GETQUOTA 0x800007 /* get user quota structure */ #define Q_SETQUOTA 0x800008 /* set user quota structure */ #define Q_GETNEXTQUOTA 0x800009 /* get disk limits and usage >= ID */ +#define Q_PATH 0x400000 /* quotactl special arg contains mount path */ /* Quota format type IDs */ #define QFMT_VFS_OLD 1
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@pengutronix.de> --- fs/quota/quota.c | 37 ++++++++++++++++++++++++++++--------- include/uapi/linux/quota.h | 1 + 2 files changed, 29 insertions(+), 9 deletions(-)