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

[Jan Kara <jack@suse.cz>]

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