diff mbox series

[RESEND] coda: stop using 'struct timespec' in user API

Message ID 20180718114645.591551-1-arnd@arndb.de
State New
Headers show
Series [RESEND] coda: stop using 'struct timespec' in user API | expand

Commit Message

Arnd Bergmann July 18, 2018, 11:46 a.m. UTC 
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.

Acked-by: Jan Harkes <jaharkes@cs.cmu.edu>

Link: https://patchwork.kernel.org/patch/10474735/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
Originally sent on June 19, which lead to a short discussion
and an Ack, but the patch did not get picked up for 4.19 yet.

I'd still like to get a reply on my last point:

On Tue, Jun 19, 2018 at 9:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 19, 2018 at 6:56 PM, Jan Harkes <jaharkes@cs.cmu.edu> wrote:

>> 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.

---
 Documentation/filesystems/coda.txt | 11 ++++++---
 fs/coda/coda_linux.c               | 50 +++++++++++++++++++++++++++++---------
 include/uapi/linux/coda.h          | 20 ++++++++++++---
 3 files changed, 62 insertions(+), 19 deletions(-)

-- 
2.9.0

Comments

Jan Harkes July 18, 2018, 3:50 p.m. UTC | #1 
On Wed, Jul 18, 2018 at 01:46:25PM +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.


I think the userbase is small enough that we can handle a much simpler
transition to 64-bit timespecs everywhere. In that case the
CODA_KERNEL_VERSION should be updated, which is currently defined in
include/uapi/linux/coda.h as 3. As moving to 64-bit timespecs only
breaks 32-bit systems this allows userspace to catch that case and
refuse to run userspace with a mismatched layout (or handle
translation).

It also would make how to handle questions about truncation moot, or at
least moves the problem out of the kernel into userspace.  We actually
were already using unsigned integers for timestamps in the client <->
server protocol, so as you noted, that does give us a little breather
until 2106.

> Originally sent on June 19, which lead to a short discussion

> and an Ack, but the patch did not get picked up for 4.19 yet.


I'm sorry, somehow I missed the follow up questions in that discussion.

> > 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.


At some point there was a webdav filesystem that used the Coda kernel
apis, but I think they may have moved to FUSE since then so I would not
be surprised if there is only a single code base at this point.

Jan
Arnd Bergmann July 18, 2018, 4:10 p.m. UTC | #2 
On Wed, Jul 18, 2018 at 5:50 PM, Jan Harkes <jaharkes@cs.cmu.edu> wrote:
> On Wed, Jul 18, 2018 at 01:46:25PM +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.

>

> I think the userbase is small enough that we can handle a much simpler

> transition to 64-bit timespecs everywhere. In that case the

> CODA_KERNEL_VERSION should be updated, which is currently defined in

> include/uapi/linux/coda.h as 3. As moving to 64-bit timespecs only

> breaks 32-bit systems this allows userspace to catch that case and

> refuse to run userspace with a mismatched layout (or handle

> translation).


Ok, so to make sure I get this right, you say we can do an
incompatible ABI change for coda without causing any problems
for existing users?

That would definitely be the easiest approach here. I guess
we also just have to be incompatible for 32-bit user space,
since it would make 32-bit users have the same ABI as 64-bit
ones, right?

I'll have another look at the ABI side then, to see how it can
be transitioned.

> It also would make how to handle questions about truncation moot, or at

> least moves the problem out of the kernel into userspace.  We actually

> were already using unsigned integers for timestamps in the client <->

> server protocol, so as you noted, that does give us a little breather

> until 2106.


ok.

>> Originally sent on June 19, which lead to a short discussion

>> and an Ack, but the patch did not get picked up for 4.19 yet.

>

> I'm sorry, somehow I missed the follow up questions in that discussion.

>

>> > 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.

>

> At some point there was a webdav filesystem that used the Coda kernel

> apis, but I think they may have moved to FUSE since then so I would not

> be surprised if there is only a single code base at this point.


Ok, I found davfs2 at http://dav.sourceforge.net/index.shtml

Ah, so the coda kernel implementation is similar to both fuse and 9pfs
in that it can connect to arbitrary user space implementations, but with
no known users other than your coda user space and some versions of
davfs2?

      Arnd
Jan Harkes July 18, 2018, 4:31 p.m. UTC | #3 
On Wed, Jul 18, 2018 at 06:10:29PM +0200, Arnd Bergmann wrote:
> On Wed, Jul 18, 2018 at 5:50 PM, Jan Harkes <jaharkes@cs.cmu.edu> wrote:

> > On Wed, Jul 18, 2018 at 01:46:25PM +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.

> >

> > I think the userbase is small enough that we can handle a much simpler

> > transition to 64-bit timespecs everywhere. In that case the

> > CODA_KERNEL_VERSION should be updated, which is currently defined in

> > include/uapi/linux/coda.h as 3. As moving to 64-bit timespecs only

> > breaks 32-bit systems this allows userspace to catch that case and

> > refuse to run userspace with a mismatched layout (or handle

> > translation).

> 

> Ok, so to make sure I get this right, you say we can do an

> incompatible ABI change for coda without causing any problems

> for existing users?

> 

> That would definitely be the easiest approach here. I guess

> we also just have to be incompatible for 32-bit user space,

> since it would make 32-bit users have the same ABI as 64-bit

> ones, right?

> 

> I'll have another look at the ABI side then, to see how it can

> be transitioned.


Correct, the first thing a client does after opening the /dev/cfs0
device is to send a CIOC_KERNEL_VERSION ioctl. In response the Coda
kernel module returns the current value of CODA_KERNEL_VERSION.

Right now anything but 3 will make the client complain about version
mismatch and refuse to start. It is trivial to allow existing 64-bit
clients to accept both 3 and 4 as valid, and when 32-bit userspace is
updated to also use 64-bit timespec it can be changed to accept only 4.

> >> > 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.

> >

> > At some point there was a webdav filesystem that used the Coda kernel

> > apis, but I think they may have moved to FUSE since then so I would not

> > be surprised if there is only a single code base at this point.

> 

> Ok, I found davfs2 at http://dav.sourceforge.net/index.shtml

> 

> Ah, so the coda kernel implementation is similar to both fuse and 9pfs

> in that it can connect to arbitrary user space implementations, but with

> no known users other than your coda user space and some versions of

> davfs2?


Correct and since the FUSE api is easier to work with it has seen more
users. AFAIK, the work on davfs2 was started before FUSE existed.

Jan
diff mbox series

Patch

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 */