[05/11] quota: Allow to pass quotactl a mountpoint
diff mbox series

Message ID 20190814121834.13983-6-s.hauer@pengutronix.de
State Changes Requested
Headers show
Series
  • Add quota support to UBIFS
Related show

Commit Message

Sascha Hauer Aug. 14, 2019, 12:18 p.m. UTC
So far quotactl only allows a path to a block device to specify a
filesystem to work on. For filesystems that don't work on block devices
such as UBIFS there is no block device, so let quotactl take the
mountpoint as an alternative way.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/quota/quota.c   | 69 ++++++++++++++++++++++++++++++++--------------
 fs/super.c         | 17 ++++++++++++
 include/linux/fs.h |  1 +
 3 files changed, 67 insertions(+), 20 deletions(-)

Comments

Al Viro Aug. 14, 2019, 11:36 p.m. UTC | #1
On Wed, Aug 14, 2019 at 02:18:28PM +0200, Sascha Hauer wrote:
> +/**
> + * reference_super - get a reference to a given superblock
> + * @sb: superblock to get the reference from
> + *
> + * Takes a reference to a superblock. Can be used as when the superblock
> + * is known and leaves it in a state as if get_super had been called.
> + */
> +void reference_super(struct super_block *sb)
> +{
> +	spin_lock(&sb_lock);
> +	sb->s_count++;
> +	spin_unlock(&sb_lock);
> +
> +	down_read(&sb->s_umount);
> +}
> +EXPORT_SYMBOL_GPL(reference_super);

NAK, for a plenty of reasons

1) introduction of EXPORT_SYMBOL_GPL garbage
2) aforementioned garbage on something that doesn't need to be exported
3) *way* too easily abused - get_super() is, at least, not tempting to
play with in random code.  This one is, and it's too low-level to
allow.
Al Viro Aug. 14, 2019, 11:39 p.m. UTC | #2
On Thu, Aug 15, 2019 at 12:36:32AM +0100, Al Viro wrote:
> On Wed, Aug 14, 2019 at 02:18:28PM +0200, Sascha Hauer wrote:
> > +/**
> > + * reference_super - get a reference to a given superblock
> > + * @sb: superblock to get the reference from
> > + *
> > + * Takes a reference to a superblock. Can be used as when the superblock
> > + * is known and leaves it in a state as if get_super had been called.
> > + */
> > +void reference_super(struct super_block *sb)
> > +{
> > +	spin_lock(&sb_lock);
> > +	sb->s_count++;
> > +	spin_unlock(&sb_lock);
> > +
> > +	down_read(&sb->s_umount);
> > +}
> > +EXPORT_SYMBOL_GPL(reference_super);
> 
> NAK, for a plenty of reasons
> 
> 1) introduction of EXPORT_SYMBOL_GPL garbage
> 2) aforementioned garbage on something that doesn't need to be exported
> 3) *way* too easily abused - get_super() is, at least, not tempting to
> play with in random code.  This one is, and it's too low-level to
> allow.

... and this is a crap userland API.

*IF* you want mountpoint-based variants of quotactl() commands, by all means,
add those.  Do not overload the old ones.  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.
Al Viro Aug. 14, 2019, 11:51 p.m. UTC | #3
On Thu, Aug 15, 2019 at 12:39:46AM +0100, Al Viro wrote:
> > 1) introduction of EXPORT_SYMBOL_GPL garbage
> > 2) aforementioned garbage on something that doesn't need to be exported
> > 3) *way* too easily abused - get_super() is, at least, not tempting to
> > play with in random code.  This one is, and it's too low-level to
> > allow.
> 
> ... and this is a crap userland API.
> 
> *IF* you want mountpoint-based variants of quotactl() commands, by all means,
> add those.  Do not overload the old ones.  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.

	To clarify: I suggest something like #define Q_PATH     0x400000
with users passing something like QCMD(Q_QUOTAON | Q_PATH, ...) instead
of QCMD(Q_QUOTAON, ...) to get a path-based behaviour.
Sascha Hauer Aug. 15, 2019, 7:46 a.m. UTC | #4
On Thu, Aug 15, 2019 at 12:39:46AM +0100, Al Viro wrote:
> On Thu, Aug 15, 2019 at 12:36:32AM +0100, Al Viro wrote:
> > On Wed, Aug 14, 2019 at 02:18:28PM +0200, Sascha Hauer wrote:
> > > +/**
> > > + * reference_super - get a reference to a given superblock
> > > + * @sb: superblock to get the reference from
> > > + *
> > > + * Takes a reference to a superblock. Can be used as when the superblock
> > > + * is known and leaves it in a state as if get_super had been called.
> > > + */
> > > +void reference_super(struct super_block *sb)
> > > +{
> > > +	spin_lock(&sb_lock);
> > > +	sb->s_count++;
> > > +	spin_unlock(&sb_lock);
> > > +
> > > +	down_read(&sb->s_umount);
> > > +}
> > > +EXPORT_SYMBOL_GPL(reference_super);
> > 
> > NAK, for a plenty of reasons
> > 
> > 1) introduction of EXPORT_SYMBOL_GPL garbage
> > 2) aforementioned garbage on something that doesn't need to be exported
> > 3) *way* too easily abused - get_super() is, at least, not tempting to
> > play with in random code.  This one is, and it's too low-level to
> > allow.
> 
> ... and this is a crap userland API.
> 
> *IF* you want mountpoint-based variants of quotactl() commands, by all means,
> add those.  Do not overload the old ones.  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'll happily drop these changes. To clarify, quota currently does:

	if (quotactl_cmd_onoff(cmd))
		sb = get_super_exclusive_thawed(bdev);
	else if (quotactl_cmd_write(cmd))
		sb = get_super_thawed(bdev);
	else
		sb = get_super(bdev);

You are saying that the struct super_block I get from a struct path pointer is
in a suitable state for all the cases above, right?

Sascha
Jan Kara Aug. 15, 2019, 9:53 a.m. UTC | #5
On Thu 15-08-19 00:51:24, Al Viro wrote:
> On Thu, Aug 15, 2019 at 12:39:46AM +0100, Al Viro wrote:
> > > 1) introduction of EXPORT_SYMBOL_GPL garbage
> > > 2) aforementioned garbage on something that doesn't need to be exported
> > > 3) *way* too easily abused - get_super() is, at least, not tempting to
> > > play with in random code.  This one is, and it's too low-level to
> > > allow.
> > 
> > ... and this is a crap userland API.
> > 
> > *IF* you want mountpoint-based variants of quotactl() commands, by all means,
> > add those.  Do not overload the old ones.  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.
> 
> 	To clarify: I suggest something like #define Q_PATH     0x400000
> with users passing something like QCMD(Q_QUOTAON | Q_PATH, ...) instead
> of QCMD(Q_QUOTAON, ...) to get a path-based behaviour.

Yeah, this sounds like a good plan to me. If Sasha plans on using userspace
quota-tools for handling ubifs, some work will be needed there as well but
it's doable.

								Honza

Patch
diff mbox series

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 64a212576a72..6d9807092ae1 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)
@@ -781,16 +782,53 @@  static bool quotactl_cmd_onoff(int cmd)
 		 (cmd == Q_XQUOTAON) || (cmd == Q_XQUOTAOFF);
 }
 
+static struct super_block *quotactl_block(const char __user *special)
+{
+#ifdef CONFIG_BLOCK
+	struct block_device *bdev;
+	struct filename *tmp = getname(special);
+
+	if (IS_ERR(tmp))
+		return ERR_CAST(tmp);
+	bdev = lookup_bdev(tmp->name);
+	putname(tmp);
+	if (IS_ERR(bdev))
+		return ERR_CAST(bdev);
+
+	bdput(bdev);
+
+	return get_super(bdev);
+#else
+	return ERR_PTR(-ENODEV);
+#endif
+}
+
+static struct super_block *quotactl_path(const char __user *special)
+{
+	struct super_block *sb;
+	struct path path;
+	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;
+
+	reference_super(sb);
+
+	path_put(&path);
+
+	return sb;
+}
+
 /*
  * look up a superblock on which quota ops will be performed
  * - use the name of a block device to find the superblock thereon
  */
-static struct super_block *quotactl_block(const char __user *special, int cmd)
+static struct super_block *quotactl_get_super(const char __user *special, int cmd)
 {
-#ifdef CONFIG_BLOCK
-	struct block_device *bdev;
 	struct super_block *sb;
-	struct filename *tmp = getname(special);
 	bool thawed = false, exclusive;
 	int ret;
 
@@ -802,18 +840,12 @@  static struct super_block *quotactl_block(const char __user *special, int cmd)
 		exclusive = false;
 	}
 
-	if (IS_ERR(tmp))
-		return ERR_CAST(tmp);
-	bdev = lookup_bdev(tmp->name);
-	putname(tmp);
-	if (IS_ERR(bdev))
-		return ERR_CAST(bdev);
-
-	bdput(bdev);
-
-	sb = get_super(bdev);
-	if (!sb)
-		return ERR_PTR(-ENODEV);
+	sb = quotactl_block(special);
+	if (IS_ERR(sb)) {
+		sb = quotactl_path(special);
+		if (IS_ERR(sb))
+			return ERR_CAST(sb);
+	}
 
 	if (thawed) {
 		ret = wait_super_thawed(sb, exclusive);
@@ -827,9 +859,6 @@  static struct super_block *quotactl_block(const char __user *special, int cmd)
 	}
 
 	return sb;
-#else
-	return ERR_PTR(-ENODEV);
-#endif
 }
 
 /*
@@ -873,7 +902,7 @@  int kernel_quotactl(unsigned int cmd, const char __user *special,
 			pathp = &path;
 	}
 
-	sb = quotactl_block(special, cmds);
+	sb = quotactl_get_super(special, cmds);
 	if (IS_ERR(sb)) {
 		ret = PTR_ERR(sb);
 		goto out;
diff --git a/fs/super.c b/fs/super.c
index 13380ffcbd8d..b23641f94557 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -819,6 +819,23 @@  int wait_super_thawed(struct super_block *sb, bool excl)
 }
 EXPORT_SYMBOL(wait_super_thawed);
 
+/**
+ * reference_super - get a reference to a given superblock
+ * @sb: superblock to get the reference from
+ *
+ * Takes a reference to a superblock. Can be used as when the superblock
+ * is known and leaves it in a state as if get_super had been called.
+ */
+void reference_super(struct super_block *sb)
+{
+	spin_lock(&sb_lock);
+	sb->s_count++;
+	spin_unlock(&sb_lock);
+
+	down_read(&sb->s_umount);
+}
+EXPORT_SYMBOL_GPL(reference_super);
+
 /**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 59d9ea62942a..66aa9e5490d0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3223,6 +3223,7 @@  extern struct super_block *get_super(struct block_device *);
 extern int wait_super_thawed(struct super_block *sb, bool excl);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern int super_wait_thawed(struct super_block *sb, bool excl);
+extern void reference_super(struct super_block *sb);
 extern void drop_super(struct super_block *sb);
 extern void drop_super_exclusive(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);