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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> 
> 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@xxxxxxxx>
> SUSE Labs, CR



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux