2018-06-19 15:40:05

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] coda: stop using 'struct timespec' in user API

We exchange file timestamps with user space using psdev device
read/write operations with a fixed but architecture specific binary
layout.

On 32-bit systems, this uses a 'timespec' structure that is defined by
the C library to contain two 32-bit values for seconds and nanoseconds.
As we get ready for the year 2038 overflow of the 32-bit signed seconds,
the kernel now uses 64-bit timestamps internally, and user space will
do the same change by changing the 'timespec' definition in the future.

Unfortunately, this breaks the layout of the coda_vattr structure, so
we need to redefine that in terms of something that does not change.
I'm introducing a new 'struct vtimespec' structure here that keeps
the existing layout, and the same change has to be done in the coda
user space copy of linux/coda.h before anyone can use that on a 32-bit
architecture with 64-bit time_t.

An open question is what should happen to actual times past y2038,
as they are now truncated to the last valid date when sent to user
space, and interpreted as pre-1970 times when a timestamp with the
MSB set is read back into the kernel. Alternatively, we could
change the new timespec64_to_coda()/coda_to_timespec64() functions
to use a different interpretation and extend the available range
further to the future by disallowing past timestamps. This would
require more changes in the user space side though.

Signed-off-by: Arnd Bergmann <[email protected]>
---
Documentation/filesystems/coda.txt | 11 ++++++---
fs/coda/coda_linux.c | 50 +++++++++++++++++++++++++++++---------
include/uapi/linux/coda.h | 20 ++++++++++++---
3 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/Documentation/filesystems/coda.txt b/Documentation/filesystems/coda.txt
index 61311356025d..ea5969068895 100644
--- a/Documentation/filesystems/coda.txt
+++ b/Documentation/filesystems/coda.txt
@@ -481,7 +481,10 @@ kernel support.



-
+ struct vtimespec {
+ long tv_sec; /* seconds */
+ long tv_nsec; /* nanoseconds */
+ };

struct coda_vattr {
enum coda_vtype va_type; /* vnode type (for create) */
@@ -493,9 +496,9 @@ kernel support.
long va_fileid; /* file id */
u_quad_t va_size; /* file size in bytes */
long va_blocksize; /* blocksize preferred for i/o */
- struct timespec va_atime; /* time of last access */
- struct timespec va_mtime; /* time of last modification */
- struct timespec va_ctime; /* time file changed */
+ struct vtimespec va_atime; /* time of last access */
+ struct vtimespec va_mtime; /* time of last modification */
+ struct vtimespec va_ctime; /* time file changed */
u_long va_gen; /* generation number of file */
u_long va_flags; /* flags defined for file */
dev_t va_rdev; /* device special file represents */
diff --git a/fs/coda/coda_linux.c b/fs/coda/coda_linux.c
index f3d543dd9a98..8addcd166908 100644
--- a/fs/coda/coda_linux.c
+++ b/fs/coda/coda_linux.c
@@ -66,6 +66,32 @@ unsigned short coda_flags_to_cflags(unsigned short flags)
return coda_flags;
}

+static struct timespec64 coda_to_timespec64(struct vtimespec ts)
+{
+ /*
+ * We interpret incoming timestamps as 'signed' to match traditional
+ * usage and support pre-1970 timestamps, but this breaks in y2038
+ * on 32-bit machines.
+ */
+ struct timespec64 ts64 = {
+ .tv_sec = ts.tv_sec,
+ .tv_nsec = ts.tv_nsec,
+ };
+
+ return ts64;
+}
+
+static struct vtimespec timespec64_to_coda(struct timespec64 ts64)
+{
+ /* clamp the timestamps to the maximum range rather than wrapping */
+ struct vtimespec ts = {
+ .tv_sec = lower_32_bits(clamp_t(time64_t, ts64.tv_sec,
+ LONG_MIN, LONG_MAX)),
+ .tv_nsec = ts64.tv_nsec,
+ };
+
+ return ts;
+}

/* utility functions below */
void coda_vattr_to_iattr(struct inode *inode, struct coda_vattr *attr)
@@ -105,11 +131,11 @@ void coda_vattr_to_iattr(struct inode *inode, struct coda_vattr *attr)
if (attr->va_size != -1)
inode->i_blocks = (attr->va_size + 511) >> 9;
if (attr->va_atime.tv_sec != -1)
- inode->i_atime = timespec_to_timespec64(attr->va_atime);
+ inode->i_atime = coda_to_timespec64(attr->va_atime);
if (attr->va_mtime.tv_sec != -1)
- inode->i_mtime = timespec_to_timespec64(attr->va_mtime);
+ inode->i_mtime = coda_to_timespec64(attr->va_mtime);
if (attr->va_ctime.tv_sec != -1)
- inode->i_ctime = timespec_to_timespec64(attr->va_ctime);
+ inode->i_ctime = coda_to_timespec64(attr->va_ctime);
}


@@ -130,12 +156,12 @@ void coda_iattr_to_vattr(struct iattr *iattr, struct coda_vattr *vattr)
vattr->va_uid = (vuid_t) -1;
vattr->va_gid = (vgid_t) -1;
vattr->va_size = (off_t) -1;
- vattr->va_atime.tv_sec = (time_t) -1;
- vattr->va_atime.tv_nsec = (time_t) -1;
- vattr->va_mtime.tv_sec = (time_t) -1;
- vattr->va_mtime.tv_nsec = (time_t) -1;
- vattr->va_ctime.tv_sec = (time_t) -1;
- vattr->va_ctime.tv_nsec = (time_t) -1;
+ vattr->va_atime.tv_sec = (long) -1;
+ vattr->va_atime.tv_nsec = (long) -1;
+ vattr->va_mtime.tv_sec = (long) -1;
+ vattr->va_mtime.tv_nsec = (long) -1;
+ vattr->va_ctime.tv_sec = (long) -1;
+ vattr->va_ctime.tv_nsec = (long) -1;
vattr->va_type = C_VNON;
vattr->va_fileid = -1;
vattr->va_gen = -1;
@@ -175,13 +201,13 @@ void coda_iattr_to_vattr(struct iattr *iattr, struct coda_vattr *vattr)
vattr->va_size = iattr->ia_size;
}
if ( valid & ATTR_ATIME ) {
- vattr->va_atime = timespec64_to_timespec(iattr->ia_atime);
+ vattr->va_atime = timespec64_to_coda(iattr->ia_atime);
}
if ( valid & ATTR_MTIME ) {
- vattr->va_mtime = timespec64_to_timespec(iattr->ia_mtime);
+ vattr->va_mtime = timespec64_to_coda(iattr->ia_mtime);
}
if ( valid & ATTR_CTIME ) {
- vattr->va_ctime = timespec64_to_timespec(iattr->ia_ctime);
+ vattr->va_ctime = timespec64_to_coda(iattr->ia_ctime);
}
}

diff --git a/include/uapi/linux/coda.h b/include/uapi/linux/coda.h
index 695fade33c64..027a8eb04423 100644
--- a/include/uapi/linux/coda.h
+++ b/include/uapi/linux/coda.h
@@ -211,6 +211,20 @@ struct CodaFid {
*/
enum coda_vtype { C_VNON, C_VREG, C_VDIR, C_VBLK, C_VCHR, C_VLNK, C_VSOCK, C_VFIFO, C_VBAD };

+#ifdef __linux__
+/*
+ * This matches the traditional Linux 'timespec' structure binary layout,
+ * before using 64-bit time_t everywhere. Overflows in y2038 on 32-bit
+ * architectures.
+ */
+struct vtimespec {
+ long tv_sec; /* seconds */
+ long tv_nsec; /* nanoseconds */
+};
+#else
+#define vtimespec timespec
+#endif
+
struct coda_vattr {
long va_type; /* vnode type (for create) */
u_short va_mode; /* files access mode and type */
@@ -220,9 +234,9 @@ struct coda_vattr {
long va_fileid; /* file id */
u_quad_t va_size; /* file size in bytes */
long va_blocksize; /* blocksize preferred for i/o */
- struct timespec va_atime; /* time of last access */
- struct timespec va_mtime; /* time of last modification */
- struct timespec va_ctime; /* time file changed */
+ struct vtimespec va_atime; /* time of last access */
+ struct vtimespec va_mtime; /* time of last modification */
+ struct vtimespec va_ctime; /* time file changed */
u_long va_gen; /* generation number of file */
u_long va_flags; /* flags defined for file */
cdev_t va_rdev; /* device special file represents */
--
2.9.0



2018-06-19 17:23:37

by Jan Harkes

[permalink] [raw]
Subject: Re: [PATCH] coda: stop using 'struct timespec' in user API

On Tue, Jun 19, 2018 at 05:37:35PM +0200, Arnd Bergmann wrote:
> Unfortunately, this breaks the layout of the coda_vattr structure, so
> we need to redefine that in terms of something that does not change.
> I'm introducing a new 'struct vtimespec' structure here that keeps
> the existing layout, and the same change has to be done in the coda
> user space copy of linux/coda.h before anyone can use that on a 32-bit
> architecture with 64-bit time_t.

This looks good to me.

> An open question is what should happen to actual times past y2038,
> as they are now truncated to the last valid date when sent to user

That is definitely quite a hard problem because this propagates all the
way back to the Coda file servers and how they store metadata.
In fact the existing client-server protocol only uses 32-bit time in
seconds, so we already lose the nanosecond resolution and 64-bit systems
don't actually benefit from having the extra bits in their struct timespec.

Not exposing an internal kernel datatype is definitely an improvement,
so this is an ACK for me.

Jan


2018-06-19 19:16:13

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] coda: stop using 'struct timespec' in user API

On Tue, Jun 19, 2018 at 6:56 PM, Jan Harkes <[email protected]> wrote:
> On Tue, Jun 19, 2018 at 05:37:35PM +0200, Arnd Bergmann wrote:
>> Unfortunately, this breaks the layout of the coda_vattr structure, so
>> we need to redefine that in terms of something that does not change.
>> I'm introducing a new 'struct vtimespec' structure here that keeps
>> the existing layout, and the same change has to be done in the coda
>> user space copy of linux/coda.h before anyone can use that on a 32-bit
>> architecture with 64-bit time_t.
>
> This looks good to me.
>
>> An open question is what should happen to actual times past y2038,
>> as they are now truncated to the last valid date when sent to user
>
> That is definitely quite a hard problem because this propagates all the
> way back to the Coda file servers and how they store metadata.
> In fact the existing client-server protocol only uses 32-bit time in
> seconds, so we already lose the nanosecond resolution and 64-bit systems
> don't actually benefit from having the extra bits in their struct timespec.
>
> Not exposing an internal kernel datatype is definitely an improvement,
> so this is an ACK for me.

Thanks,

2018-06-19 19:20:24

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] coda: stop using 'struct timespec' in user API

On Tue, Jun 19, 2018 at 9:13 PM, Arnd Bergmann <[email protected]> wrote:
> On Tue, Jun 19, 2018 at 6:56 PM, Jan Harkes <[email protected]> wrote:

>>> An open question is what should happen to actual times past y2038,
>>> as they are now truncated to the last valid date when sent to user
>>
>> That is definitely quite a hard problem because this propagates all the
>> way back to the Coda file servers and how they store metadata.
>> In fact the existing client-server protocol only uses 32-bit time in
>> seconds, so we already lose the nanosecond resolution and 64-bit systems
>> don't actually benefit from having the extra bits in their struct timespec.

I couldn't find out enough background for this, maybe you can fill it
in: I see that there is a user space component and a server component,
but I'm not sure if there is exactly one of each, or if there are multiple
implementations that are written against the same interface.

If we only have one code base, it should be fairly straightforward to
make it deal with 'unsigned' timestamps consistently, which would
let the code work fine until 2106 rather than wrapping around from
2038 to 1902.

> > Not exposing an internal kernel datatype is definitely an improvement,
> > so this is an ACK for me.

To clarify: the problem isn't as much the internal kernel type, but the
glibc internal type that we know is going to change with future glibc
versions. This means it is important that the header file change makes
it into every user space program that uses the psdev interface.

Arnd