* [PATCH] quota: widen timestamps for the fs_disk_quota structure
@ 2020-09-04 5:39 Darrick J. Wong
2020-09-04 8:31 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2020-09-04 5:39 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Widen the timestamp fields in struct fs_disk_quota to handle quota grace
expiration times beyond 2038. Since the only filesystem that's going to
use this (XFS) only supports unsigned 34-bit quantities, adding an extra
5 bits here should work fine. We can rev the structure again in 350
years.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/quota/quota.c | 40 +++++++++++++++++++++++++++++++++++++---
include/uapi/linux/dqblk_xfs.h | 13 +++++++++++--
2 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 5444d3c4d93f..efa14d9ee06f 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,27 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
return sb->s_qcop->set_dqblk(sb, qid, &qdq);
}
+static inline __s8 copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
+ __s32 *timer_lo, s64 timer)
+{
+ *timer_lo = timer;
+ if (d->d_fieldmask & FS_DQ_BIGTIME)
+ return timer >> 32;
+ return 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,6 +635,10 @@ 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_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_itimer,
+ src->d_ino_timer);
+ dst->d_btimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_btimer,
+ src->d_spc_timer);
dst->d_itimer = src->d_ino_timer;
dst->d_btimer = src->d_spc_timer;
dst->d_iwarns = src->d_ino_warns;
@@ -613,7 +646,8 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
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;
+ dst->d_rtbtimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer,
+ 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..a684f64d9cc0 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -71,8 +71,11 @@ typedef struct fs_disk_quota {
__u64 d_rtb_softlimit;/* preferred limit on RT disk blks */
__u64 d_rtbcount; /* # realtime blocks owned */
__s32 d_rtbtimer; /* similar to above; for RT disk blks */
- __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
- __s16 d_padding3; /* padding3 - for future use */
+ __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
+ __s8 d_itimer_hi:5; /* upper 5 bits of timers */
+ __s8 d_btimer_hi:5;
+ __s8 d_rtbtimer_hi:5;
+ __u8 d_padding3:1; /* padding3 - for future use */
char d_padding4[8]; /* yet more padding */
} fs_disk_quota_t;
@@ -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 37-bit signed integers, with the upper 5
+ * bits encoded in the _hi fields.
+ */
+#define FS_DQ_BIGTIME (1<<15)
+
/*
* Various flags related to quotactl(2).
*/
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] quota: widen timestamps for the fs_disk_quota structure
2020-09-04 5:39 [PATCH] quota: widen timestamps for the fs_disk_quota structure Darrick J. Wong
@ 2020-09-04 8:31 ` Jan Kara
2020-09-04 15:29 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2020-09-04 8:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Jan Kara, linux-fsdevel, xfs
On Thu 03-09-20 22:39:31, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Widen the timestamp fields in struct fs_disk_quota to handle quota grace
> expiration times beyond 2038. Since the only filesystem that's going to
> use this (XFS) only supports unsigned 34-bit quantities, adding an extra
> 5 bits here should work fine. We can rev the structure again in 350
> years.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Some comments below...
> @@ -588,10 +600,27 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
> return sb->s_qcop->set_dqblk(sb, qid, &qdq);
> }
>
> +static inline __s8 copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> + __s32 *timer_lo, s64 timer)
> +{
> + *timer_lo = timer;
> + if (d->d_fieldmask & FS_DQ_BIGTIME)
> + return timer >> 32;
> + return 0;
> +}
Hum, this function API looks a bit strange to me - directly store timer_lo
and just return timer_hi... Why not having timer_hi as another function
argument?
> @@ -606,6 +635,10 @@ 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_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_itimer,
> + src->d_ino_timer);
> + dst->d_btimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_btimer,
> + src->d_spc_timer);
> dst->d_itimer = src->d_ino_timer;
> dst->d_btimer = src->d_spc_timer;
Also it seems pointless (if not outright buggy due to sign-extension rules)
to store to say d_itimer when copy_to_xfs_dqblk_ts() already did it...
> dst->d_iwarns = src->d_ino_warns;
> @@ -613,7 +646,8 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
> 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;
> + dst->d_rtbtimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer,
> + 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..a684f64d9cc0 100644
> --- a/include/uapi/linux/dqblk_xfs.h
> +++ b/include/uapi/linux/dqblk_xfs.h
> @@ -71,8 +71,11 @@ typedef struct fs_disk_quota {
> __u64 d_rtb_softlimit;/* preferred limit on RT disk blks */
> __u64 d_rtbcount; /* # realtime blocks owned */
> __s32 d_rtbtimer; /* similar to above; for RT disk blks */
> - __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
> - __s16 d_padding3; /* padding3 - for future use */
> + __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
> + __s8 d_itimer_hi:5; /* upper 5 bits of timers */
> + __s8 d_btimer_hi:5;
> + __s8 d_rtbtimer_hi:5;
> + __u8 d_padding3:1; /* padding3 - for future use */
> char d_padding4[8]; /* yet more padding */
> } fs_disk_quota_t;
I'm a bit nervous about passing bitfields through kernel-userspace
interface. It *should* work OK but I'm not sure rules for bitfield packing
between different compilers are always compatible. E.g. in this case will
the compiler emit three 1-byte fields (as __s8 kind of suggests), just
masking 5-bits out of each or will it use 16-bit wide memory location with
all four fields packed together? And if this is even defined? I didn't find
anything definitive. Also I've found some notes that the order of bit
fields in a word is implementation defined...
So to save us some headaches, I'd prefer to use just three times __s8 for
the _hi fields and then check whether userspace didn't pass too big values
(more than 5 significant bits) when copying from userspace.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] quota: widen timestamps for the fs_disk_quota structure
2020-09-04 8:31 ` Jan Kara
@ 2020-09-04 15:29 ` Darrick J. Wong
0 siblings, 0 replies; 3+ messages in thread
From: Darrick J. Wong @ 2020-09-04 15:29 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, xfs
On Fri, Sep 04, 2020 at 10:31:23AM +0200, Jan Kara wrote:
> On Thu 03-09-20 22:39:31, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Widen the timestamp fields in struct fs_disk_quota to handle quota grace
> > expiration times beyond 2038. Since the only filesystem that's going to
> > use this (XFS) only supports unsigned 34-bit quantities, adding an extra
> > 5 bits here should work fine. We can rev the structure again in 350
> > years.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Some comments below...
>
> > @@ -588,10 +600,27 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
> > return sb->s_qcop->set_dqblk(sb, qid, &qdq);
> > }
> >
> > +static inline __s8 copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > + __s32 *timer_lo, s64 timer)
> > +{
> > + *timer_lo = timer;
> > + if (d->d_fieldmask & FS_DQ_BIGTIME)
> > + return timer >> 32;
> > + return 0;
> > +}
>
> Hum, this function API looks a bit strange to me - directly store timer_lo
> and just return timer_hi... Why not having timer_hi as another function
> argument?
You can't pass pointers to a bitset. :)
> > @@ -606,6 +635,10 @@ 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_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_itimer,
> > + src->d_ino_timer);
> > + dst->d_btimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_btimer,
> > + src->d_spc_timer);
> > dst->d_itimer = src->d_ino_timer;
> > dst->d_btimer = src->d_spc_timer;
>
> Also it seems pointless (if not outright buggy due to sign-extension rules)
> to store to say d_itimer when copy_to_xfs_dqblk_ts() already did it...
Oops. That was a straight up bug. :(
> > dst->d_iwarns = src->d_ino_warns;
> > @@ -613,7 +646,8 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
> > 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;
> > + dst->d_rtbtimer_hi = copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer,
> > + 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..a684f64d9cc0 100644
> > --- a/include/uapi/linux/dqblk_xfs.h
> > +++ b/include/uapi/linux/dqblk_xfs.h
> > @@ -71,8 +71,11 @@ typedef struct fs_disk_quota {
> > __u64 d_rtb_softlimit;/* preferred limit on RT disk blks */
> > __u64 d_rtbcount; /* # realtime blocks owned */
> > __s32 d_rtbtimer; /* similar to above; for RT disk blks */
> > - __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
> > - __s16 d_padding3; /* padding3 - for future use */
> > + __u16 d_rtbwarns; /* # warnings issued wrt RT disk blks */
> > + __s8 d_itimer_hi:5; /* upper 5 bits of timers */
> > + __s8 d_btimer_hi:5;
> > + __s8 d_rtbtimer_hi:5;
> > + __u8 d_padding3:1; /* padding3 - for future use */
> > char d_padding4[8]; /* yet more padding */
> > } fs_disk_quota_t;
>
> I'm a bit nervous about passing bitfields through kernel-userspace
> interface. It *should* work OK but I'm not sure rules for bitfield packing
> between different compilers are always compatible. E.g. in this case will
> the compiler emit three 1-byte fields (as __s8 kind of suggests), just
> masking 5-bits out of each or will it use 16-bit wide memory location with
> all four fields packed together? And if this is even defined? I didn't find
> anything definitive. Also I've found some notes that the order of bit
> fields in a word is implementation defined...
>
> So to save us some headaches, I'd prefer to use just three times __s8 for
> the _hi fields and then check whether userspace didn't pass too big values
> (more than 5 significant bits) when copying from userspace.
Ok. I was trying to leave the u32 and u64 paddings, but I'll pick
something to burn down. __s8[3] it is. :)
--D
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-04 15:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 5:39 [PATCH] quota: widen timestamps for the fs_disk_quota structure Darrick J. Wong
2020-09-04 8:31 ` Jan Kara
2020-09-04 15:29 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).