Message ID | 157784106702.1364230.14985571182679451055.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: widen timestamps to deal with y2038 | expand |
On 12/31/19 7:11 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_format.h | 19 +++++++++++++++++++ > fs/xfs/xfs_ondisk.h | 8 ++++++++ > fs/xfs/xfs_super.c | 4 ++-- > 3 files changed, 29 insertions(+), 2 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 9ff373962d10..82b15832ba32 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -841,11 +841,30 @@ typedef struct xfs_agfl { > ASSERT(xfs_daddr_to_agno(mp, d) == \ > xfs_daddr_to_agno(mp, (d) + (len) - 1))) > > +/* > + * XFS Timestamps > + * ============== > + * > + * Inode timestamps consist of signed 32-bit counters for seconds and > + * nanoseconds; time zero is the Unix epoch, Jan 1 00:00:00 UTC 1970. > + */ > typedef struct xfs_timestamp { > __be32 t_sec; /* timestamp seconds */ > __be32 t_nsec; /* timestamp nanoseconds */ > } xfs_timestamp_t; > > +/* > + * Smallest possible timestamp with traditional timestamps, which is > + * Dec 13 20:45:52 UTC 1901. > + */ > +#define XFS_INO_TIME_MIN ((int64_t)S32_MIN) > + > +/* > + * Largest possible timestamp with traditional timestamps, which is > + * Jan 19 03:14:07 UTC 2038. > + */ > +#define XFS_INO_TIME_MAX ((int64_t)S32_MAX) > + > /* > * On-disk inode structure. > * > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h > index fa0ec2fae14a..f67f3645efcd 100644 > --- a/fs/xfs/xfs_ondisk.h > +++ b/fs/xfs/xfs_ondisk.h > @@ -15,9 +15,17 @@ > "XFS: offsetof(" #structname ", " #member ") is wrong, " \ > "expected " #off) > > +#define XFS_CHECK_VALUE(value, expected) \ > + BUILD_BUG_ON_MSG((value) != (expected), \ > + "XFS: value of " #value " is wrong, expected " #expected) > + > static inline void __init > xfs_check_ondisk_structs(void) > { > + /* make sure timestamp limits are correct */ > + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL); > + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); IMHO this really shouldn't be in a function with this name, as it's not checking an ondisk struct. And I'm not really sure what it's protecting against? Basically you put an integer in one #define and check it in another? > + > /* ag/file structures */ > XFS_CHECK_STRUCT_SIZE(struct xfs_acl, 4); > XFS_CHECK_STRUCT_SIZE(struct xfs_acl_entry, 12); > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index f687181a2720..3bddf13cd8ea 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1582,8 +1582,8 @@ xfs_fc_fill_super( > sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); > sb->s_max_links = XFS_MAXLINK; > sb->s_time_gran = 1; > - sb->s_time_min = S32_MIN; > - sb->s_time_max = S32_MAX; > + sb->s_time_min = XFS_INO_TIME_MIN; > + sb->s_time_max = XFS_INO_TIME_MAX; > sb->s_iflags |= SB_I_CGROUPWB; > > set_posix_acl_flag(sb); >
On Wed, Feb 12, 2020 at 05:00:59PM -0600, Eric Sandeen wrote: > On 12/31/19 7:11 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/libxfs/xfs_format.h | 19 +++++++++++++++++++ > > fs/xfs/xfs_ondisk.h | 8 ++++++++ > > fs/xfs/xfs_super.c | 4 ++-- > > 3 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > index 9ff373962d10..82b15832ba32 100644 > > --- a/fs/xfs/libxfs/xfs_format.h > > +++ b/fs/xfs/libxfs/xfs_format.h > > @@ -841,11 +841,30 @@ typedef struct xfs_agfl { > > ASSERT(xfs_daddr_to_agno(mp, d) == \ > > xfs_daddr_to_agno(mp, (d) + (len) - 1))) > > > > +/* > > + * XFS Timestamps > > + * ============== > > + * > > + * Inode timestamps consist of signed 32-bit counters for seconds and > > + * nanoseconds; time zero is the Unix epoch, Jan 1 00:00:00 UTC 1970. > > + */ > > typedef struct xfs_timestamp { > > __be32 t_sec; /* timestamp seconds */ > > __be32 t_nsec; /* timestamp nanoseconds */ > > } xfs_timestamp_t; > > > > +/* > > + * Smallest possible timestamp with traditional timestamps, which is > > + * Dec 13 20:45:52 UTC 1901. > > + */ > > +#define XFS_INO_TIME_MIN ((int64_t)S32_MIN) > > + > > +/* > > + * Largest possible timestamp with traditional timestamps, which is > > + * Jan 19 03:14:07 UTC 2038. > > + */ > > +#define XFS_INO_TIME_MAX ((int64_t)S32_MAX) > > + > > /* > > * On-disk inode structure. > > * > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h > > index fa0ec2fae14a..f67f3645efcd 100644 > > --- a/fs/xfs/xfs_ondisk.h > > +++ b/fs/xfs/xfs_ondisk.h > > @@ -15,9 +15,17 @@ > > "XFS: offsetof(" #structname ", " #member ") is wrong, " \ > > "expected " #off) > > > > +#define XFS_CHECK_VALUE(value, expected) \ > > + BUILD_BUG_ON_MSG((value) != (expected), \ > > + "XFS: value of " #value " is wrong, expected " #expected) > > + > > static inline void __init > > xfs_check_ondisk_structs(void) > > { > > + /* make sure timestamp limits are correct */ > > + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL); > > + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); > > IMHO this really shouldn't be in a function with this name, as it's not checking > an ondisk struct. And I'm not really sure what it's protecting against? > Basically you put an integer in one #define and check it in another? Admittedly /this/ part isn't so crucial, because S32_MAX is never going to be redefined. However, I added this for completeness; notice that the patch that widens xfs_timestamp_t adds similar checks for the new minimum and maximum timestamp, whose values are not so straightforward. Also, I get that this isn't directly checking an ondisk structure, but given that we use these constants, there ought to be a check against incorrect computation *somewhere*. The BUILD_BUG_ON macros don't produce any real code (and this function is called at __init time) so what's the harm? --D > > + > > /* ag/file structures */ > > XFS_CHECK_STRUCT_SIZE(struct xfs_acl, 4); > > XFS_CHECK_STRUCT_SIZE(struct xfs_acl_entry, 12); > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index f687181a2720..3bddf13cd8ea 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -1582,8 +1582,8 @@ xfs_fc_fill_super( > > sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); > > sb->s_max_links = XFS_MAXLINK; > > sb->s_time_gran = 1; > > - sb->s_time_min = S32_MIN; > > - sb->s_time_max = S32_MAX; > > + sb->s_time_min = XFS_INO_TIME_MIN; > > + sb->s_time_max = XFS_INO_TIME_MAX; > > sb->s_iflags |= SB_I_CGROUPWB; > > > > set_posix_acl_flag(sb); > >
On 2/12/20 7:26 PM, Darrick J. Wong wrote: > On Wed, Feb 12, 2020 at 05:00:59PM -0600, Eric Sandeen wrote: >> On 12/31/19 7:11 PM, Darrick J. Wong wrote: >>> From: Darrick J. Wong <darrick.wong@oracle.com> >>> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> ... >>> static inline void __init >>> xfs_check_ondisk_structs(void) >>> { >>> + /* make sure timestamp limits are correct */ >>> + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL); >>> + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); >> >> IMHO this really shouldn't be in a function with this name, as it's not checking >> an ondisk struct. And I'm not really sure what it's protecting against? >> Basically you put an integer in one #define and check it in another? > > Admittedly /this/ part isn't so crucial, because S32_MAX is never going > to be redefined. However, I added this for completeness; notice that > the patch that widens xfs_timestamp_t adds similar checks for the new > minimum and maximum timestamp, whose values are not so straightforward. > > Also, I get that this isn't directly checking an ondisk structure, but > given that we use these constants, there ought to be a check against > incorrect computation *somewhere*. The BUILD_BUG_ON macros don't > produce any real code (and this function is called at __init time) so > what's the harm? OCD? Maybe just make a xfs_check_time_vals() to go with xfs_check_ondisk_structs() or something. -Eric
On Wed, Feb 12, 2020 at 07:50:02PM -0600, Eric Sandeen wrote: > On 2/12/20 7:26 PM, Darrick J. Wong wrote: > > On Wed, Feb 12, 2020 at 05:00:59PM -0600, Eric Sandeen wrote: > >> On 12/31/19 7:11 PM, Darrick J. Wong wrote: > >>> From: Darrick J. Wong <darrick.wong@oracle.com> > >>> > >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > ... > > >>> static inline void __init > >>> xfs_check_ondisk_structs(void) > >>> { > >>> + /* make sure timestamp limits are correct */ > >>> + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL); > >>> + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); > >> > >> IMHO this really shouldn't be in a function with this name, as it's not checking > >> an ondisk struct. And I'm not really sure what it's protecting against? > >> Basically you put an integer in one #define and check it in another? > > > > Admittedly /this/ part isn't so crucial, because S32_MAX is never going > > to be redefined. However, I added this for completeness; notice that > > the patch that widens xfs_timestamp_t adds similar checks for the new > > minimum and maximum timestamp, whose values are not so straightforward. > > > > Also, I get that this isn't directly checking an ondisk structure, but > > given that we use these constants, there ought to be a check against > > incorrect computation *somewhere*. The BUILD_BUG_ON macros don't > > produce any real code (and this function is called at __init time) so > > what's the harm? > > OCD? Maybe just make a xfs_check_time_vals() to go with > xfs_check_ondisk_structs() or something. Done. --D > -Eric
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 9ff373962d10..82b15832ba32 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -841,11 +841,30 @@ typedef struct xfs_agfl { ASSERT(xfs_daddr_to_agno(mp, d) == \ xfs_daddr_to_agno(mp, (d) + (len) - 1))) +/* + * XFS Timestamps + * ============== + * + * Inode timestamps consist of signed 32-bit counters for seconds and + * nanoseconds; time zero is the Unix epoch, Jan 1 00:00:00 UTC 1970. + */ typedef struct xfs_timestamp { __be32 t_sec; /* timestamp seconds */ __be32 t_nsec; /* timestamp nanoseconds */ } xfs_timestamp_t; +/* + * Smallest possible timestamp with traditional timestamps, which is + * Dec 13 20:45:52 UTC 1901. + */ +#define XFS_INO_TIME_MIN ((int64_t)S32_MIN) + +/* + * Largest possible timestamp with traditional timestamps, which is + * Jan 19 03:14:07 UTC 2038. + */ +#define XFS_INO_TIME_MAX ((int64_t)S32_MAX) + /* * On-disk inode structure. * diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h index fa0ec2fae14a..f67f3645efcd 100644 --- a/fs/xfs/xfs_ondisk.h +++ b/fs/xfs/xfs_ondisk.h @@ -15,9 +15,17 @@ "XFS: offsetof(" #structname ", " #member ") is wrong, " \ "expected " #off) +#define XFS_CHECK_VALUE(value, expected) \ + BUILD_BUG_ON_MSG((value) != (expected), \ + "XFS: value of " #value " is wrong, expected " #expected) + static inline void __init xfs_check_ondisk_structs(void) { + /* make sure timestamp limits are correct */ + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL); + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); + /* ag/file structures */ XFS_CHECK_STRUCT_SIZE(struct xfs_acl, 4); XFS_CHECK_STRUCT_SIZE(struct xfs_acl_entry, 12); diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index f687181a2720..3bddf13cd8ea 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1582,8 +1582,8 @@ xfs_fc_fill_super( sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits); sb->s_max_links = XFS_MAXLINK; sb->s_time_gran = 1; - sb->s_time_min = S32_MIN; - sb->s_time_max = S32_MAX; + sb->s_time_min = XFS_INO_TIME_MIN; + sb->s_time_max = XFS_INO_TIME_MAX; sb->s_iflags |= SB_I_CGROUPWB; set_posix_acl_flag(sb);