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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> On Tue, Sep 08, 2020 at 06:32:51PM -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;
> > +}
> 
> I'm still confused by this.  What breaks if you just do:
> 
> 	*timer_lo = timer;
> 	*timer_hi = timer >> 32;

"I don't know."

The manpage for quotactl doesn't actually specify the behavior of the
padding fields.  The /implementation/ is careful enough to zero
everything, but the interface specification doesn't explicitly require
software to do so.

Because the contents of the padding fields aren't defined by the
documentation, the kernel cannot simply start using the d_padding2 field
because there could be an old kernel that doesn't zero the padding,
which would lead to confusion if the new userspace were mated to such a
kernel.

Therefore, we have to add a flag that states explicitly that we are
using the timer_hi fields.  This is also the only way that an old
program can detect that it's being fed a structure that it might not
recognise.

Keep in mind that if @timer is negative (i.e. before the unix epoch)
then technically timer_hi has to be all ones if FS_DQ_BIGTIME is set.
The only user doesn't support that, but that's no excuse for sloppiness.

> >  	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;
> 
> You still need this (I guess?  In case somebody's written to the
> d_padding2 field?)

Yes.

--D