[PATCH] xfs: fix Q_XQUOTARM ioctl

[PATCH] xfs: fix Q_XQUOTARM ioctl

[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.

This has been broken more or less forever, AFAICT.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index af33caf..6f14b8f 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[Christoph Hellwig]

On Mon, Apr 21, 2014 at 03:33:37PM -0500, Eric Sandeen wrote:
> 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.

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[Eric Sandeen]

On 4/22/14, 1:43 AM, Christoph Hellwig wrote:
> On Mon, Apr 21, 2014 at 03:33:37PM -0500, Eric Sandeen wrote:
>> 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.
> 
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] xfs: fix Q_XQUOTARM ioctl

[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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 2b363e2..ff3f0b3 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -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:
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index af33caf..2ad1b98 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -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,
 };
diff --git a/include/linux/quota.h b/include/linux/quota.h
index cc7494a..0f3c5d3 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: fix Q_XQUOTARM ioctl

[Dave Chinner]

On Tue, Apr 22, 2014 at 01:48:38PM -0500, Eric Sandeen wrote:
> 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.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: fix Q_XQUOTARM ioctl

[Jan Kara]

On Wed 23-04-14 07:30:51, Dave Chinner wrote:
> On Tue, Apr 22, 2014 at 01:48:38PM -0500, Eric Sandeen wrote:
> > 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.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> 
> 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 <jack@suse.cz>

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: fix Q_XQUOTARM ioctl

[Christoph Hellwig]

On Tue, Apr 22, 2014 at 01:48:38PM -0500, Eric Sandeen wrote:
> 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.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good for now:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: fix Q_XQUOTARM ioctl

[Eric Sandeen]

On 4/23/14, 10:27 AM, Christoph Hellwig wrote:
> On Tue, Apr 22, 2014 at 01:48:38PM -0500, Eric Sandeen wrote:
>> 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.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Looks good for now:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs: fix Q_XQUOTARM ioctl

[Christoph Hellwig]

On Wed, Apr 23, 2014 at 10:28:58AM -0500, Eric Sandeen wrote:
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[Christoph Hellwig]

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[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.

-Eric

On 5/3/14, 6:48 AM, Christoph Hellwig wrote:
> 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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[Christoph Hellwig]

On Sat, May 03, 2014 at 09:36:05AM -0500, Eric Sandeen wrote:
> 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?

> 
> -Eric
> 
> On 5/3/14, 6:48 AM, Christoph Hellwig wrote:
> > 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
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix Q_XQUOTARM ioctl

[Dave Chinner]

On Sat, May 03, 2014 at 08:14:40AM -0700, Christoph Hellwig wrote:
> On Sat, May 03, 2014 at 09:36:05AM -0500, Eric Sandeen wrote:
> > 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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs