[PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Soon, XFS will support quota grace period expiration timestamps beyond
the year 2038, widen the timestamp fields to handle the extra time bits.
Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
bits here should work fine.  (Note that XFS is the only user of this
structure.)

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: use __s8 for timestamp extension
---
 fs/quota/quota.c               |   43 +++++++++++++++++++++++++++++++++++-----
 include/uapi/linux/dqblk_xfs.h |   11 +++++++++-
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 5444d3c4d93f..eefac57c52fd 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
 	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
 }
 
+static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
+		__s32 timer, __s8 timer_hi)
+{
+	if (d->d_fieldmask & FS_DQ_BIGTIME)
+		return (u32)timer | (s64)timer_hi << 32;
+	return timer;
+}
+
 static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
 {
 	dst->d_spc_hardlimit = quota_bbtob(src->d_blk_hardlimit);
@@ -489,14 +497,18 @@ static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
 	dst->d_ino_softlimit = src->d_ino_softlimit;
 	dst->d_space = quota_bbtob(src->d_bcount);
 	dst->d_ino_count = src->d_icount;
-	dst->d_ino_timer = src->d_itimer;
-	dst->d_spc_timer = src->d_btimer;
+	dst->d_ino_timer = copy_from_xfs_dqblk_ts(src, src->d_itimer,
+						  src->d_itimer_hi);
+	dst->d_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_btimer,
+						  src->d_btimer_hi);
 	dst->d_ino_warns = src->d_iwarns;
 	dst->d_spc_warns = src->d_bwarns;
 	dst->d_rt_spc_hardlimit = quota_bbtob(src->d_rtb_hardlimit);
 	dst->d_rt_spc_softlimit = quota_bbtob(src->d_rtb_softlimit);
 	dst->d_rt_space = quota_bbtob(src->d_rtbcount);
 	dst->d_rt_spc_timer = src->d_rtbtimer;
+	dst->d_rt_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_rtbtimer,
+						     src->d_rtbtimer_hi);
 	dst->d_rt_spc_warns = src->d_rtbwarns;
 	dst->d_fieldmask = 0;
 	if (src->d_fieldmask & FS_DQ_ISOFT)
@@ -588,10 +600,28 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
 	return sb->s_qcop->set_dqblk(sb, qid, &qdq);
 }
 
+static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
+		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
+{
+	*timer_lo = timer;
+	if (d->d_fieldmask & FS_DQ_BIGTIME)
+		*timer_hi = timer >> 32;
+	else
+		*timer_hi = 0;
+}
+
+static inline bool want_bigtime(s64 timer)
+{
+	return timer > S32_MAX || timer < S32_MIN;
+}
+
 static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
 			      int type, qid_t id)
 {
 	memset(dst, 0, sizeof(*dst));
+	if (want_bigtime(src->d_ino_timer) || want_bigtime(src->d_spc_timer) ||
+	    want_bigtime(src->d_rt_spc_timer))
+		dst->d_fieldmask |= FS_DQ_BIGTIME;
 	dst->d_version = FS_DQUOT_VERSION;
 	dst->d_id = id;
 	if (type == USRQUOTA)
@@ -606,14 +636,17 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
 	dst->d_ino_softlimit = src->d_ino_softlimit;
 	dst->d_bcount = quota_btobb(src->d_space);
 	dst->d_icount = src->d_ino_count;
-	dst->d_itimer = src->d_ino_timer;
-	dst->d_btimer = src->d_spc_timer;
+	copy_to_xfs_dqblk_ts(dst, &dst->d_itimer, &dst->d_itimer_hi,
+			     src->d_ino_timer);
+	copy_to_xfs_dqblk_ts(dst, &dst->d_btimer, &dst->d_btimer_hi,
+			     src->d_spc_timer);
 	dst->d_iwarns = src->d_ino_warns;
 	dst->d_bwarns = src->d_spc_warns;
 	dst->d_rtb_hardlimit = quota_btobb(src->d_rt_spc_hardlimit);
 	dst->d_rtb_softlimit = quota_btobb(src->d_rt_spc_softlimit);
 	dst->d_rtbcount = quota_btobb(src->d_rt_space);
-	dst->d_rtbtimer = src->d_rt_spc_timer;
+	copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer, &dst->d_rtbtimer_hi,
+			     src->d_rt_spc_timer);
 	dst->d_rtbwarns = src->d_rt_spc_warns;
 }
 
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 03d890b80ebc..16d73f54376d 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -66,7 +66,10 @@ typedef struct fs_disk_quota {
 	__s32		d_btimer;	/* similar to above; for disk blocks */
 	__u16	  	d_iwarns;       /* # warnings issued wrt num inodes */
 	__u16	  	d_bwarns;       /* # warnings issued wrt disk blocks */
-	__s32		d_padding2;	/* padding2 - for future use */
+	__s8		d_itimer_hi;	/* upper 8 bits of timer values */
+	__s8		d_btimer_hi;
+	__s8		d_rtbtimer_hi;
+	__s8		d_padding2;	/* padding2 - for future use */
 	__u64		d_rtb_hardlimit;/* absolute limit on realtime blks */
 	__u64		d_rtb_softlimit;/* preferred limit on RT disk blks */
 	__u64		d_rtbcount;	/* # realtime blocks owned */
@@ -121,6 +124,12 @@ typedef struct fs_disk_quota {
 #define FS_DQ_RTBCOUNT		(1<<14)
 #define FS_DQ_ACCT_MASK		(FS_DQ_BCOUNT | FS_DQ_ICOUNT | FS_DQ_RTBCOUNT)
 
+/*
+ * Quota expiration timestamps are 40-bit signed integers, with the upper 8
+ * bits encoded in the _hi fields.
+ */
+#define FS_DQ_BIGTIME		(1<<15)
+
 /*
  * Various flags related to quotactl(2).
  */

[PATCH] quotatools: support grace period expirations past y2038 in userspace

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add the ability to interpret the larger quota grace period expiration
timestamps that the kernel can export via struct xfs_kern_dqblk.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 quotaio_xfs.c |   33 +++++++++++++++++++++++++++++----
 quotaio_xfs.h |   11 ++++++++++-
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/quotaio_xfs.c b/quotaio_xfs.c
index 3333bb1..9854ec2 100644
--- a/quotaio_xfs.c
+++ b/quotaio_xfs.c
@@ -42,6 +42,29 @@ scan_dquots:	xfs_scan_dquots,
 report:		xfs_report
 };
 
+static inline time_t xfs_kern2utildqblk_ts(const struct xfs_kern_dqblk *k,
+		__s32 timer, __s8 timer_hi)
+{
+	if (k->d_fieldmask & FS_DQ_BIGTIME)
+		return (__u32)timer | (__s64)timer_hi << 32;
+	return timer;
+}
+
+static inline void xfs_util2kerndqblk_ts(const struct xfs_kern_dqblk *k,
+		__s32 *timer_lo, __s8 *timer_hi, time_t timer)
+{
+	*timer_lo = timer;
+	if (k->d_fieldmask & FS_DQ_BIGTIME)
+		*timer_hi = timer >> 32;
+	else
+		*timer_hi = 0;
+}
+
+static inline int want_bigtime(time_t timer)
+{
+	return timer > INT32_MAX || timer < INT32_MIN;
+}
+
 /*
  *	Convert XFS kernel quota format to utility format
  */
@@ -53,8 +76,8 @@ static inline void xfs_kern2utildqblk(struct util_dqblk *u, struct xfs_kern_dqbl
 	u->dqb_bsoftlimit = k->d_blk_softlimit >> 1;
 	u->dqb_curinodes = k->d_icount;
 	u->dqb_curspace = ((qsize_t)k->d_bcount) << 9;
-	u->dqb_itime = k->d_itimer;
-	u->dqb_btime = k->d_btimer;
+	u->dqb_itime = xfs_kern2utildqblk_ts(k, k->d_itimer, k->d_itimer_hi);
+	u->dqb_btime = xfs_kern2utildqblk_ts(k, k->d_btimer, k->d_btimer_hi);
 }
 
 /*
@@ -69,8 +92,10 @@ static inline void xfs_util2kerndqblk(struct xfs_kern_dqblk *k, struct util_dqbl
 	k->d_blk_softlimit = u->dqb_bsoftlimit << 1;
 	k->d_icount = u->dqb_curinodes;
 	k->d_bcount = u->dqb_curspace >> 9;
-	k->d_itimer = u->dqb_itime;
-	k->d_btimer = u->dqb_btime;
+	if (want_bigtime(u->dqb_itime) || want_bigtime(u->dqb_btime))
+		k->d_fieldmask |= FS_DQ_BIGTIME;
+	xfs_util2kerndqblk_ts(k, &k->d_itimer, &k->d_itimer_hi, u->dqb_itime);
+	xfs_util2kerndqblk_ts(k, &k->d_btimer, &k->d_btimer_hi, u->dqb_btime);
 }
 
 /*
diff --git a/quotaio_xfs.h b/quotaio_xfs.h
index be7f86f..e0c2a62 100644
--- a/quotaio_xfs.h
+++ b/quotaio_xfs.h
@@ -72,7 +72,10 @@ typedef struct fs_disk_quota {
 	__s32 d_btimer;		/* similar to above; for disk blocks */
 	__u16 d_iwarns;		/* # warnings issued wrt num inodes */
 	__u16 d_bwarns;		/* # warnings issued wrt disk blocks */
-	__s32 d_padding2;	/* padding2 - for future use */
+	__s8 d_itimer_hi;	/* upper 8 bits of timer values */
+	__s8 d_btimer_hi;
+	__s8 d_rtbtimer_hi;
+	__s8 d_padding2;	/* padding2 - for future use */
 	__u64 d_rtb_hardlimit;	/* absolute limit on realtime blks */
 	__u64 d_rtb_softlimit;	/* preferred limit on RT disk blks */
 	__u64 d_rtbcount;	/* # realtime blocks owned */
@@ -114,6 +117,12 @@ typedef struct fs_disk_quota {
 #define FS_DQ_RTBCOUNT          (1<<14)
 #define FS_DQ_ACCT_MASK         (FS_DQ_BCOUNT | FS_DQ_ICOUNT | FS_DQ_RTBCOUNT)
 
+/*
+ * Quota expiration timestamps are 40-bit signed integers, with the upper 8
+ * bits encoded in the _hi fields.
+ */
+#define FS_DQ_BIGTIME		(1<<15)
+
 /*
  * Various flags related to quotactl(2).  Only relevant to XFS filesystems.
  */

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Matthew Wilcox]

On Sat, Sep 05, 2020 at 09:47:03AM -0700, Darrick J. Wong wrote:
> +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> +{
> +	*timer_lo = timer;
> +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> +		*timer_hi = timer >> 32;
> +	else
> +		*timer_hi = 0;
> +}

Is that actually the right thing to do?  If FS_DQ_BIGTIME is not set,
I would expect us to avoid writing to timer_hi at all.  Alternatively, if
we do want to write to timer_hi, why not write to it unconditionally?

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

On Sat, Sep 05, 2020 at 11:02:31PM +0100, Matthew Wilcox wrote:
> On Sat, Sep 05, 2020 at 09:47:03AM -0700, Darrick J. Wong wrote:
> > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > +{
> > +	*timer_lo = timer;
> > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > +		*timer_hi = timer >> 32;
> > +	else
> > +		*timer_hi = 0;
> > +}
> 
> Is that actually the right thing to do?  If FS_DQ_BIGTIME is not set,
> I would expect us to avoid writing to timer_hi at all.  Alternatively, if
> we do want to write to timer_hi, why not write to it unconditionally?

If the flag isn't set, then the space used by timer_hi is a zero-filled
padding field.  Therefore, I made this function zero timer_hi if the
bigtime flag isn't set.  It's redundant with the memset five lines up
from the call site, but I don't like leaving logic bombs in case this
function ever gets exported elsewhere.

--D

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Jan Kara]

On Sat 05-09-20 09:47:03, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Soon, XFS will support quota grace period expiration timestamps beyond
> the year 2038, widen the timestamp fields to handle the extra time bits.
> Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
> bits here should work fine.  (Note that XFS is the only user of this
> structure.)
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good to me. Just one question below:

> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 5444d3c4d93f..eefac57c52fd 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
>  	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
>  }
>  
> +static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
> +		__s32 timer, __s8 timer_hi)
> +{
> +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> +		return (u32)timer | (s64)timer_hi << 32;
> +	return timer;
> +}
> +

So this doesn't do any checks that the resulting time fits into 34-bits you
speak about in the changelog. So how will XFS react if malicious / buggy
userspace will pass too big timestamp? I suppose xfs_fs_set_dqblk() should
return EFBIG or EINVAL or something like that which I'm not sure it does...

For record I've checked VFS quota implementation and it doesn't need any
checks because VFS in memory structures and on-disk format use 64-bit
timestamps. The ancient quota format uses 32-bit timestamps for 32-bit
archs so these would get silently truncated when stored on disk but
honestly I don't think I care (that format was deprecated some 20 years
ago).

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

Re: [PATCH] quotatools: support grace period expirations past y2038 in userspace

[Jan Kara]

On Sat 05-09-20 09:50:26, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add the ability to interpret the larger quota grace period expiration
> timestamps that the kernel can export via struct xfs_kern_dqblk.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks. I've added the patch to quota-tools tree. I've just extended it a
bit so that xfs_util2kerndqblk() checks whether the passed time fits into
the 40-bit field and returns error otherwise instead of silently truncating
the timestamp...

								Honza

> ---
>  quotaio_xfs.c |   33 +++++++++++++++++++++++++++++----
>  quotaio_xfs.h |   11 ++++++++++-
>  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/quotaio_xfs.c b/quotaio_xfs.c
> index 3333bb1..9854ec2 100644
> --- a/quotaio_xfs.c
> +++ b/quotaio_xfs.c
> @@ -42,6 +42,29 @@ scan_dquots:	xfs_scan_dquots,
>  report:		xfs_report
>  };
>  
> +static inline time_t xfs_kern2utildqblk_ts(const struct xfs_kern_dqblk *k,
> +		__s32 timer, __s8 timer_hi)
> +{
> +	if (k->d_fieldmask & FS_DQ_BIGTIME)
> +		return (__u32)timer | (__s64)timer_hi << 32;
> +	return timer;
> +}
> +
> +static inline void xfs_util2kerndqblk_ts(const struct xfs_kern_dqblk *k,
> +		__s32 *timer_lo, __s8 *timer_hi, time_t timer)
> +{
> +	*timer_lo = timer;
> +	if (k->d_fieldmask & FS_DQ_BIGTIME)
> +		*timer_hi = timer >> 32;
> +	else
> +		*timer_hi = 0;
> +}
> +
> +static inline int want_bigtime(time_t timer)
> +{
> +	return timer > INT32_MAX || timer < INT32_MIN;
> +}
> +
>  /*
>   *	Convert XFS kernel quota format to utility format
>   */
> @@ -53,8 +76,8 @@ static inline void xfs_kern2utildqblk(struct util_dqblk *u, struct xfs_kern_dqbl
>  	u->dqb_bsoftlimit = k->d_blk_softlimit >> 1;
>  	u->dqb_curinodes = k->d_icount;
>  	u->dqb_curspace = ((qsize_t)k->d_bcount) << 9;
> -	u->dqb_itime = k->d_itimer;
> -	u->dqb_btime = k->d_btimer;
> +	u->dqb_itime = xfs_kern2utildqblk_ts(k, k->d_itimer, k->d_itimer_hi);
> +	u->dqb_btime = xfs_kern2utildqblk_ts(k, k->d_btimer, k->d_btimer_hi);
>  }
>  
>  /*
> @@ -69,8 +92,10 @@ static inline void xfs_util2kerndqblk(struct xfs_kern_dqblk *k, struct util_dqbl
>  	k->d_blk_softlimit = u->dqb_bsoftlimit << 1;
>  	k->d_icount = u->dqb_curinodes;
>  	k->d_bcount = u->dqb_curspace >> 9;
> -	k->d_itimer = u->dqb_itime;
> -	k->d_btimer = u->dqb_btime;
> +	if (want_bigtime(u->dqb_itime) || want_bigtime(u->dqb_btime))
> +		k->d_fieldmask |= FS_DQ_BIGTIME;
> +	xfs_util2kerndqblk_ts(k, &k->d_itimer, &k->d_itimer_hi, u->dqb_itime);
> +	xfs_util2kerndqblk_ts(k, &k->d_btimer, &k->d_btimer_hi, u->dqb_btime);
>  }
>  
>  /*
> diff --git a/quotaio_xfs.h b/quotaio_xfs.h
> index be7f86f..e0c2a62 100644
> --- a/quotaio_xfs.h
> +++ b/quotaio_xfs.h
> @@ -72,7 +72,10 @@ typedef struct fs_disk_quota {
>  	__s32 d_btimer;		/* similar to above; for disk blocks */
>  	__u16 d_iwarns;		/* # warnings issued wrt num inodes */
>  	__u16 d_bwarns;		/* # warnings issued wrt disk blocks */
> -	__s32 d_padding2;	/* padding2 - for future use */
> +	__s8 d_itimer_hi;	/* upper 8 bits of timer values */
> +	__s8 d_btimer_hi;
> +	__s8 d_rtbtimer_hi;
> +	__s8 d_padding2;	/* padding2 - for future use */
>  	__u64 d_rtb_hardlimit;	/* absolute limit on realtime blks */
>  	__u64 d_rtb_softlimit;	/* preferred limit on RT disk blks */
>  	__u64 d_rtbcount;	/* # realtime blocks owned */
> @@ -114,6 +117,12 @@ typedef struct fs_disk_quota {
>  #define FS_DQ_RTBCOUNT          (1<<14)
>  #define FS_DQ_ACCT_MASK         (FS_DQ_BCOUNT | FS_DQ_ICOUNT | FS_DQ_RTBCOUNT)
>  
> +/*
> + * Quota expiration timestamps are 40-bit signed integers, with the upper 8
> + * bits encoded in the _hi fields.
> + */
> +#define FS_DQ_BIGTIME		(1<<15)
> +
>  /*
>   * Various flags related to quotactl(2).  Only relevant to XFS filesystems.
>   */
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

On Mon, Sep 07, 2020 at 12:02:18PM +0200, Jan Kara wrote:
> On Sat 05-09-20 09:47:03, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Soon, XFS will support quota grace period expiration timestamps beyond
> > the year 2038, widen the timestamp fields to handle the extra time bits.
> > Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
> > bits here should work fine.  (Note that XFS is the only user of this
> > structure.)
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Looks good to me. Just one question below:
> 
> > diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> > index 5444d3c4d93f..eefac57c52fd 100644
> > --- a/fs/quota/quota.c
> > +++ b/fs/quota/quota.c
> > @@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
> >  	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
> >  }
> >  
> > +static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > +		__s32 timer, __s8 timer_hi)
> > +{
> > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > +		return (u32)timer | (s64)timer_hi << 32;
> > +	return timer;
> > +}
> > +
> 
> So this doesn't do any checks that the resulting time fits into 34-bits you
> speak about in the changelog. So how will XFS react if malicious / buggy
> userspace will pass too big timestamp? I suppose xfs_fs_set_dqblk() should
> return EFBIG or EINVAL or something like that which I'm not sure it does...
> 
> For record I've checked VFS quota implementation and it doesn't need any
> checks because VFS in memory structures and on-disk format use 64-bit
> timestamps. The ancient quota format uses 32-bit timestamps for 32-bit
> archs so these would get silently truncated when stored on disk but
> honestly I don't think I care (that format was deprecated some 20 years
> ago).

XFS will clamp any out-of-bounds value to the nearest representable
number.  For example, if you tried to extend a quota's grace expiration
to the year 2600, it set the expiration to 2486, similar to what the vfs
does for timestamps now.  If you try to set the default grace period to,
say, 100 years, it will clamp that to 68 years (2^31-1).

(I doubt anyone cares to set a 60+ year grace period, but as some
apparently immortal person claims to be playing a 600-year musical
score[1] perhaps we will need to revisit that...)

--D

[1] https://en.wikipedia.org/wiki/As_Slow_as_Possible

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

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Jan Kara]

On Mon 07-09-20 08:01:04, Darrick J. Wong wrote:
> On Mon, Sep 07, 2020 at 12:02:18PM +0200, Jan Kara wrote:
> > On Sat 05-09-20 09:47:03, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Soon, XFS will support quota grace period expiration timestamps beyond
> > > the year 2038, widen the timestamp fields to handle the extra time bits.
> > > Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
> > > bits here should work fine.  (Note that XFS is the only user of this
> > > structure.)
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Looks good to me. Just one question below:
> > 
> > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> > > index 5444d3c4d93f..eefac57c52fd 100644
> > > --- a/fs/quota/quota.c
> > > +++ b/fs/quota/quota.c
> > > @@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
> > >  	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
> > >  }
> > >  
> > > +static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > +		__s32 timer, __s8 timer_hi)
> > > +{
> > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > +		return (u32)timer | (s64)timer_hi << 32;
> > > +	return timer;
> > > +}
> > > +
> > 
> > So this doesn't do any checks that the resulting time fits into 34-bits you
> > speak about in the changelog. So how will XFS react if malicious / buggy
> > userspace will pass too big timestamp? I suppose xfs_fs_set_dqblk() should
> > return EFBIG or EINVAL or something like that which I'm not sure it does...
> > 
> > For record I've checked VFS quota implementation and it doesn't need any
> > checks because VFS in memory structures and on-disk format use 64-bit
> > timestamps. The ancient quota format uses 32-bit timestamps for 32-bit
> > archs so these would get silently truncated when stored on disk but
> > honestly I don't think I care (that format was deprecated some 20 years
> > ago).
> 
> XFS will clamp any out-of-bounds value to the nearest representable
> number.  For example, if you tried to extend a quota's grace expiration
> to the year 2600, it set the expiration to 2486, similar to what the vfs
> does for timestamps now.  If you try to set the default grace period to,
> say, 100 years, it will clamp that to 68 years (2^31-1).

OK, sounds good. I've pushed out the patch to my tree.

> (I doubt anyone cares to set a 60+ year grace period, but as some
> apparently immortal person claims to be playing a 600-year musical
> score[1] perhaps we will need to revisit that...)
> 
> --D
> 
> [1] https://en.wikipedia.org/wiki/As_Slow_as_Possible

;)

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

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Christoph Hellwig]

On Sat, Sep 05, 2020 at 09:47:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Soon, XFS will support quota grace period expiration timestamps beyond
> the year 2038, widen the timestamp fields to handle the extra time bits.
> Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
> bits here should work fine.  (Note that XFS is the only user of this
> structure.)
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH v2] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

On Sat, Sep 05, 2020 at 09:47:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Soon, XFS will support quota grace period expiration timestamps beyond
> the year 2038, widen the timestamp fields to handle the extra time bits.
> Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
> bits here should work fine.  (Note that XFS is the only user of this
> structure.)
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: use __s8 for timestamp extension
> ---
>  fs/quota/quota.c               |   43 +++++++++++++++++++++++++++++++++++-----
>  include/uapi/linux/dqblk_xfs.h |   11 +++++++++-
>  2 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 5444d3c4d93f..eefac57c52fd 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
>  	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
>  }
>  
> +static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
> +		__s32 timer, __s8 timer_hi)
> +{
> +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> +		return (u32)timer | (s64)timer_hi << 32;
> +	return timer;
> +}
> +
>  static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
>  {
>  	dst->d_spc_hardlimit = quota_bbtob(src->d_blk_hardlimit);
> @@ -489,14 +497,18 @@ static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
>  	dst->d_ino_softlimit = src->d_ino_softlimit;
>  	dst->d_space = quota_bbtob(src->d_bcount);
>  	dst->d_ino_count = src->d_icount;
> -	dst->d_ino_timer = src->d_itimer;
> -	dst->d_spc_timer = src->d_btimer;
> +	dst->d_ino_timer = copy_from_xfs_dqblk_ts(src, src->d_itimer,
> +						  src->d_itimer_hi);
> +	dst->d_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_btimer,
> +						  src->d_btimer_hi);
>  	dst->d_ino_warns = src->d_iwarns;
>  	dst->d_spc_warns = src->d_bwarns;
>  	dst->d_rt_spc_hardlimit = quota_bbtob(src->d_rtb_hardlimit);
>  	dst->d_rt_spc_softlimit = quota_bbtob(src->d_rtb_softlimit);
>  	dst->d_rt_space = quota_bbtob(src->d_rtbcount);
>  	dst->d_rt_spc_timer = src->d_rtbtimer;

OFC it's only now that the 0day robot catches up and tells me that I
forgot to remove the above statement.  Ah well, v3 on its way...

--D

> +	dst->d_rt_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_rtbtimer,
> +						     src->d_rtbtimer_hi);
>  	dst->d_rt_spc_warns = src->d_rtbwarns;
>  	dst->d_fieldmask = 0;
>  	if (src->d_fieldmask & FS_DQ_ISOFT)
> @@ -588,10 +600,28 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
>  	return sb->s_qcop->set_dqblk(sb, qid, &qdq);
>  }
>  
> +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> +{
> +	*timer_lo = timer;
> +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> +		*timer_hi = timer >> 32;
> +	else
> +		*timer_hi = 0;
> +}
> +
> +static inline bool want_bigtime(s64 timer)
> +{
> +	return timer > S32_MAX || timer < S32_MIN;
> +}
> +
>  static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
>  			      int type, qid_t id)
>  {
>  	memset(dst, 0, sizeof(*dst));
> +	if (want_bigtime(src->d_ino_timer) || want_bigtime(src->d_spc_timer) ||
> +	    want_bigtime(src->d_rt_spc_timer))
> +		dst->d_fieldmask |= FS_DQ_BIGTIME;
>  	dst->d_version = FS_DQUOT_VERSION;
>  	dst->d_id = id;
>  	if (type == USRQUOTA)
> @@ -606,14 +636,17 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
>  	dst->d_ino_softlimit = src->d_ino_softlimit;
>  	dst->d_bcount = quota_btobb(src->d_space);
>  	dst->d_icount = src->d_ino_count;
> -	dst->d_itimer = src->d_ino_timer;
> -	dst->d_btimer = src->d_spc_timer;
> +	copy_to_xfs_dqblk_ts(dst, &dst->d_itimer, &dst->d_itimer_hi,
> +			     src->d_ino_timer);
> +	copy_to_xfs_dqblk_ts(dst, &dst->d_btimer, &dst->d_btimer_hi,
> +			     src->d_spc_timer);
>  	dst->d_iwarns = src->d_ino_warns;
>  	dst->d_bwarns = src->d_spc_warns;
>  	dst->d_rtb_hardlimit = quota_btobb(src->d_rt_spc_hardlimit);
>  	dst->d_rtb_softlimit = quota_btobb(src->d_rt_spc_softlimit);
>  	dst->d_rtbcount = quota_btobb(src->d_rt_space);
> -	dst->d_rtbtimer = src->d_rt_spc_timer;
> +	copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer, &dst->d_rtbtimer_hi,
> +			     src->d_rt_spc_timer);
>  	dst->d_rtbwarns = src->d_rt_spc_warns;
>  }
>  
> diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> index 03d890b80ebc..16d73f54376d 100644
> --- a/include/uapi/linux/dqblk_xfs.h
> +++ b/include/uapi/linux/dqblk_xfs.h
> @@ -66,7 +66,10 @@ typedef struct fs_disk_quota {
>  	__s32		d_btimer;	/* similar to above; for disk blocks */
>  	__u16	  	d_iwarns;       /* # warnings issued wrt num inodes */
>  	__u16	  	d_bwarns;       /* # warnings issued wrt disk blocks */
> -	__s32		d_padding2;	/* padding2 - for future use */
> +	__s8		d_itimer_hi;	/* upper 8 bits of timer values */
> +	__s8		d_btimer_hi;
> +	__s8		d_rtbtimer_hi;
> +	__s8		d_padding2;	/* padding2 - for future use */
>  	__u64		d_rtb_hardlimit;/* absolute limit on realtime blks */
>  	__u64		d_rtb_softlimit;/* preferred limit on RT disk blks */
>  	__u64		d_rtbcount;	/* # realtime blocks owned */
> @@ -121,6 +124,12 @@ typedef struct fs_disk_quota {
>  #define FS_DQ_RTBCOUNT		(1<<14)
>  #define FS_DQ_ACCT_MASK		(FS_DQ_BCOUNT | FS_DQ_ICOUNT | FS_DQ_RTBCOUNT)
>  
> +/*
> + * Quota expiration timestamps are 40-bit signed integers, with the upper 8
> + * bits encoded in the _hi fields.
> + */
> +#define FS_DQ_BIGTIME		(1<<15)
> +
>  /*
>   * Various flags related to quotactl(2).
>   */