[PATCH 0/4] Quota cleanups

[PATCH 0/4] Quota cleanups

[Jan Kara]

  Hello,

  these are 4 quota code cleanup patches from my series unifying VFS and
XFS quota interfaces. If noone objects, I'll add them to my tree in a few
days.

								Honza

[PATCH 1/4] quota: Don't store flags for v2 quota format

[Jan Kara]

Currently, v2 quota format blindly stored flags from in-memory dqinfo on
disk, although there are no flags supported. Since it is stupid to store
flags which have no effect, just store 0 unconditionally and don't
bother loading it from disk.

Note that userspace could have stored some flags there via Q_SETINFO
quotactl and then later read them (although flags have no effect) but
I'm pretty sure noone does that (most definitely quota-tools don't and
quota interface doesn't have too much other users).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/quota_v2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/quota/quota_v2.c b/fs/quota/quota_v2.c
index 02751ec695c5..b291602e1193 100644
--- a/fs/quota/quota_v2.c
+++ b/fs/quota/quota_v2.c
@@ -126,7 +126,8 @@ static int v2_read_file_info(struct super_block *sb, int type)
 	}
 	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
 	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
-	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
+	/* No flags currently supported */
+	info->dqi_flags = le32_to_cpu(0);
 	qinfo->dqi_sb = sb;
 	qinfo->dqi_type = type;
 	qinfo->dqi_blocks = le32_to_cpu(dinfo.dqi_blocks);
@@ -157,7 +158,8 @@ static int v2_write_file_info(struct super_block *sb, int type)
 	info->dqi_flags &= ~DQF_INFO_DIRTY;
 	dinfo.dqi_bgrace = cpu_to_le32(info->dqi_bgrace);
 	dinfo.dqi_igrace = cpu_to_le32(info->dqi_igrace);
-	dinfo.dqi_flags = cpu_to_le32(info->dqi_flags & DQF_MASK);
+	/* No flags currently supported */
+	dinfo.dqi_flags = 0;
 	spin_unlock(&dq_data_lock);
 	dinfo.dqi_blocks = cpu_to_le32(qinfo->dqi_blocks);
 	dinfo.dqi_free_blk = cpu_to_le32(qinfo->dqi_free_blk);
-- 
2.1.2

Re: [PATCH 1/4] quota: Don't store flags for v2 quota format

[Christoph Hellwig]

On Wed, Jan 14, 2015 at 07:27:08PM +0100, Jan Kara wrote:
> Currently, v2 quota format blindly stored flags from in-memory dqinfo on
> disk, although there are no flags supported. Since it is stupid to store
> flags which have no effect, just store 0 unconditionally and don't
> bother loading it from disk.
> 
> Note that userspace could have stored some flags there via Q_SETINFO
> quotactl and then later read them (although flags have no effect) but
> I'm pretty sure noone does that (most definitely quota-tools don't and
> quota interface doesn't have too much other users).

What about future proofing?  Current kernels can store flags on disk,
so the best is to reserve the currently (and possibly previously)
assigned values, and mask them out when reading from disk.

Re: [PATCH 1/4] quota: Don't store flags for v2 quota format

[Jan Kara]

On Thu 15-01-15 01:40:34, Christoph Hellwig wrote:
> On Wed, Jan 14, 2015 at 07:27:08PM +0100, Jan Kara wrote:
> > Currently, v2 quota format blindly stored flags from in-memory dqinfo on
> > disk, although there are no flags supported. Since it is stupid to store
> > flags which have no effect, just store 0 unconditionally and don't
> > bother loading it from disk.
> > 
> > Note that userspace could have stored some flags there via Q_SETINFO
> > quotactl and then later read them (although flags have no effect) but
> > I'm pretty sure noone does that (most definitely quota-tools don't and
> > quota interface doesn't have too much other users).
> 
> What about future proofing?  Current kernels can store flags on disk,
> so the best is to reserve the currently (and possibly previously)
> assigned values, and mask them out when reading from disk.
  Hum, I'm not sure I follow you. Current kernels will store any 32-bit
number user sets in flags field. So if we wanted to be 100% safe, we'd have
to just ignore that field. Which isn't currently a problem since quota code
doesn't use the field for anything (it was added just for future
extensions). But since I'm pretty certain noone actually relies on values
of that field, I though we could just get away with forcibly zeroing the
field now and if there's a need to use the field in a few years, we could
start using it.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/4] quota: Don't store flags for v2 quota format

[Christoph Hellwig]

On Thu, Jan 15, 2015 at 11:13:10AM +0100, Jan Kara wrote:
>   Hum, I'm not sure I follow you. Current kernels will store any 32-bit
> number user sets in flags field. So if we wanted to be 100% safe, we'd have
> to just ignore that field. Which isn't currently a problem since quota code
> doesn't use the field for anything (it was added just for future
> extensions). But since I'm pretty certain noone actually relies on values
> of that field, I though we could just get away with forcibly zeroing the
> field now and if there's a need to use the field in a few years, we could
> start using it.

Oh, I misread the code and your description.  I thought we would just
store any potentially valid in-core flag on disk.

I guess for now the best case would be to stop storing anything, and
then just make an educated decision if/when we need a flags field.

Re: [PATCH 1/4] quota: Don't store flags for v2 quota format

[Jan Kara]

On Mon 19-01-15 01:14:32, Christoph Hellwig wrote:
> On Thu, Jan 15, 2015 at 11:13:10AM +0100, Jan Kara wrote:
> >   Hum, I'm not sure I follow you. Current kernels will store any 32-bit
> > number user sets in flags field. So if we wanted to be 100% safe, we'd have
> > to just ignore that field. Which isn't currently a problem since quota code
> > doesn't use the field for anything (it was added just for future
> > extensions). But since I'm pretty certain noone actually relies on values
> > of that field, I though we could just get away with forcibly zeroing the
> > field now and if there's a need to use the field in a few years, we could
> > start using it.
> 
> Oh, I misread the code and your description.  I thought we would just
> store any potentially valid in-core flag on disk.
> 
> I guess for now the best case would be to stop storing anything, and
> then just make an educated decision if/when we need a flags field.
  Yes, that's what I do in the patch. So we are in agreement here.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 2/4] ocfs2: Move OLQF_CLEAN flag out of generic quota flags

[Jan Kara]

OLQF_CLEAN flag is used by OCFS2 on disk to recognize whether quota
recovery is needed or not. We also somewhat abuse mem_dqinfo->dqi_flags
to pass this flag around. Use private flags for this to avoid clashes
with other quota flags / not pollute generic quota flag namespace.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/quota.h       |  1 +
 fs/ocfs2/quota_local.c | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
index 1eae330193a6..b6d51333ad02 100644
--- a/fs/ocfs2/quota.h
+++ b/fs/ocfs2/quota.h
@@ -48,6 +48,7 @@ struct ocfs2_quota_recovery {
 /* In-memory structure with quota header information */
 struct ocfs2_mem_dqinfo {
 	unsigned int dqi_type;		/* Quota type this structure describes */
+	unsigned int dqi_flags;		/* Flags OLQF_* */
 	unsigned int dqi_chunks;	/* Number of chunks in local quota file */
 	unsigned int dqi_blocks;	/* Number of blocks allocated for local quota file */
 	unsigned int dqi_syncms;	/* How often should we sync with other nodes */
diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
index 10b653930ee2..55f832f553cb 100644
--- a/fs/ocfs2/quota_local.c
+++ b/fs/ocfs2/quota_local.c
@@ -292,7 +292,7 @@ static void olq_update_info(struct buffer_head *bh, void *private)
 	ldinfo = (struct ocfs2_local_disk_dqinfo *)(bh->b_data +
 						OCFS2_LOCAL_INFO_OFF);
 	spin_lock(&dq_data_lock);
-	ldinfo->dqi_flags = cpu_to_le32(info->dqi_flags & DQF_MASK);
+	ldinfo->dqi_flags = cpu_to_le32(oinfo->dqi_flags);
 	ldinfo->dqi_chunks = cpu_to_le32(oinfo->dqi_chunks);
 	ldinfo->dqi_blocks = cpu_to_le32(oinfo->dqi_blocks);
 	spin_unlock(&dq_data_lock);
@@ -737,13 +737,13 @@ static int ocfs2_local_read_info(struct super_block *sb, int type)
 	}
 	ldinfo = (struct ocfs2_local_disk_dqinfo *)(bh->b_data +
 						OCFS2_LOCAL_INFO_OFF);
-	info->dqi_flags = le32_to_cpu(ldinfo->dqi_flags);
+	oinfo->dqi_flags = le32_to_cpu(ldinfo->dqi_flags);
 	oinfo->dqi_chunks = le32_to_cpu(ldinfo->dqi_chunks);
 	oinfo->dqi_blocks = le32_to_cpu(ldinfo->dqi_blocks);
 	oinfo->dqi_libh = bh;
 
 	/* We crashed when using local quota file? */
-	if (!(info->dqi_flags & OLQF_CLEAN)) {
+	if (!(oinfo->dqi_flags & OLQF_CLEAN)) {
 		rec = OCFS2_SB(sb)->quota_rec;
 		if (!rec) {
 			rec = ocfs2_alloc_quota_recovery();
@@ -772,7 +772,7 @@ static int ocfs2_local_read_info(struct super_block *sb, int type)
 	}
 
 	/* Now mark quota file as used */
-	info->dqi_flags &= ~OLQF_CLEAN;
+	oinfo->dqi_flags &= ~OLQF_CLEAN;
 	status = ocfs2_modify_bh(lqinode, bh, olq_update_info, info);
 	if (status < 0) {
 		mlog_errno(status);
@@ -857,7 +857,7 @@ static int ocfs2_local_free_info(struct super_block *sb, int type)
 		goto out;
 
 	/* Mark local file as clean */
-	info->dqi_flags |= OLQF_CLEAN;
+	oinfo->dqi_flags |= OLQF_CLEAN;
 	status = ocfs2_modify_bh(sb_dqopt(sb)->files[type],
 				 oinfo->dqi_libh,
 				 olq_update_info,
-- 
2.1.2

[PATCH 3/4] quota: Cleanup flags definitions

[Jan Kara]

Currently all quota flags were defined just in kernel-private headers.
Export flags readable / writeable from userspace to userspace via
include/uapi/linux/quota.h.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c           |  2 +-
 include/linux/dqblk_v1.h   |  3 ---
 include/linux/quota.h      | 14 ++++++++------
 include/uapi/linux/quota.h | 14 +++++++++++++-
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 8f0acef3d184..f8be368b9086 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1248,7 +1248,7 @@ static int ignore_hardlimit(struct dquot *dquot)
 
 	return capable(CAP_SYS_RESOURCE) &&
 	       (info->dqi_format->qf_fmt_id != QFMT_VFS_OLD ||
-		!(info->dqi_flags & V1_DQF_RSQUASH));
+		!(info->dqi_flags & DQF_ROOT_SQUASH));
 }
 
 /* needs dq_data_lock */
diff --git a/include/linux/dqblk_v1.h b/include/linux/dqblk_v1.h
index 3713a7232dd8..c0d4d1e2a45c 100644
--- a/include/linux/dqblk_v1.h
+++ b/include/linux/dqblk_v1.h
@@ -5,9 +5,6 @@
 #ifndef _LINUX_DQBLK_V1_H
 #define _LINUX_DQBLK_V1_H
 
-/* Root squash turned on */
-#define V1_DQF_RSQUASH 1
-
 /* Numbers of blocks needed for updates */
 #define V1_INIT_ALLOC 1
 #define V1_INIT_REWRITE 1
diff --git a/include/linux/quota.h b/include/linux/quota.h
index 50978b781a19..0c42113607ce 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -223,12 +223,14 @@ struct mem_dqinfo {
 
 struct super_block;
 
-#define DQF_MASK 0xffff		/* Mask for format specific flags */
-#define DQF_GETINFO_MASK 0x1ffff	/* Mask for flags passed to userspace */
-#define DQF_SETINFO_MASK 0xffff		/* Mask for flags modifiable from userspace */
-#define DQF_SYS_FILE_B		16
-#define DQF_SYS_FILE (1 << DQF_SYS_FILE_B)	/* Quota file stored as system file */
-#define DQF_INFO_DIRTY_B	31
+/* Mask for flags passed to userspace */
+#define DQF_GETINFO_MASK (DQF_ROOT_SQUASH | DQF_SYS_FILE)
+/* Mask for flags modifiable from userspace */
+#define DQF_SETINFO_MASK DQF_ROOT_SQUASH
+
+enum {
+	DQF_INFO_DIRTY_B = DQF_PRIVATE,
+};
 #define DQF_INFO_DIRTY (1 << DQF_INFO_DIRTY_B)	/* Is info dirty? */
 
 extern void mark_info_dirty(struct super_block *sb, int type);
diff --git a/include/uapi/linux/quota.h b/include/uapi/linux/quota.h
index 3b6cfbeb086d..1f49b8341c99 100644
--- a/include/uapi/linux/quota.h
+++ b/include/uapi/linux/quota.h
@@ -126,10 +126,22 @@ struct if_dqblk {
 #define IIF_FLAGS	4
 #define IIF_ALL		(IIF_BGRACE | IIF_IGRACE | IIF_FLAGS)
 
+enum {
+	DQF_ROOT_SQUASH_B = 0,
+	DQF_SYS_FILE_B = 16,
+	/* Kernel internal flags invisible to userspace */
+	DQF_PRIVATE
+};
+
+/* Root squash enabled (for v1 quota format) */
+#define DQF_ROOT_SQUASH	(1 << DQF_ROOT_SQUASH_B)
+/* Quota stored in a system file */
+#define DQF_SYS_FILE	(1 << DQF_SYS_FILE_B)
+
 struct if_dqinfo {
 	__u64 dqi_bgrace;
 	__u64 dqi_igrace;
-	__u32 dqi_flags;
+	__u32 dqi_flags;	/* DFQ_* */
 	__u32 dqi_valid;
 };
 
-- 
2.1.2

[PATCH 4/4] quota: Verify flags passed to Q_SETINFO

[Jan Kara]

Currently flags passed via Q_SETINFO were just stored. This makes it
hard to add new flags since in theory userspace could be just setting /
clearing random flags. Since currently there is only one userspace
settable flag and that is somewhat obscure flags only for ancient v1
quota format, I'm reasonably sure noone operates these flags and
hopefully we are fine just adding the check that passed flags are sane.
If we indeed find some userspace program that gets broken by the strict
check, we can always remove it again.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index f8be368b9086..d25c3243c196 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2582,6 +2582,14 @@ int dquot_set_dqinfo(struct super_block *sb, int type, struct if_dqinfo *ii)
 		goto out;
 	}
 	mi = sb_dqopt(sb)->info + type;
+	if (ii->dqi_valid & IIF_FLAGS) {
+		if (ii->dqi_flags & ~DQF_SETINFO_MASK ||
+		    (ii->dqi_flags & DQF_ROOT_SQUASH &&
+		     mi->dqi_format->qf_fmt_id != QFMT_VFS_OLD)) {
+			err = -EINVAL;
+			goto out;
+		}
+	}
 	spin_lock(&dq_data_lock);
 	if (ii->dqi_valid & IIF_BGRACE)
 		mi->dqi_bgrace = ii->dqi_bgrace;
-- 
2.1.2