linux-kernel - Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160120230632.GZ6033@dastard>
Date:	Thu, 21 Jan 2016 10:06:32 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Deepa Dinamani <deepa.kernel@...il.com>,
	Andreas Dilger <adilger@...ger.ca>,
	"y2038@...ts.linaro.org" <y2038@...ts.linaro.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

On Tue, Jan 19, 2016 at 11:25:02PM +0100, Arnd Bergmann wrote:
> On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote:
> > You're doing it wrong. fat_time_fat2unix() still gets passed
> > &inode->i_mtime, and the function prototype is changed to a
> > timespec64.  *Nothing else needs to change*, because
> > fat_time_fat2unix() does it own calculations and then stores the
> > time directly into the timespec structure members....
> 
> Any idea how to improve this somewhat lacking patch?
> 
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index b97f1df910ab..7fbb07dcad36 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -68,22 +68,24 @@ xfs_trans_ichgtime(
>  	int			flags)
>  {
>  	struct inode		*inode = VFS_I(ip);
> -	struct timespec		tv;
> +	struct timespec		tv, mtime, ctime;
>  
>  	ASSERT(tp);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> -	tv = current_fs_time(inode->i_sb);
> +	tv = vfs_time_to_timespec(current_fs_time(inode->i_sb));
> +	mtime = vfs_time_to_timespec(inode->i_mtime);
> +	ctime = vfs_time_to_timespec(inode->i_ctime);
>  
>  	if ((flags & XFS_ICHGTIME_MOD) &&
> -	    !timespec_equal(&inode->i_mtime, &tv)) {
> -		inode->i_mtime = tv;
> +	    !timespec_equal(&mtime, &tv)) {
> +		inode->i_mtime = timespec_to_vfs_time(tv);
>  		ip->i_d.di_mtime.t_sec = tv.tv_sec;
>  		ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>  	}
>  	if ((flags & XFS_ICHGTIME_CHG) &&
> -	    !timespec_equal(&inode->i_ctime, &tv)) {
> -		inode->i_ctime = tv;
> +	    !timespec_equal(&ctime, &tv)) {
> +		inode->i_ctime = timespec_to_vfs_time(tv);
>  		ip->i_d.di_ctime.t_sec = tv.tv_sec;
>  		ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>  	}

WTF? That's insane and completely unnecessary. It's even worse than
the FAT example I've already pointed out was just fucking wrong.

The only change this function requires is:

> The way that Deepa suggests I think would turn out as:
> 
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index b97f1df910ab..54fc3c41047a 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -68,7 +68,7 @@ xfs_trans_ichgtime(
>  	int			flags)
>  {
>  	struct inode		*inode = VFS_I(ip);
> -	struct timespec		tv;
> +	struct vfs_time		tv;

+ 	struct timespec64	tv;

>  
>  	ASSERT(tp);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> @@ -76,13 +76,13 @@ xfs_trans_ichgtime(
>  	tv = current_fs_time(inode->i_sb);
>  
>  	if ((flags & XFS_ICHGTIME_MOD) &&
> -	    !timespec_equal(&inode->i_mtime, &tv)) {
> +	    !vfs_time_equal(&inode->i_mtime, &tv)) {

+	    !timespec64_equal(&inode->i_mtime, &tv)) {

>  		inode->i_mtime = tv;
>  		ip->i_d.di_mtime.t_sec = tv.tv_sec;
>  		ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>  	}
>  	if ((flags & XFS_ICHGTIME_CHG) &&
> -	    !timespec_equal(&inode->i_ctime, &tv)) {
> +	    !vfs_time_equal(&inode->i_ctime, &tv)) {

+	    !timespec64_equal(&inode->i_ctime, &tv)) {

>  		inode->i_ctime = tv;
>  		ip->i_d.di_ctime.t_sec = tv.tv_sec;
>  		ip->i_d.di_ctime.t_nsec = tv.tv_nsec;

> which I would much prefer here.

IOWs, you're now finally suggesting doing the *simple conversion*
I've been suggesting needs to be made *since the start* of this
long, frustrating thread, except you *still want to abstract the
timestamp unnecessarily*.

For the last time: use timespec64 directly, do not abstract it
in any way.

-Dave.
-- 
Dave Chinner
david@...morbit.com