| Message ID | 20200904053931.GB6096@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | quota: widen timestamps for the fs_disk_quota structure | expand |
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
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
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). */