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]

Date:	Mon, 11 Jan 2016 10:03:39 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Deepa Dinamani <deepa.kernel@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	y2038@...ts.linaro.org
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> The current representation of inode times in struct inode, struct iattr,
> and struct kstat use struct timespec. timespec is not y2038 safe.
> 
> Use scalar data types (seconds and nanoseconds stored separately) to
> represent timestamps in struct inode in order to maintain same size for
> times across 32 bit and 64 bit architectures.
> In addition, lay them out such that they are packed on a naturally
> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
> This will help save RAM space as inode structure is cached in memory.
> The other structures are transient and do not benefit from these
> changes.

IMO, this decisions sends the patch series immediately down the
wrong path. TO me, this is a severe case of premature optimisation
because everything gets way more complex just to save those 8 bytes,
especially as those holes can be filled simply by changing the
variable declaration order in the structure and adding a simple
comment.

And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
all; you've got to touch lots of code(*), making it all shouty and
harder to read.  They seem only to exist because of the above
structural change requires an abstract timestamp accessor while
CONFIG_FS_USES_64BIT_TIME exists. Given that goes away at the end o
the series, so should the macro - if we use a struct timespec64 in
the first place, it isn't even necessary as a temporary construct.

(*) I note you haven't touched XFS, which means you've probably
broken lots of other filesystem code. e.g. in XFS, functions like
xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time
directly and hence are not going to compile after this patch series.

Cheers,

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