xfs: fix Q_XQUOTARM ioctl

PATCH In review Eric Sandeen 5 participants 13 messages Apr 21, 2014 – May 4, 2014

Subsystems (2)

XFS filesystem Diskquota

lore

Share & tools

lore

Eric Sandeen

author

2014-04-21 20:33 UTC #0 lore raw

PATCH 1/1 xfs: fix Q_XQUOTARM ioctl

The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.

Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.

Split out the flag conversion from userspace to kernelspace, depending on the quotactl that is being executed; add 2 new helpers to do this, so that we send the right flags to the lower-level functions.

This has been broken more or less forever, AFAICT.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_quotaops.c +39 ~~-11~~

index af33caf..6f14b8f 100644
@@ -66,19 +66,11 @@ xfs_fs_get_xstatev(
 	return -xfs_qm_scall_getqstatv(mp, fqs);
 }
 
+/* Convert quotactl flags to internal flags for Q_XQUOTAON/OFF */
 STATIC int
-xfs_fs_set_xstate(
-	struct super_block	*sb,
-	unsigned int		uflags,
-	int			op)
+xfs_qm_import_flags(unsigned int uflags)
 {
-	struct xfs_mount	*mp = XFS_M(sb);
-	unsigned int		flags = 0;
-
-	if (sb->s_flags & MS_RDONLY)
-		return -EROFS;
-	if (op != Q_XQUOTARM && !XFS_IS_QUOTA_RUNNING(mp))
-		return -ENOSYS;
+	unsigned int flags = 0;
 
 	if (uflags & FS_QUOTA_UDQ_ACCT)
 		flags |= XFS_UQUOTA_ACCT;

@@ -93,16 +85,52 @@ xfs_fs_set_xstate(
 	if (uflags & FS_QUOTA_PDQ_ENFD)
 		flags |= XFS_PQUOTA_ENFD;
 
+	return flags;
+}
+
+/* Convert quotactl flags to internal flags for Q_XQUOTARM */
+STATIC int
+xfs_qm_import_qtype_flags(unsigned int uflags)
+{
+	unsigned int flags = 0;
+
+	if (uflags & FS_USER_QUOTA)
+		flags |= XFS_DQ_USER;
+	if (uflags & FS_GROUP_QUOTA)
+		flags |= XFS_DQ_GROUP;
+	if (uflags & FS_USER_QUOTA)
+		flags |= XFS_DQ_PROJ;
+
+	return flags;
+}
+
+STATIC int
+xfs_fs_set_xstate(
+	struct super_block	*sb,
+	unsigned int		uflags,
+	int			op)
+{
+	struct xfs_mount	*mp = XFS_M(sb);
+	unsigned int		flags;
+
+	if (sb->s_flags & MS_RDONLY)
+		return -EROFS;
+	if (op != Q_XQUOTARM && !XFS_IS_QUOTA_RUNNING(mp))
+		return -ENOSYS;
+
 	switch (op) {
 	case Q_XQUOTAON:
+		flags = xfs_qm_import_flags(uflags);
 		return -xfs_qm_scall_quotaon(mp, flags);
 	case Q_XQUOTAOFF:
 		if (!XFS_IS_QUOTA_ON(mp))
 			return -EINVAL;
+		flags = xfs_qm_import_flags(uflags);
 		return -xfs_qm_scall_quotaoff(mp, flags);
 	case Q_XQUOTARM:
 		if (XFS_IS_QUOTA_ON(mp))
 			return -EINVAL;
+		flags = xfs_qm_import_qtype_flags(uflags);
 		return -xfs_qm_scall_trunc_qfiles(mp, flags);
 	}

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-04-22 06:43 UTC #1 lore raw

Quoted 12 lines · Eric Sandeen

The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.

Quoted 6 lines

Split out the flag conversion from userspace to kernelspace, depending on the quotactl that is being executed; add 2 new helpers to do this, so that we send the right flags to the lower-level functions.
This has been broken more or less forever, AFAICT.

So if this isn't very urgent I'd rather replace ->set_xstate with one operation per ioctl, so that we don't multiple different kinds of flags over the same operation.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2014-04-22 15:54 UTC #2 lore raw

Quoted 18 lines · Christoph Hellwig

Quoted 17 lines · Eric Sandeen
The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.
Split out the flag conversion from userspace to kernelspace, depending on the quotactl that is being executed; add 2 new helpers to do this, so that we send the right flags to the lower-level functions.

Quoted 6 lines

Quoted 1 line
This has been broken more or less forever, AFAICT.
So if this isn't very urgent I'd rather replace ->set_xstate with one operation per ioctl, so that we don't multiple different kinds of flags over the same operation.

eh, that's probably better, I'll take a look.

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-04-22 18:48 UTC #3 lore raw

PATCH v2 1/1 xfs: fix Q_XQUOTARM ioctl

In reply to Eric Sandeen

Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.

Add a new quotactl op vector specifically for the QUOTARM operation, since it operates with a different flag space.

This has been broken more or less forever, AFAICT.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/quota/quota.c +13 -1

index 2b363e2..ff3f0b3 100644
@@ -278,6 +278,17 @@ static int quota_getxquota(struct super_block *sb, int type, qid_t id,
 	return ret;
 }
 
+static int quota_rmxquota(struct super_block *sb, void __user *addr)
+{
+	__u32 flags;
+
+	if (copy_from_user(&flags, addr, sizeof(flags)))
+		return -EFAULT;
+	if (!sb->s_qcop->rm_xquota)
+		return -ENOSYS;
+	return sb->s_qcop->rm_xquota(sb, flags);
+}
+
 /* Copy parameters and call proper function */
 static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 		       void __user *addr, struct path *path)

@@ -316,8 +327,9 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 		return sb->s_qcop->quota_sync(sb, type);
 	case Q_XQUOTAON:
 	case Q_XQUOTAOFF:
-	case Q_XQUOTARM:
 		return quota_setxstate(sb, cmd, addr);
+	case Q_XQUOTARM:
+		return quota_rmxquota(sb, addr);
 	case Q_XGETQSTAT:
 		return quota_getxstate(sb, addr);
 	case Q_XGETQSTATV:

fs/xfs/xfs_quotaops.c +25 -4

index af33caf..2ad1b98 100644
@@ -100,16 +100,36 @@ xfs_fs_set_xstate(
 		if (!XFS_IS_QUOTA_ON(mp))
 			return -EINVAL;
 		return -xfs_qm_scall_quotaoff(mp, flags);
-	case Q_XQUOTARM:
-		if (XFS_IS_QUOTA_ON(mp))
-			return -EINVAL;
-		return -xfs_qm_scall_trunc_qfiles(mp, flags);
 	}
 
 	return -EINVAL;
 }
 
 STATIC int
+xfs_fs_rm_xquota(
+	struct super_block	*sb,
+	unsigned int		uflags)
+{
+	struct xfs_mount	*mp = XFS_M(sb);
+	unsigned int		flags = 0;
+	
+	if (sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
+	if (XFS_IS_QUOTA_ON(mp))
+		return -EINVAL;
+
+	if (uflags & FS_USER_QUOTA)
+		flags |= XFS_DQ_USER;
+	if (uflags & FS_GROUP_QUOTA)
+		flags |= XFS_DQ_GROUP;
+	if (uflags & FS_USER_QUOTA)
+		flags |= XFS_DQ_PROJ;
+
+	return -xfs_qm_scall_trunc_qfiles(mp, flags);
+}	
+
+STATIC int
 xfs_fs_get_dqblk(
 	struct super_block	*sb,
 	struct kqid		qid,

@@ -149,6 +169,7 @@ const struct quotactl_ops xfs_quotactl_operations = {
 	.get_xstatev		= xfs_fs_get_xstatev,
 	.get_xstate		= xfs_fs_get_xstate,
 	.set_xstate		= xfs_fs_set_xstate,
+	.rm_xquota		= xfs_fs_rm_xquota,
 	.get_dqblk		= xfs_fs_get_dqblk,
 	.set_dqblk		= xfs_fs_set_dqblk,
 };

include/linux/quota.h +1 -0

index cc7494a..0f3c5d3 100644
@@ -329,6 +329,7 @@ struct quotactl_ops {
 	int (*get_xstate)(struct super_block *, struct fs_quota_stat *);
 	int (*set_xstate)(struct super_block *, unsigned int, int);
 	int (*get_xstatev)(struct super_block *, struct fs_quota_statv *);
+	int (*rm_xquota)(struct super_block *, unsigned int);
 };
 
 struct quota_format_type {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2014-04-22 21:30 UTC #4 lore raw

Quoted 12 lines · Eric Sandeen

The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.

Quoted 7 lines

Add a new quotactl op vector specifically for the QUOTARM operation, since it operates with a different flag space.
This has been broken more or less forever, AFAICT.
Signed-off-by Eric Sandeen <[email protected]>
---

Looks fine to me, but I'll need Jan to ack it before I pull it in.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jan Kara

Acked-by

2014-04-22 21:48 UTC #5 lore raw

Quoted 16 lines · Dave Chinner

Quoted 15 lines · Eric Sandeen
The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.
Add a new quotactl op vector specifically for the QUOTARM operation, since it operates with a different flag space.

Quoted 6 lines

Quoted 4 lines
This has been broken more or less forever, AFAICT.
Signed-off-by Eric Sandeen <[email protected]>
---
Looks fine to me, but I'll need Jan to ack it before I pull it in.

The patch looks good to me as well. So feel free to add:

Acked-by Jan Kara <[email protected]>

Honza

Signature · 7 lines

Jan Kara <[email protected]>
SUSE Labs, CR

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by on v2 1/1

2014-04-23 15:27 UTC #6 lore raw

In reply to Eric Sandeen

Quoted 12 lines · Eric Sandeen

The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.

Quoted 6 lines

Add a new quotactl op vector specifically for the QUOTARM operation, since it operates with a different flag space.
This has been broken more or less forever, AFAICT.
Signed-off-by Eric Sandeen <[email protected]>

Looks good for now:

Reviewed-by Christoph Hellwig <[email protected]>

If you have a spare cycle or two I think splitting quotaon and quotaoff might not be an all that bad idea either.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-04-23 15:28 UTC #7 lore raw

Quoted 18 lines · Christoph Hellwig

Quoted 17 lines · Eric Sandeen
The Q_XQUOTARM quotactl was not working properly, because we weren't passing around proper flags. The xfs_fs_set_xstate() ioctl handler used the same flags for Q_XQUOTAON/OFF as well as for Q_XQUOTARM, but Q_XQUOTAON/OFF look for XFS_UQUOTA_ACCT, XFS_UQUOTA_ENFD, XFS_GQUOTA_ACCT etc, i.e. quota type + state, while Q_XQUOTARM looks only for the type of quota, i.e. XFS_DQ_USER, XFS_DQ_GROUP etc.
Unfortunately these flag spaces overlap a bit, so we got semi-random results for Q_XQUOTARM; i.e. the value for XFS_DQ_USER == XFS_UQUOTA_ACCT, etc. yeargh.
Add a new quotactl op vector specifically for the QUOTARM operation, since it operates with a different flag space.
This has been broken more or less forever, AFAICT.

Quoted 8 lines

Quoted 1 line
Signed-off-by Eric Sandeen <[email protected]>
Looks good for now:
Reviewed-by Christoph Hellwig <[email protected]>
If you have a spare cycle or two I think splitting quotaon and quotaoff might not be an all that bad idea either.

Just out of curiousity, for what reason - just parity w/ the non-xfs ops?

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-04-23 15:39 UTC #8 lore raw

Quoted 2 lines · Eric Sandeen

Just out of curiousity, for what reason - just parity w/ the non-xfs ops?

Because it's just cleaner to have one methods for one operation instead of overloading them with different opts like an ioctl.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-05-03 11:48 UTC #9 lore raw

In reply to Eric Sandeen

Can I assumes that xfs/007 failing with:

--- tests/xfs/007.out 2014-05-03 08:58:08.000000000 +0000
+++ /root/xfstests/results//xfs/007.out.bad 2014-05-03
11:48:05.000000000 +0000
@@ -7,4 +7,4 @@
*** umount
*** Usage after quotarm ***
core.nblocks = 0
-core.nblocks = 0
+core.nblocks = 1

is because this patch didn't get picked up yet?

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-05-03 14:36 UTC #10 lore raw

Whoops, missed reply-all.

Yes, it will fail this way w/o the kernel patch.

User quota gets removed OK because flags accidentally line up; group quota remains, so you'd see exactly this.

-Eric

Quoted 12 lines · Christoph Hellwig

Can I assumes that xfs/007 failing with:
root/xfstests/results//xfs/007.out.bad renamed from xfs/007.out +0 -0
11:48:05.000000000 +0000
root/xfstests/results//xfs/007.out.bad renamed from xfs/007.out +1 -1
@@ -7,4 +7,4 @@
 *** umount
 *** Usage after quotarm ***
 core.nblocks = 0
-core.nblocks = 0
+core.nblocks = 1

Quoted 7 lines

is because this patch didn't get picked up yet?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2014-05-03 15:14 UTC #11 lore raw

Quoted 6 lines · Eric Sandeen

Whoops, missed reply-all.
Yes, it will fail this way w/o the kernel patch.
User quota gets removed OK because flags accidentally line up; group quota remains, so you'd see exactly this.

Ok. Dave, can you pick it up for for-next?

Quoted 18 lines

-Eric
Quoted 14 lines · Christoph Hellwig
Can I assumes that xfs/007 failing with:
root/xfstests/results//xfs/007.out.bad renamed from xfs/007.out +0 -0
11:48:05.000000000 +0000
root/xfstests/results//xfs/007.out.bad renamed from xfs/007.out +1 -1
@@ -7,4 +7,4 @@
 *** umount
 *** Usage after quotarm ***
 core.nblocks = 0
-core.nblocks = 0
+core.nblocks = 1
is because this patch didn't get picked up yet?

Quoted 10 lines

Quoted 5 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

---end quoted text---

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2014-05-04 00:21 UTC #12 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 6 lines · Eric Sandeen
Whoops, missed reply-all.
Yes, it will fail this way w/o the kernel patch.
User quota gets removed OK because flags accidentally line up; group quota remains, so you'd see exactly this.
Ok. Dave, can you pick it up for for-next?

It's in my queue - I've been trying to get everything sorted for a 3.15-rc4 update first before pushing more 3.16 changes into for-next. That's been going round in circles on the truncate_setsize() issue, but I think that is sorted now so it'll be done soon. ;)

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs