diff mbox series

[1/8] quota: Allow to pass mount path to quotactl

Message ID 20200124131323.23885-2-s.hauer@pengutronix.de
State Not Applicable
Headers show
Series Add quota support to UBIFS | expand

Commit Message

Sascha Hauer Jan. 24, 2020, 1:13 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           | 53 +++++++++++++++++++++++++++-----------
 include/uapi/linux/quota.h |  1 +
 2 files changed, 39 insertions(+), 15 deletions(-)

Comments

Jan Kara Jan. 27, 2020, 10:45 a.m. UTC | #1 
On Fri 24-01-20 14:13:16, 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 comments are below.

> @@ -821,15 +822,20 @@ 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 file_path, *file_pathp = NULL, sb_path;
>  	int ret;
> +	bool q_path;
>  
>  	cmds = cmd >> SUBCMDSHIFT;
>  	type = cmd & SUBCMDMASK;
>  
> +

Unnecessary empty line added...

>  	if (type >= MAXQUOTAS)
>  		return -EINVAL;
>  
> +	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
> @@ -847,28 +853,45 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
>  	 * resolution (think about autofs) and thus deadlocks could arise.
>  	 */
>  	if (cmds == Q_QUOTAON) {
> -		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &path);
> +		ret = user_path_at(AT_FDCWD, addr,
> +				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> +				   &file_path);
>  		if (ret)
> -			pathp = ERR_PTR(ret);
> +			file_pathp = ERR_PTR(ret);
>  		else
> -			pathp = &path;
> +			file_pathp = &file_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,
> +				   &sb_path);
> +		if (ret)
> +			goto out;
> +
> +		sb = sb_path.mnt->mnt_sb;

So I've realized that just looking up superblock with user_path_at() is not
enough. Quota code also expects that the superblock will be locked
(sb->s_umount) and filesystem will not be frozen (in case the quota
operation is going to modify the filesystem). This is needed to serialize
e.g. remount and quota operations or quota operations among themselves.
So you still need something like following to get superblock from the path:

static int quotactl_path(const char __user *special, int cmd, struct path *path)
{
	struct super_block *sb;

	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
			   path);
	if (ret)
		return ret;
	sb = sb_path.mnt->mnt_sb;
restart:
	if (quotactl_cmd_onoff(cmd))
		down_write(&sb->s_umount);
	else
		down_read(&sb->s_umount);

	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
		if (quotactl_cmd_onoff(cmd))
			up_write(&sb->s_umount);
		else
			up_read(&sb->s_umount);
		wait_event(sb->s_writers.wait_unfrozen,
			   sb->s_writers.frozen == SB_UNFROZEN);
		goto restart;

	}
	return sb;
}

And then appropriate counterparts when releasing the superblock.

								Honza
Sascha Hauer Jan. 28, 2020, 10:06 a.m. UTC | #2 
Hi Jan,

On Mon, Jan 27, 2020 at 11:45:18AM +0100, Jan Kara wrote:
> >  	cmds = cmd >> SUBCMDSHIFT;
> >  	type = cmd & SUBCMDMASK;
> >  
> > +
> 
> Unnecessary empty line added...

Fixed

> > +	if (q_path) {
> > +		ret = user_path_at(AT_FDCWD, special,
> > +				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > +				   &sb_path);
> > +		if (ret)
> > +			goto out;
> > +
> > +		sb = sb_path.mnt->mnt_sb;
> 
> So I've realized that just looking up superblock with user_path_at() is not
> enough. Quota code also expects that the superblock will be locked
> (sb->s_umount) and filesystem will not be frozen (in case the quota
> operation is going to modify the filesystem). This is needed to serialize
> e.g. remount and quota operations or quota operations among themselves.
> So you still need something like following to get superblock from the path:

Ok, here's an updated version. I'll send an update for the whole series
when Richard had a look over it.

Sascha

----------------------------8<-----------------------------

From 9c91395f2667c8a48f52a80896e559daf16f4a4c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Wed, 30 Oct 2019 08:35:11 +0100
Subject: [PATCH] quota: Allow to pass mount path to quotactl

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           | 75 ++++++++++++++++++++++++++++++++------
 include/uapi/linux/quota.h |  1 +
 2 files changed, 64 insertions(+), 12 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 5444d3c4d93f..712b71760f9d 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)
@@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
 #endif
 }
 
+static struct super_block *quotactl_path(const char __user *special, int cmd,
+					 struct path *path)
+{
+	struct super_block *sb;
+	int ret;
+
+	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
+			   path);
+	if (ret)
+		return ERR_PTR(ret);
+
+	sb = path->mnt->mnt_sb;
+restart:
+	if (quotactl_cmd_onoff(cmd))
+		down_write(&sb->s_umount);
+	else
+		down_read(&sb->s_umount);
+
+	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
+		if (quotactl_cmd_onoff(cmd))
+			up_write(&sb->s_umount);
+		else
+			up_read(&sb->s_umount);
+		wait_event(sb->s_writers.wait_unfrozen,
+			   sb->s_writers.frozen == SB_UNFROZEN);
+		goto restart;
+	}
+
+	return sb;
+}
 /*
  * This is the system call interface. This communicates with
  * the user-level programs. Currently this only supports diskquota
@@ -821,8 +852,9 @@ 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 file_path, *file_pathp = NULL, sb_path;
 	int ret;
+	bool q_path;
 
 	cmds = cmd >> SUBCMDSHIFT;
 	type = cmd & SUBCMDMASK;
@@ -830,6 +862,9 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
 	if (type >= MAXQUOTAS)
 		return -EINVAL;
 
+	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
@@ -847,28 +882,44 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
 	 * resolution (think about autofs) and thus deadlocks could arise.
 	 */
 	if (cmds == Q_QUOTAON) {
-		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &path);
+		ret = user_path_at(AT_FDCWD, addr,
+				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
+				   &file_path);
 		if (ret)
-			pathp = ERR_PTR(ret);
+			file_pathp = ERR_PTR(ret);
 		else
-			pathp = &path;
+			file_pathp = &file_path;
 	}
 
-	sb = quotactl_block(special, cmds);
+	if (q_path)
+		sb = quotactl_path(special, cmds, &sb_path);
+	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);
+	ret = do_quotactl(sb, type, cmds, id, addr, file_pathp);
+
+	if (q_path) {
+		if (quotactl_cmd_onoff(cmd))
+			up_write(&sb->s_umount);
+		else
+			up_read(&sb->s_umount);
+
+		path_put(&sb_path);
+	} else {
+		if (!quotactl_cmd_onoff(cmds))
+			drop_super(sb);
+		else
+			drop_super_exclusive(sb);
+	}
 
-	if (!quotactl_cmd_onoff(cmds))
-		drop_super(sb);
-	else
-		drop_super_exclusive(sb);
 out:
-	if (pathp && !IS_ERR(pathp))
-		path_put(pathp);
+	if (file_pathp && !IS_ERR(file_pathp))
+		path_put(file_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
Jan Kara Jan. 28, 2020, 11:41 a.m. UTC | #3 
On Tue 28-01-20 11:06:31, Sascha Hauer wrote:
> Hi Jan,
> 
> On Mon, Jan 27, 2020 at 11:45:18AM +0100, Jan Kara wrote:
> > >  	cmds = cmd >> SUBCMDSHIFT;
> > >  	type = cmd & SUBCMDMASK;
> > >  
> > > +
> > 
> > Unnecessary empty line added...
> 
> Fixed
> 
> > > +	if (q_path) {
> > > +		ret = user_path_at(AT_FDCWD, special,
> > > +				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > +				   &sb_path);
> > > +		if (ret)
> > > +			goto out;
> > > +
> > > +		sb = sb_path.mnt->mnt_sb;
> > 
> > So I've realized that just looking up superblock with user_path_at() is not
> > enough. Quota code also expects that the superblock will be locked
> > (sb->s_umount) and filesystem will not be frozen (in case the quota
> > operation is going to modify the filesystem). This is needed to serialize
> > e.g. remount and quota operations or quota operations among themselves.
> > So you still need something like following to get superblock from the path:
> 
> Ok, here's an updated version. I'll send an update for the whole series
> when Richard had a look over it.
> 
> Sascha
> 
> ----------------------------8<-----------------------------
> 
> From 9c91395f2667c8a48f52a80896e559daf16f4a4c Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Wed, 30 Oct 2019 08:35:11 +0100
> Subject: [PATCH] quota: Allow to pass mount path to quotactl
> 
> 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. The patch looks good to me now. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/quota/quota.c           | 75 ++++++++++++++++++++++++++++++++------
>  include/uapi/linux/quota.h |  1 +
>  2 files changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 5444d3c4d93f..712b71760f9d 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)
> @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
>  #endif
>  }
>  
> +static struct super_block *quotactl_path(const char __user *special, int cmd,
> +					 struct path *path)
> +{
> +	struct super_block *sb;
> +	int ret;
> +
> +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> +			   path);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	sb = path->mnt->mnt_sb;
> +restart:
> +	if (quotactl_cmd_onoff(cmd))
> +		down_write(&sb->s_umount);
> +	else
> +		down_read(&sb->s_umount);
> +
> +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> +		if (quotactl_cmd_onoff(cmd))
> +			up_write(&sb->s_umount);
> +		else
> +			up_read(&sb->s_umount);
> +		wait_event(sb->s_writers.wait_unfrozen,
> +			   sb->s_writers.frozen == SB_UNFROZEN);
> +		goto restart;
> +	}
> +
> +	return sb;
> +}
>  /*
>   * This is the system call interface. This communicates with
>   * the user-level programs. Currently this only supports diskquota
> @@ -821,8 +852,9 @@ 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 file_path, *file_pathp = NULL, sb_path;
>  	int ret;
> +	bool q_path;
>  
>  	cmds = cmd >> SUBCMDSHIFT;
>  	type = cmd & SUBCMDMASK;
> @@ -830,6 +862,9 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
>  	if (type >= MAXQUOTAS)
>  		return -EINVAL;
>  
> +	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
> @@ -847,28 +882,44 @@ int kernel_quotactl(unsigned int cmd, const char __user *special,
>  	 * resolution (think about autofs) and thus deadlocks could arise.
>  	 */
>  	if (cmds == Q_QUOTAON) {
> -		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &path);
> +		ret = user_path_at(AT_FDCWD, addr,
> +				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> +				   &file_path);
>  		if (ret)
> -			pathp = ERR_PTR(ret);
> +			file_pathp = ERR_PTR(ret);
>  		else
> -			pathp = &path;
> +			file_pathp = &file_path;
>  	}
>  
> -	sb = quotactl_block(special, cmds);
> +	if (q_path)
> +		sb = quotactl_path(special, cmds, &sb_path);
> +	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);
> +	ret = do_quotactl(sb, type, cmds, id, addr, file_pathp);
> +
> +	if (q_path) {
> +		if (quotactl_cmd_onoff(cmd))
> +			up_write(&sb->s_umount);
> +		else
> +			up_read(&sb->s_umount);
> +
> +		path_put(&sb_path);
> +	} else {
> +		if (!quotactl_cmd_onoff(cmds))
> +			drop_super(sb);
> +		else
> +			drop_super_exclusive(sb);
> +	}
>  
> -	if (!quotactl_cmd_onoff(cmds))
> -		drop_super(sb);
> -	else
> -		drop_super_exclusive(sb);
>  out:
> -	if (pathp && !IS_ERR(pathp))
> -		path_put(pathp);
> +	if (file_pathp && !IS_ERR(file_pathp))
> +		path_put(file_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
> -- 
> 2.25.0
> 
> 
> -- 
> 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 |
Al Viro Jan. 29, 2020, 1:29 a.m. UTC | #4 
On Tue, Jan 28, 2020 at 11:06:31AM +0100, Sascha Hauer wrote:
> Hi Jan,

> @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
>  #endif
>  }
>  
> +static struct super_block *quotactl_path(const char __user *special, int cmd,
> +					 struct path *path)
> +{
> +	struct super_block *sb;
> +	int ret;
> +
> +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> +			   path);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	sb = path->mnt->mnt_sb;
> +restart:
> +	if (quotactl_cmd_onoff(cmd))
> +		down_write(&sb->s_umount);
> +	else
> +		down_read(&sb->s_umount);
> +
> +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> +		if (quotactl_cmd_onoff(cmd))
> +			up_write(&sb->s_umount);
> +		else
> +			up_read(&sb->s_umount);
> +		wait_event(sb->s_writers.wait_unfrozen,
> +			   sb->s_writers.frozen == SB_UNFROZEN);
> +		goto restart;
> +	}
> +
> +	return sb;
> +}

This partial duplicate of __get_super_thawed() guts does *not* belong here,
especially not interleaved with quota-specific checks.

> +	if (q_path) {
> +		if (quotactl_cmd_onoff(cmd))
> +			up_write(&sb->s_umount);
> +		else
> +			up_read(&sb->s_umount);
> +
> +		path_put(&sb_path);
> +	} else {
> +		if (!quotactl_cmd_onoff(cmds))
> +			drop_super(sb);
> +		else
> +			drop_super_exclusive(sb);
> +	}

Er...  Why not have the same code that you've used to lock the damn thing
(needs to be moved to fs/super.c) simply get a passive ref to it?  Then
you could do the same thing, q_path or no q_path...
Jan Kara Jan. 29, 2020, 4:14 p.m. UTC | #5 
On Wed 29-01-20 01:29:29, Al Viro wrote:
> On Tue, Jan 28, 2020 at 11:06:31AM +0100, Sascha Hauer wrote:
> > Hi Jan,
> 
> > @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
> >  #endif
> >  }
> >  
> > +static struct super_block *quotactl_path(const char __user *special, int cmd,
> > +					 struct path *path)
> > +{
> > +	struct super_block *sb;
> > +	int ret;
> > +
> > +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > +			   path);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	sb = path->mnt->mnt_sb;
> > +restart:
> > +	if (quotactl_cmd_onoff(cmd))
> > +		down_write(&sb->s_umount);
> > +	else
> > +		down_read(&sb->s_umount);
> > +
> > +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> > +		if (quotactl_cmd_onoff(cmd))
> > +			up_write(&sb->s_umount);
> > +		else
> > +			up_read(&sb->s_umount);
> > +		wait_event(sb->s_writers.wait_unfrozen,
> > +			   sb->s_writers.frozen == SB_UNFROZEN);
> > +		goto restart;
> > +	}
> > +
> > +	return sb;
> > +}
> 
> This partial duplicate of __get_super_thawed() guts does *not* belong here,
> especially not interleaved with quota-specific checks.

OK, so some primitive in fs/super.c like:

void hold_super_thawed(struct super_block *sb, bool excl);

that would implement the above functionality and grab passive reference?

> > +	if (q_path) {
> > +		if (quotactl_cmd_onoff(cmd))
> > +			up_write(&sb->s_umount);
> > +		else
> > +			up_read(&sb->s_umount);
> > +
> > +		path_put(&sb_path);
> > +	} else {
> > +		if (!quotactl_cmd_onoff(cmds))
> > +			drop_super(sb);
> > +		else
> > +			drop_super_exclusive(sb);
> > +	}
> 
> Er...  Why not have the same code that you've used to lock the damn thing
> (needs to be moved to fs/super.c) simply get a passive ref to it?  Then
> you could do the same thing, q_path or no q_path...

Yes.

								Honza
Sascha Hauer Feb. 4, 2020, 10:35 a.m. UTC | #6 
On Wed, Jan 29, 2020 at 01:29:29AM +0000, Al Viro wrote:
> On Tue, Jan 28, 2020 at 11:06:31AM +0100, Sascha Hauer wrote:
> > Hi Jan,
> 
> > @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
> >  #endif
> >  }
> >  
> > +static struct super_block *quotactl_path(const char __user *special, int cmd,
> > +					 struct path *path)
> > +{
> > +	struct super_block *sb;
> > +	int ret;
> > +
> > +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > +			   path);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	sb = path->mnt->mnt_sb;
> > +restart:
> > +	if (quotactl_cmd_onoff(cmd))
> > +		down_write(&sb->s_umount);
> > +	else
> > +		down_read(&sb->s_umount);
> > +
> > +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> > +		if (quotactl_cmd_onoff(cmd))
> > +			up_write(&sb->s_umount);
> > +		else
> > +			up_read(&sb->s_umount);
> > +		wait_event(sb->s_writers.wait_unfrozen,
> > +			   sb->s_writers.frozen == SB_UNFROZEN);
> > +		goto restart;
> > +	}
> > +
> > +	return sb;
> > +}
> 
> This partial duplicate of __get_super_thawed() guts does *not* belong here,
> especially not interleaved with quota-specific checks.
> 
> > +	if (q_path) {
> > +		if (quotactl_cmd_onoff(cmd))
> > +			up_write(&sb->s_umount);
> > +		else
> > +			up_read(&sb->s_umount);
> > +
> > +		path_put(&sb_path);
> > +	} else {
> > +		if (!quotactl_cmd_onoff(cmds))
> > +			drop_super(sb);
> > +		else
> > +			drop_super_exclusive(sb);
> > +	}
> 
> Er...  Why not have the same code that you've used to lock the damn thing
> (needs to be moved to fs/super.c) simply get a passive ref to it?  Then
> you could do the same thing, q_path or no q_path...

I am getting confused here. To an earlier version of this series you
responded:

> And for path-based you don't need to mess with superblock
> references - just keep the struct path until the end.  That
> will keep the superblock alive and active just fine.

I did that and got the objection from Jan:

> So I've realized that just looking up superblock with user_path_at() is not
> enough. Quota code also expects that the superblock will be locked
> (sb->s_umount) and filesystem will not be frozen (in case the quota
> operation is going to modify the filesystem). This is needed to serialize
> e.g. remount and quota operations or quota operations among themselves.

So after drawing circles we now seem to be back at passive references.
What I have now in my tree is this in fs/super.c, untested currently:

static bool __grab_super_thawed(struct super_block *sb, bool excl)
{
	while (1) {
		bool again = false;

		spin_lock(&sb_lock);

		if (hlist_unhashed(&sb->s_instances)) {
			spin_unlock(&sb_lock);
			return false;
		}

		sb->s_count++;
		spin_unlock(&sb_lock);

		if (excl)
			down_write(&sb->s_umount);
		else
			down_read(&sb->s_umount);

		if (sb->s_root && (sb->s_flags & SB_BORN)) {
			if (sb->s_writers.frozen == SB_UNFROZEN)
				return true;
			else
				again = true;
		}

		if (excl)
			up_write(&sb->s_umount);
		else
			up_read(&sb->s_umount);

		if (again)
			wait_event(sb->s_writers.wait_unfrozen,
				   sb->s_writers.frozen == SB_UNFROZEN);

		put_super(sb);

		if (!again)
			return false;
	}

	return ret;
}

int grab_super_thawed(struct super_block *sb)
{
	return __grab_super_thawed(sb, false);
}

int grab_super_exclusive_thawed(struct super_block *sb)
{
	return __grab_super_thawed(sb, true);
}

Does this look ok now?

Sascha
Jan Kara Feb. 18, 2020, 9:22 a.m. UTC | #7 
I'm sorry for the late reply, I was busy with other things and I wasn't
quite sure how I'd like this to be handled :)

On Tue 04-02-20 11:35:23, Sascha Hauer wrote:
> On Wed, Jan 29, 2020 at 01:29:29AM +0000, Al Viro wrote:
> > On Tue, Jan 28, 2020 at 11:06:31AM +0100, Sascha Hauer wrote:
> > > Hi Jan,
> > 
> > > @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
> > >  #endif
> > >  }
> > >  
> > > +static struct super_block *quotactl_path(const char __user *special, int cmd,
> > > +					 struct path *path)
> > > +{
> > > +	struct super_block *sb;
> > > +	int ret;
> > > +
> > > +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > +			   path);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	sb = path->mnt->mnt_sb;
> > > +restart:
> > > +	if (quotactl_cmd_onoff(cmd))
> > > +		down_write(&sb->s_umount);
> > > +	else
> > > +		down_read(&sb->s_umount);
> > > +
> > > +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> > > +		if (quotactl_cmd_onoff(cmd))
> > > +			up_write(&sb->s_umount);
> > > +		else
> > > +			up_read(&sb->s_umount);
> > > +		wait_event(sb->s_writers.wait_unfrozen,
> > > +			   sb->s_writers.frozen == SB_UNFROZEN);
> > > +		goto restart;
> > > +	}
> > > +
> > > +	return sb;
> > > +}
> > 
> > This partial duplicate of __get_super_thawed() guts does *not* belong here,
> > especially not interleaved with quota-specific checks.
> > 
> > > +	if (q_path) {
> > > +		if (quotactl_cmd_onoff(cmd))
> > > +			up_write(&sb->s_umount);
> > > +		else
> > > +			up_read(&sb->s_umount);
> > > +
> > > +		path_put(&sb_path);
> > > +	} else {
> > > +		if (!quotactl_cmd_onoff(cmds))
> > > +			drop_super(sb);
> > > +		else
> > > +			drop_super_exclusive(sb);
> > > +	}
> > 
> > Er...  Why not have the same code that you've used to lock the damn thing
> > (needs to be moved to fs/super.c) simply get a passive ref to it?  Then
> > you could do the same thing, q_path or no q_path...
> 
> I am getting confused here. To an earlier version of this series you
> responded:
> 
> > And for path-based you don't need to mess with superblock
> > references - just keep the struct path until the end.  That
> > will keep the superblock alive and active just fine.
> 
> I did that and got the objection from Jan:
> 
> > So I've realized that just looking up superblock with user_path_at() is not
> > enough. Quota code also expects that the superblock will be locked
> > (sb->s_umount) and filesystem will not be frozen (in case the quota
> > operation is going to modify the filesystem). This is needed to serialize
> > e.g. remount and quota operations or quota operations among themselves.

Yes, using passive reference is not necessary. On the other hand the
symmetry with how get_super() and friends work has some appeal too so if Al
wants that, well, he's the maintainer ;)

> So after drawing circles we now seem to be back at passive references.
> What I have now in my tree is this in fs/super.c, untested currently:

I was thinking how to make the API most sensible. In the end I've decided
for a variant that is attached - we pass in struct path which enforces
active reference to a superblock and thus we don't have to be afraid of the
superblock going away or similar problems. Also the operation "get me
superblock for a path" kind of makes sense...

Guys, what do you think?

								Honza
diff mbox series

Patch

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 5444d3c4d93f..9ef51d02d5a5 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)
@@ -821,15 +822,20 @@  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 file_path, *file_pathp = NULL, sb_path;
 	int ret;
+	bool q_path;
 
 	cmds = cmd >> SUBCMDSHIFT;
 	type = cmd & SUBCMDMASK;
 
+
 	if (type >= MAXQUOTAS)
 		return -EINVAL;
 
+	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
@@ -847,28 +853,45 @@  int kernel_quotactl(unsigned int cmd, const char __user *special,
 	 * resolution (think about autofs) and thus deadlocks could arise.
 	 */
 	if (cmds == Q_QUOTAON) {
-		ret = user_path_at(AT_FDCWD, addr, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &path);
+		ret = user_path_at(AT_FDCWD, addr,
+				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
+				   &file_path);
 		if (ret)
-			pathp = ERR_PTR(ret);
+			file_pathp = ERR_PTR(ret);
 		else
-			pathp = &path;
+			file_pathp = &file_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,
+				   &sb_path);
+		if (ret)
+			goto out;
+
+		sb = sb_path.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);
+	ret = do_quotactl(sb, type, cmds, id, addr, file_pathp);
+
+	if (q_path) {
+		path_put(&sb_path);
+	} else {
+		if (!quotactl_cmd_onoff(cmds))
+			drop_super(sb);
+		else
+			drop_super_exclusive(sb);
+	}
 
-	if (!quotactl_cmd_onoff(cmds))
-		drop_super(sb);
-	else
-		drop_super_exclusive(sb);
 out:
-	if (pathp && !IS_ERR(pathp))
-		path_put(pathp);
+	if (file_pathp && !IS_ERR(file_pathp))
+		path_put(file_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