[04/10] quota: Allow to pass mount path to quotactl
diff mbox series

Message ID 20191030152702.14269-5-s.hauer@pengutronix.de
State New
Headers show
Series
  • Add quota support to UBIFS
Related show

Commit Message

Sascha Hauer Oct. 30, 2019, 3:26 p.m. UTC
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(-)

Comments

Jan Kara Nov. 1, 2019, 4:07 p.m. UTC | #1
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
Sascha Hauer Nov. 11, 2019, 10:08 a.m. UTC | #2
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
Jan Kara Nov. 11, 2019, 11:05 a.m. UTC | #3
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
Sascha Hauer Nov. 11, 2019, 11:14 a.m. UTC | #4
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

Patch
diff mbox series

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