These are updates to devidce drivers and file systems that for some reason or another were not included in the kernel in the previous y2038 series.
I've gone through all users of time_t again to make sure the kernel is in a long-term maintainable state.
Posting these as a series for better organization, but each change here is applicable standalone.
Please merge, review, ack/nack etc as you see fit. My plan is to include any patches that don't get a reply this time around in a future pull request, probably for linux-5.6.
As mentioned before, the full series of 90 patches is available at https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y...
Arnd
Arnd Bergmann (16): staging: exfat: use prandom_u32() for i_generation fat: use prandom_u32() for i_generation net: sock: use __kernel_old_timespec instead of timespec dlm: use SO_SNDTIMEO_NEW instead of SO_SNDTIMEO_OLD xtensa: ISS: avoid struct timeval um: ubd: use 64-bit time_t where possible acct: stop using get_seconds() tsacct: add 64-bit btime field netfilter: nft_meta: use 64-bit time arithmetic packet: clarify timestamp overflow quota: avoid time_t in v1_disk_dqblk definition hostfs: pass 64-bit timestamps to/from user space hfs/hfsplus: use 64-bit inode timestamps drm/msm: avoid using 'timespec' drm/etnaviv: use ktime_t for timeouts firewire: ohci: stop using get_seconds() for BUS_TIME
arch/um/drivers/cow.h | 2 +- arch/um/drivers/cow_user.c | 7 +++-- arch/um/drivers/ubd_kern.c | 10 +++---- arch/um/include/shared/os.h | 2 +- arch/um/os-Linux/file.c | 2 +- .../platforms/iss/include/platform/simcall.h | 4 +-- drivers/firewire/ohci.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 19 ++++++------- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 21 ++++++-------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 5 ++-- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4 +-- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 4 +-- drivers/gpu/drm/msm/msm_drv.h | 3 +- drivers/staging/exfat/exfat_super.c | 4 +-- fs/dlm/lowcomms.c | 6 ++-- fs/fat/inode.c | 3 +- fs/hfs/hfs_fs.h | 26 +++++++++++++---- fs/hfs/inode.c | 4 +-- fs/hfsplus/hfsplus_fs.h | 26 +++++++++++++---- fs/hfsplus/inode.c | 12 ++++---- fs/hostfs/hostfs.h | 22 +++++++++------ fs/hostfs/hostfs_kern.c | 15 ++++++---- fs/quota/quotaio_v1.h | 6 ++-- include/linux/skbuff.h | 7 +++-- include/uapi/linux/acct.h | 2 ++ include/uapi/linux/taskstats.h | 6 +++- kernel/acct.c | 4 ++- kernel/tsacct.c | 9 ++++-- net/compat.c | 2 +- net/ipv4/tcp.c | 28 +++++++++++-------- net/netfilter/nft_meta.c | 10 +++---- net/packet/af_packet.c | 27 +++++++++++------- net/socket.c | 2 +- 34 files changed, 184 insertions(+), 124 deletions(-)
Similar to commit 46c9a946d766 ("shmem: use monotonic time for i_generation") we should not use the deprecated get_seconds() interface for i_generation.
prandom_u32() is the replacement used in other file systems.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/staging/exfat/exfat_super.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/exfat/exfat_super.c b/drivers/staging/exfat/exfat_super.c index 3b2b0ceb7297..da76c607f589 100644 --- a/drivers/staging/exfat/exfat_super.c +++ b/drivers/staging/exfat/exfat_super.c @@ -26,7 +26,7 @@ #include <linux/sched.h> #include <linux/fs_struct.h> #include <linux/namei.h> - +#include <linux/random.h> #include <linux/string.h> #include <linux/nls.h> #include <linux/mutex.h> @@ -3314,7 +3314,7 @@ static int exfat_fill_inode(struct inode *inode, struct file_id_t *fid) inode->i_uid = sbi->options.fs_uid; inode->i_gid = sbi->options.fs_gid; INC_IVERSION(inode); - inode->i_generation = get_seconds(); + inode->i_generation = prandom_u32();
if (info.Attr & ATTR_SUBDIR) { /* directory */ inode->i_generation &= ~1;
Similar to commit 46c9a946d766 ("shmem: use monotonic time for i_generation") we should not use the deprecated get_seconds() interface for i_generation.
prandom_u32() is the replacement used in other file systems.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- fs/fat/inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c index 5f04c5c810fb..594b05ae16c9 100644 --- a/fs/fat/inode.c +++ b/fs/fat/inode.c @@ -21,6 +21,7 @@ #include <linux/blkdev.h> #include <linux/backing-dev.h> #include <asm/unaligned.h> +#include <linux/random.h> #include <linux/iversion.h> #include "fat.h"
@@ -521,7 +522,7 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de) inode->i_uid = sbi->options.fs_uid; inode->i_gid = sbi->options.fs_gid; inode_inc_iversion(inode); - inode->i_generation = get_seconds(); + inode->i_generation = prandom_u32();
if ((de->attr & ATTR_DIR) && !IS_FREE(de->name)) { inode->i_generation &= ~1;
The 'timespec' type definition and helpers like ktime_to_timespec() or timespec64_to_timespec() should no longer be used in the kernel so we can remove them and avoid introducing y2038 issues in new code.
Change the socket code that needs to pass a timespec to user space for backward compatibility to use __kernel_old_timespec instead. This type has the same layout but with a clearer defined name.
Slightly reformat tcp_recv_timestamp() for consistency after the removal of timespec64_to_timespec().
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/linux/skbuff.h | 7 +++++-- net/compat.c | 2 +- net/ipv4/tcp.c | 28 ++++++++++++++++------------ net/socket.c | 2 +- 4 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 64a395c7f689..6d64ffe92867 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3656,9 +3656,12 @@ static inline void skb_get_new_timestamp(const struct sk_buff *skb, }
static inline void skb_get_timestampns(const struct sk_buff *skb, - struct timespec *stamp) + struct __kernel_old_timespec *stamp) { - *stamp = ktime_to_timespec(skb->tstamp); + struct timespec64 ts = ktime_to_timespec64(skb->tstamp); + + stamp->tv_sec = ts.tv_sec; + stamp->tv_nsec = ts.tv_nsec; }
static inline void skb_get_new_timestampns(const struct sk_buff *skb, diff --git a/net/compat.c b/net/compat.c index 0f7ded26059e..47d99c784947 100644 --- a/net/compat.c +++ b/net/compat.c @@ -232,7 +232,7 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *dat (type == SO_TIMESTAMPNS_OLD || type == SO_TIMESTAMPING_OLD)) { int count = type == SO_TIMESTAMPNS_OLD ? 1 : 3; int i; - struct timespec *ts = (struct timespec *)data; + struct __kernel_old_timespec *ts = data; for (i = 0; i < count; i++) { cts[i].tv_sec = ts[i].tv_sec; cts[i].tv_nsec = ts[i].tv_nsec; diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index d8876f0e9672..013f635db19c 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -1864,29 +1864,33 @@ static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { if (new_tstamp) { - struct __kernel_timespec kts = {tss->ts[0].tv_sec, tss->ts[0].tv_nsec}; - + struct __kernel_timespec kts = { + .tv_sec = tss->ts[0].tv_sec, + .tv_nsec = tss->ts[0].tv_nsec, + }; put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, sizeof(kts), &kts); } else { - struct timespec ts_old = timespec64_to_timespec(tss->ts[0]); - + struct __kernel_old_timespec ts_old = { + .tv_sec = tss->ts[0].tv_sec, + .tv_nsec = tss->ts[0].tv_nsec, + }; put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, sizeof(ts_old), &ts_old); } } else { if (new_tstamp) { - struct __kernel_sock_timeval stv; - - stv.tv_sec = tss->ts[0].tv_sec; - stv.tv_usec = tss->ts[0].tv_nsec / 1000; + struct __kernel_sock_timeval stv = { + .tv_sec = tss->ts[0].tv_sec, + .tv_usec = tss->ts[0].tv_nsec / 1000, + }; put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, sizeof(stv), &stv); } else { - struct __kernel_old_timeval tv; - - tv.tv_sec = tss->ts[0].tv_sec; - tv.tv_usec = tss->ts[0].tv_nsec / 1000; + struct __kernel_old_timeval tv = { + .tv_sec = tss->ts[0].tv_sec, + .tv_usec = tss->ts[0].tv_nsec / 1000, + }; put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, sizeof(tv), &tv); } diff --git a/net/socket.c b/net/socket.c index 98f6544b0096..9ab00a080760 100644 --- a/net/socket.c +++ b/net/socket.c @@ -793,7 +793,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, sizeof(ts), &ts); } else { - struct timespec ts; + struct __kernel_old_timespec ts;
skb_get_timestampns(skb, &ts); put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD,
Acked-by: Deepa Dinamani deepa.kernel@gmail.com
Eliminate one more use of 'struct timeval' from the kernel so we can eventually remove the definition as well.
The kernel supports the new format with a 64-bit time_t version of timeval here, so use that instead of the old timeval.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- fs/dlm/lowcomms.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c index 3951d39b9b75..cdfaf4f0e11a 100644 --- a/fs/dlm/lowcomms.c +++ b/fs/dlm/lowcomms.c @@ -1035,7 +1035,7 @@ static void sctp_connect_to_sock(struct connection *con) int result; int addr_len; struct socket *sock; - struct timeval tv = { .tv_sec = 5, .tv_usec = 0 }; + struct __kernel_sock_timeval tv = { .tv_sec = 5, .tv_usec = 0 };
if (con->nodeid == 0) { log_print("attempt to connect sock 0 foiled"); @@ -1087,12 +1087,12 @@ static void sctp_connect_to_sock(struct connection *con) * since O_NONBLOCK argument in connect() function does not work here, * then, we should restore the default value of this attribute. */ - kernel_setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO_OLD, (char *)&tv, + kernel_setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO_NEW, (char *)&tv, sizeof(tv)); result = sock->ops->connect(sock, (struct sockaddr *)&daddr, addr_len, 0); memset(&tv, 0, sizeof(tv)); - kernel_setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO_OLD, (char *)&tv, + kernel_setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO_NEW, (char *)&tv, sizeof(tv));
if (result == -EINPROGRESS)
Acked-by: Deepa Dinamani deepa.kernel@gmail.com
'struct timeval' will get removed from the kernel, change the one user in arch/xtensa to avoid referencing it, by using a fixed-length array instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- arch/xtensa/platforms/iss/include/platform/simcall.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/xtensa/platforms/iss/include/platform/simcall.h b/arch/xtensa/platforms/iss/include/platform/simcall.h index 2ba45858e50a..4e2a48380dbf 100644 --- a/arch/xtensa/platforms/iss/include/platform/simcall.h +++ b/arch/xtensa/platforms/iss/include/platform/simcall.h @@ -113,9 +113,9 @@ static inline int simc_write(int fd, const void *buf, size_t count)
static inline int simc_poll(int fd) { - struct timeval tv = { .tv_sec = 0, .tv_usec = 0 }; + long timeval[2] = { 0, 0 };
- return __simc(SYS_select_one, fd, XTISS_SELECT_ONE_READ, (int)&tv); + return __simc(SYS_select_one, fd, XTISS_SELECT_ONE_READ, (int)&timeval); }
static inline int simc_lseek(int fd, uint32_t off, int whence)
On Fri, Nov 8, 2019 at 1:34 PM Arnd Bergmann arnd@arndb.de wrote:
'struct timeval' will get removed from the kernel, change the one user in arch/xtensa to avoid referencing it, by using a fixed-length array instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de
arch/xtensa/platforms/iss/include/platform/simcall.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by: Max Filippov jcmvbkbc@gmail.com
The ubd code suffers from a possible y2038 overflow on 32-bit architectures, both for the cow header and the os_file_modtime() function.
Replace time_t with time64_t to extend the ubd_kern side as much as possible.
Whether this makes a difference for the user side depends on the host libc implementation that may use either 32-bit or 64-bit time_t.
For the cow file format, the header contains an unsigned 32-bit timestamp, which is good until y2106, passing this through a 'long long' gives us a consistent interpretation between 32-bit and 64-bit um kernels.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- arch/um/drivers/cow.h | 2 +- arch/um/drivers/cow_user.c | 7 ++++--- arch/um/drivers/ubd_kern.c | 10 +++++----- arch/um/include/shared/os.h | 2 +- arch/um/os-Linux/file.c | 2 +- 5 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/arch/um/drivers/cow.h b/arch/um/drivers/cow.h index 760c507dd5b6..103adac691ed 100644 --- a/arch/um/drivers/cow.h +++ b/arch/um/drivers/cow.h @@ -11,7 +11,7 @@ extern int init_cow_file(int fd, char *cow_file, char *backing_file, extern int file_reader(__u64 offset, char *buf, int len, void *arg); extern int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg, __u32 *version_out, - char **backing_file_out, time_t *mtime_out, + char **backing_file_out, long long *mtime_out, unsigned long long *size_out, int *sectorsize_out, __u32 *align_out, int *bitmap_offset_out);
diff --git a/arch/um/drivers/cow_user.c b/arch/um/drivers/cow_user.c index 74b0c2686c95..29b46581ddd1 100644 --- a/arch/um/drivers/cow_user.c +++ b/arch/um/drivers/cow_user.c @@ -17,6 +17,7 @@
#define PATH_LEN_V1 256
+/* unsigned time_t works until year 2106 */ typedef __u32 time32_t;
struct cow_header_v1 { @@ -197,7 +198,7 @@ int write_cow_header(char *cow_file, int fd, char *backing_file, int sectorsize, int alignment, unsigned long long *size) { struct cow_header_v3 *header; - unsigned long modtime; + long long modtime; int err;
err = cow_seek_file(fd, 0); @@ -276,7 +277,7 @@ int file_reader(__u64 offset, char *buf, int len, void *arg)
int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg, __u32 *version_out, char **backing_file_out, - time_t *mtime_out, unsigned long long *size_out, + long long *mtime_out, unsigned long long *size_out, int *sectorsize_out, __u32 *align_out, int *bitmap_offset_out) { @@ -363,7 +364,7 @@ int read_cow_header(int (*reader)(__u64, char *, int, void *), void *arg,
/* * this was used until Dec2005 - 64bits are needed to represent - * 2038+. I.e. we can safely do this truncating cast. + * 2106+. I.e. we can safely do this truncating cast. * * Additionally, we must use be32toh() instead of be64toh(), since * the program used to use the former (tested - I got mtime diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 6627d7c30f37..dcabb463e011 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -561,7 +561,7 @@ static inline int ubd_file_size(struct ubd *ubd_dev, __u64 *size_out) __u32 version; __u32 align; char *backing_file; - time_t mtime; + time64_t mtime; unsigned long long size; int sector_size; int bitmap_offset; @@ -600,9 +600,9 @@ static int read_cow_bitmap(int fd, void *buf, int offset, int len) return 0; }
-static int backing_file_mismatch(char *file, __u64 size, time_t mtime) +static int backing_file_mismatch(char *file, __u64 size, time64_t mtime) { - unsigned long modtime; + time64_t modtime; unsigned long long actual; int err;
@@ -628,7 +628,7 @@ static int backing_file_mismatch(char *file, __u64 size, time_t mtime) return -EINVAL; } if (modtime != mtime) { - printk(KERN_ERR "mtime mismatch (%ld vs %ld) of COW header vs " + printk(KERN_ERR "mtime mismatch (%lld vs %lld) of COW header vs " "backing file\n", mtime, modtime); return -EINVAL; } @@ -671,7 +671,7 @@ static int open_ubd_file(char *file, struct openflags *openflags, int shared, unsigned long *bitmap_len_out, int *data_offset_out, int *create_cow_out) { - time_t mtime; + time64_t mtime; unsigned long long size; __u32 version, align; char *backing_file; diff --git a/arch/um/include/shared/os.h b/arch/um/include/shared/os.h index 506bcd1bca68..0f30204b6afa 100644 --- a/arch/um/include/shared/os.h +++ b/arch/um/include/shared/os.h @@ -150,7 +150,7 @@ extern int os_sync_file(int fd); extern int os_file_size(const char *file, unsigned long long *size_out); extern int os_pread_file(int fd, void *buf, int len, unsigned long long offset); extern int os_pwrite_file(int fd, const void *buf, int count, unsigned long long offset); -extern int os_file_modtime(const char *file, unsigned long *modtime); +extern int os_file_modtime(const char *file, long long *modtime); extern int os_pipe(int *fd, int stream, int close_on_exec); extern int os_set_fd_async(int fd); extern int os_clear_fd_async(int fd); diff --git a/arch/um/os-Linux/file.c b/arch/um/os-Linux/file.c index 5133e3afb96f..fbda10535dab 100644 --- a/arch/um/os-Linux/file.c +++ b/arch/um/os-Linux/file.c @@ -341,7 +341,7 @@ int os_file_size(const char *file, unsigned long long *size_out) return 0; }
-int os_file_modtime(const char *file, unsigned long *modtime) +int os_file_modtime(const char *file, long long *modtime) { struct uml_stat buf; int err;
In 'struct acct', 'struct acct_v3', and 'struct taskstats' we have a 32-bit 'ac_btime' field containing an absolute time value, which will overflow in year 2106.
There are two possible ways to deal with it:
a) let it overflow and have user space code deal with reconstructing the data based on the current time, or b) truncate the times based on the range of the u32 type.
Neither of them solves the actual problem. Pick the second one to best document what the issue is, and have someone fix it in a future version.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/uapi/linux/acct.h | 2 ++ include/uapi/linux/taskstats.h | 1 + kernel/acct.c | 4 +++- kernel/tsacct.c | 8 +++++--- 4 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/acct.h b/include/uapi/linux/acct.h index 0e72172cd23a..985b89068591 100644 --- a/include/uapi/linux/acct.h +++ b/include/uapi/linux/acct.h @@ -49,6 +49,7 @@ struct acct __u16 ac_uid16; /* LSB of Real User ID */ __u16 ac_gid16; /* LSB of Real Group ID */ __u16 ac_tty; /* Control Terminal */ + /* __u32 range means times from 1970 to 2106 */ __u32 ac_btime; /* Process Creation Time */ comp_t ac_utime; /* User Time */ comp_t ac_stime; /* System Time */ @@ -81,6 +82,7 @@ struct acct_v3 __u32 ac_gid; /* Real Group ID */ __u32 ac_pid; /* Process ID */ __u32 ac_ppid; /* Parent Process ID */ + /* __u32 range means times from 1970 to 2106 */ __u32 ac_btime; /* Process Creation Time */ #ifdef __KERNEL__ __u32 ac_etime; /* Elapsed Time */ diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h index 5e8ca16a9079..7d3ea366e93b 100644 --- a/include/uapi/linux/taskstats.h +++ b/include/uapi/linux/taskstats.h @@ -112,6 +112,7 @@ struct taskstats { __u32 ac_gid; /* Group ID */ __u32 ac_pid; /* Process ID */ __u32 ac_ppid; /* Parent process ID */ + /* __u32 range means times from 1970 to 2106 */ __u32 ac_btime; /* Begin time [sec since 1970] */ __u64 ac_etime __attribute__((aligned(8))); /* Elapsed time [usec] */ diff --git a/kernel/acct.c b/kernel/acct.c index 81f9831a7859..11ff4a596d6b 100644 --- a/kernel/acct.c +++ b/kernel/acct.c @@ -416,6 +416,7 @@ static void fill_ac(acct_t *ac) { struct pacct_struct *pacct = ¤t->signal->pacct; u64 elapsed, run_time; + time64_t btime; struct tty_struct *tty;
/* @@ -448,7 +449,8 @@ static void fill_ac(acct_t *ac) } #endif do_div(elapsed, AHZ); - ac->ac_btime = get_seconds() - elapsed; + btime = ktime_get_real_seconds() - elapsed; + ac->ac_btime = clamp_t(time64_t, btime, 0, U32_MAX); #if ACCT_VERSION==2 ac->ac_ahz = AHZ; #endif diff --git a/kernel/tsacct.c b/kernel/tsacct.c index 7be3e7530841..ab12616ee6fb 100644 --- a/kernel/tsacct.c +++ b/kernel/tsacct.c @@ -24,6 +24,7 @@ void bacct_add_tsk(struct user_namespace *user_ns, const struct cred *tcred; u64 utime, stime, utimescaled, stimescaled; u64 delta; + time64_t btime;
BUILD_BUG_ON(TS_COMM_LEN < TASK_COMM_LEN);
@@ -32,9 +33,10 @@ void bacct_add_tsk(struct user_namespace *user_ns, /* Convert to micro seconds */ do_div(delta, NSEC_PER_USEC); stats->ac_etime = delta; - /* Convert to seconds for btime */ - do_div(delta, USEC_PER_SEC); - stats->ac_btime = get_seconds() - delta; + /* Convert to seconds for btime (note y2106 limit) */ + btime = ktime_get_real_seconds() - div_u64(delta, USEC_PER_SEC); + stats->ac_btime = clamp_t(time64_t, btime, 0, U32_MAX); + if (thread_group_leader(tsk)) { stats->ac_exitcode = tsk->exit_code; if (tsk->flags & PF_FORKNOEXEC)
As there is only a 32-bit ac_btime field in taskstat and we should handle dates after the overflow, add a new field with the same information but 64-bit width that can hold a full time64_t.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/uapi/linux/taskstats.h | 5 ++++- kernel/tsacct.c | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h index 7d3ea366e93b..ccbd08709321 100644 --- a/include/uapi/linux/taskstats.h +++ b/include/uapi/linux/taskstats.h @@ -34,7 +34,7 @@ */
-#define TASKSTATS_VERSION 9 +#define TASKSTATS_VERSION 10 #define TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN * in linux/sched.h */
@@ -169,6 +169,9 @@ struct taskstats { /* Delay waiting for thrashing page */ __u64 thrashing_count; __u64 thrashing_delay_total; + + /* v10: 64-bit btime to avoid overflow */ + __u64 ac_btime64; /* 64-bit begin time */ };
diff --git a/kernel/tsacct.c b/kernel/tsacct.c index ab12616ee6fb..257ffb993ea2 100644 --- a/kernel/tsacct.c +++ b/kernel/tsacct.c @@ -36,6 +36,7 @@ void bacct_add_tsk(struct user_namespace *user_ns, /* Convert to seconds for btime (note y2106 limit) */ btime = ktime_get_real_seconds() - div_u64(delta, USEC_PER_SEC); stats->ac_btime = clamp_t(time64_t, btime, 0, U32_MAX); + stats->ac_btime64 = btime;
if (thread_group_leader(tsk)) { stats->ac_exitcode = tsk->exit_code;
On 32-bit architectures, get_seconds() returns an unsigned 32-bit time value, which also matches the type used in the nft_meta code. This will not overflow in year 2038 as a time_t would, but it still suffers from the overflow problem later on in year 2106.
Change this instance to use the time64_t type consistently and avoid the deprecated get_seconds().
The nft_meta_weekday() calculation potentially gets a little slower on 32-bit architectures, but now it has the same behavior as on 64-bit architectures and does not overflow.
Fixes: 63d10e12b00d ("netfilter: nft_meta: support for time matching") Signed-off-by: Arnd Bergmann arnd@arndb.de --- net/netfilter/nft_meta.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c index 317e3a9e8c5b..dda1e55d5801 100644 --- a/net/netfilter/nft_meta.c +++ b/net/netfilter/nft_meta.c @@ -33,19 +33,19 @@
static DEFINE_PER_CPU(struct rnd_state, nft_prandom_state);
-static u8 nft_meta_weekday(unsigned long secs) +static u8 nft_meta_weekday(time64_t secs) { unsigned int dse; u8 wday;
secs -= NFT_META_SECS_PER_MINUTE * sys_tz.tz_minuteswest; - dse = secs / NFT_META_SECS_PER_DAY; + dse = div_u64(secs, NFT_META_SECS_PER_DAY); wday = (4 + dse) % NFT_META_DAYS_PER_WEEK;
return wday; }
-static u32 nft_meta_hour(unsigned long secs) +static u32 nft_meta_hour(time64_t secs) { struct tm tm;
@@ -250,10 +250,10 @@ void nft_meta_get_eval(const struct nft_expr *expr, nft_reg_store64(dest, ktime_get_real_ns()); break; case NFT_META_TIME_DAY: - nft_reg_store8(dest, nft_meta_weekday(get_seconds())); + nft_reg_store8(dest, nft_meta_weekday(ktime_get_real_seconds())); break; case NFT_META_TIME_HOUR: - *dest = nft_meta_hour(get_seconds()); + *dest = nft_meta_hour(ktime_get_real_seconds()); break; default: WARN_ON(1);
On Fri, Nov 08, 2019 at 10:32:47PM +0100, Arnd Bergmann wrote:
On 32-bit architectures, get_seconds() returns an unsigned 32-bit time value, which also matches the type used in the nft_meta code. This will not overflow in year 2038 as a time_t would, but it still suffers from the overflow problem later on in year 2106.
I wonder if the assumption that people will still use nft_meta 80 years from now is an optimistic or pessimistic one. :)
Change this instance to use the time64_t type consistently and avoid the deprecated get_seconds().
The nft_meta_weekday() calculation potentially gets a little slower on 32-bit architectures, but now it has the same behavior as on 64-bit architectures and does not overflow.
Fixes: 63d10e12b00d ("netfilter: nft_meta: support for time matching") Signed-off-by: Arnd Bergmann arnd@arndb.de
Acked-by: Phil Sutter phil@nwl.cc
On Fri, Nov 08, 2019 at 10:32:47PM +0100, Arnd Bergmann wrote:
On 32-bit architectures, get_seconds() returns an unsigned 32-bit time value, which also matches the type used in the nft_meta code. This will not overflow in year 2038 as a time_t would, but it still suffers from the overflow problem later on in year 2106.
Change this instance to use the time64_t type consistently and avoid the deprecated get_seconds().
The nft_meta_weekday() calculation potentially gets a little slower on 32-bit architectures, but now it has the same behavior as on 64-bit architectures and does not overflow.
Applied, thanks Arnd.
The memory mapped packet socket data structure in version 1 through 3 all contain 32-bit second values for the packet time stamps, which makes them suffer from the overflow of time_t in y2038 or y2106 (depending on whether user space interprets the value as signed or unsigned).
The implementation uses the deprecated getnstimeofday() function.
In order to get rid of that, this changes the code to use ktime_get_real_ts64() as a replacement, documenting the nature of the overflow. As long as the user applications treat the timestamps as unsigned, or only use the difference between timestamps, they are fine, and changing the timestamps to 64-bit wouldn't require a more invasive user space API change.
Note: a lot of other APIs suffer from incompatible structures when time_t gets redefined to 64-bit in 32-bit user space, but this one does not.
Acked-by: Willem de Bruijn willemb@google.com Link: https://lore.kernel.org/lkml/CAF=yD-Jomr-gWSR-EBNKnSpFL46UeG564FLfqTCMNEm-pr... Signed-off-by: Arnd Bergmann arnd@arndb.de --- net/packet/af_packet.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 82a50e850245..0bfdb07e253b 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -408,17 +408,17 @@ static int __packet_get_status(const struct packet_sock *po, void *frame) } }
-static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec *ts, +static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec64 *ts, unsigned int flags) { struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
if (shhwtstamps && (flags & SOF_TIMESTAMPING_RAW_HARDWARE) && - ktime_to_timespec_cond(shhwtstamps->hwtstamp, ts)) + ktime_to_timespec64_cond(shhwtstamps->hwtstamp, ts)) return TP_STATUS_TS_RAW_HARDWARE;
- if (ktime_to_timespec_cond(skb->tstamp, ts)) + if (ktime_to_timespec64_cond(skb->tstamp, ts)) return TP_STATUS_TS_SOFTWARE;
return 0; @@ -428,13 +428,20 @@ static __u32 __packet_set_timestamp(struct packet_sock *po, void *frame, struct sk_buff *skb) { union tpacket_uhdr h; - struct timespec ts; + struct timespec64 ts; __u32 ts_status;
if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp))) return 0;
h.raw = frame; + /* + * versions 1 through 3 overflow the timestamps in y2106, since they + * all store the seconds in a 32-bit unsigned integer. + * If we create a version 4, that should have a 64-bit timestamp, + * either 64-bit seconds + 32-bit nanoseconds, or just 64-bit + * nanoseconds. + */ switch (po->tp_version) { case TPACKET_V1: h.h1->tp_sec = ts.tv_sec; @@ -774,8 +781,8 @@ static void prb_close_block(struct tpacket_kbdq_core *pkc1, * It shouldn't really happen as we don't close empty * blocks. See prb_retire_rx_blk_timer_expired(). */ - struct timespec ts; - getnstimeofday(&ts); + struct timespec64 ts; + ktime_get_real_ts64(&ts); h1->ts_last_pkt.ts_sec = ts.tv_sec; h1->ts_last_pkt.ts_nsec = ts.tv_nsec; } @@ -805,7 +812,7 @@ static void prb_thaw_queue(struct tpacket_kbdq_core *pkc) static void prb_open_block(struct tpacket_kbdq_core *pkc1, struct tpacket_block_desc *pbd1) { - struct timespec ts; + struct timespec64 ts; struct tpacket_hdr_v1 *h1 = &pbd1->hdr.bh1;
smp_rmb(); @@ -818,7 +825,7 @@ static void prb_open_block(struct tpacket_kbdq_core *pkc1, BLOCK_NUM_PKTS(pbd1) = 0; BLOCK_LEN(pbd1) = BLK_PLUS_PRIV(pkc1->blk_sizeof_priv);
- getnstimeofday(&ts); + ktime_get_real_ts64(&ts);
h1->ts_first_pkt.ts_sec = ts.tv_sec; h1->ts_first_pkt.ts_nsec = ts.tv_nsec; @@ -2162,7 +2169,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, unsigned long status = TP_STATUS_USER; unsigned short macoff, netoff, hdrlen; struct sk_buff *copy_skb = NULL; - struct timespec ts; + struct timespec64 ts; __u32 ts_status; bool is_drop_n_account = false; bool do_vnet = false; @@ -2294,7 +2301,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, skb_copy_bits(skb, 0, h.raw + macoff, snaplen);
if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp))) - getnstimeofday(&ts); + ktime_get_real_ts64(&ts);
status |= ts_status;
The time_t type is part of the user interface and not always the same, with the move to 64-bit timestamps and the difference between architectures.
Make the quota format definition independent of this type and use a basic type of the same length. Make it unsigned in the process to keep the v1 format working until year 2106 instead of 2038 on 32-bit architectures.
Hopefully, everybody has already moved to a newer format long ago (v2 was introduced with linux-2.4), but it's hard to be sure.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- fs/quota/quotaio_v1.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/quota/quotaio_v1.h b/fs/quota/quotaio_v1.h index bd11e2c08119..31dca9a89176 100644 --- a/fs/quota/quotaio_v1.h +++ b/fs/quota/quotaio_v1.h @@ -25,8 +25,10 @@ struct v1_disk_dqblk { __u32 dqb_ihardlimit; /* absolute limit on allocated inodes */ __u32 dqb_isoftlimit; /* preferred inode limit */ __u32 dqb_curinodes; /* current # allocated inodes */ - time_t dqb_btime; /* time limit for excessive disk use */ - time_t dqb_itime; /* time limit for excessive inode use */ + + /* below fields differ in length on 32-bit vs 64-bit architectures */ + unsigned long dqb_btime; /* time limit for excessive disk use */ + unsigned long dqb_itime; /* time limit for excessive inode use */ };
#define v1_dqoff(UID) ((loff_t)((UID) * sizeof (struct v1_disk_dqblk)))
The use of 'struct timespec' is deprecated in the kernel, so we want to avoid the conversions from/to the proper timespec64 structure.
On the user space side, we have a 'struct timespec' that is defined by the C library and that will be incompatible with the kernel's view on 32-bit architectures once they move to a 64-bit time_t, breaking the shared binary layout of hostfs_iattr and hostfs_stat.
This changes the two structures to use a new hostfs_timespec structure with fixed 64-bit seconds/nanoseconds for passing the timestamps between hostfs_kern.c and hostfs_user.c. With a new enough user space side, this will allow timestamps beyond year 2038.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- fs/hostfs/hostfs.h | 22 +++++++++++++--------- fs/hostfs/hostfs_kern.c | 15 +++++++++------ 2 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h index f4295aa19350..69cb796f6270 100644 --- a/fs/hostfs/hostfs.h +++ b/fs/hostfs/hostfs.h @@ -37,16 +37,20 @@ * is on, and remove the appropriate bits from attr->ia_mode (attr is a * "struct iattr *"). -BlaisorBlade */ +struct hostfs_timespec { + long long tv_sec; + long long tv_nsec; +};
struct hostfs_iattr { - unsigned int ia_valid; - unsigned short ia_mode; - uid_t ia_uid; - gid_t ia_gid; - loff_t ia_size; - struct timespec ia_atime; - struct timespec ia_mtime; - struct timespec ia_ctime; + unsigned int ia_valid; + unsigned short ia_mode; + uid_t ia_uid; + gid_t ia_gid; + loff_t ia_size; + struct hostfs_timespec ia_atime; + struct hostfs_timespec ia_mtime; + struct hostfs_timespec ia_ctime; };
struct hostfs_stat { @@ -56,7 +60,7 @@ struct hostfs_stat { unsigned int uid; unsigned int gid; unsigned long long size; - struct timespec atime, mtime, ctime; + struct hostfs_timespec atime, mtime, ctime; unsigned int blksize; unsigned long long blocks; unsigned int maj; diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c index 5a7eb0c79839..e6b8c49076bb 100644 --- a/fs/hostfs/hostfs_kern.c +++ b/fs/hostfs/hostfs_kern.c @@ -549,9 +549,9 @@ static int read_name(struct inode *ino, char *name) set_nlink(ino, st.nlink); i_uid_write(ino, st.uid); i_gid_write(ino, st.gid); - ino->i_atime = timespec_to_timespec64(st.atime); - ino->i_mtime = timespec_to_timespec64(st.mtime); - ino->i_ctime = timespec_to_timespec64(st.ctime); + ino->i_atime = (struct timespec64){ st.atime.tv_sec, st.atime.tv_nsec }; + ino->i_mtime = (struct timespec64){ st.mtime.tv_sec, st.mtime.tv_nsec }; + ino->i_ctime = (struct timespec64){ st.ctime.tv_sec, st.ctime.tv_nsec }; ino->i_size = st.size; ino->i_blocks = st.blocks; return 0; @@ -820,15 +820,18 @@ static int hostfs_setattr(struct dentry *dentry, struct iattr *attr) } if (attr->ia_valid & ATTR_ATIME) { attrs.ia_valid |= HOSTFS_ATTR_ATIME; - attrs.ia_atime = timespec64_to_timespec(attr->ia_atime); + attrs.ia_atime = (struct hostfs_timespec) + { attr->ia_atime.tv_sec, attr->ia_atime.tv_nsec }; } if (attr->ia_valid & ATTR_MTIME) { attrs.ia_valid |= HOSTFS_ATTR_MTIME; - attrs.ia_mtime = timespec64_to_timespec(attr->ia_mtime); + attrs.ia_mtime = (struct hostfs_timespec) + { attr->ia_mtime.tv_sec, attr->ia_mtime.tv_nsec }; } if (attr->ia_valid & ATTR_CTIME) { attrs.ia_valid |= HOSTFS_ATTR_CTIME; - attrs.ia_ctime = timespec64_to_timespec(attr->ia_ctime); + attrs.ia_ctime = (struct hostfs_timespec) + { attr->ia_ctime.tv_sec, attr->ia_ctime.tv_nsec }; } if (attr->ia_valid & ATTR_ATIME_SET) { attrs.ia_valid |= HOSTFS_ATTR_ATIME_SET;
On Fri, 2019-11-08 at 22:32 +0100, Arnd Bergmann wrote:
The use of 'struct timespec' is deprecated in the kernel, so we want to avoid the conversions from/to the proper timespec64 structure.
On the user space side, we have a 'struct timespec' that is defined by the C library and that will be incompatible with the kernel's view on 32-bit architectures once they move to a 64-bit time_t, breaking the shared binary layout of hostfs_iattr and hostfs_stat.
This changes the two structures to use a new hostfs_timespec structure with fixed 64-bit seconds/nanoseconds for passing the timestamps between hostfs_kern.c and hostfs_user.c. With a new enough user space side, this will allow timestamps beyond year 2038.
[...]
The "user-space" side has a structure assignment in set_attr():
if (attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)) { err = stat_file(file, &st, fd); attrs->ia_atime = st.atime; attrs->ia_mtime = st.mtime; if (err != 0) return err; }
which will also need to be updated for this type change.
Ben.
On Wed, 2019-11-20 at 20:30 +0000, Ben Hutchings wrote:
On Fri, 2019-11-08 at 22:32 +0100, Arnd Bergmann wrote:
The use of 'struct timespec' is deprecated in the kernel, so we want to avoid the conversions from/to the proper timespec64 structure.
On the user space side, we have a 'struct timespec' that is defined by the C library and that will be incompatible with the kernel's view on 32-bit architectures once they move to a 64-bit time_t, breaking the shared binary layout of hostfs_iattr and hostfs_stat.
This changes the two structures to use a new hostfs_timespec structure with fixed 64-bit seconds/nanoseconds for passing the timestamps between hostfs_kern.c and hostfs_user.c. With a new enough user space side, this will allow timestamps beyond year 2038.
[...]
The "user-space" side has a structure assignment in set_attr():
if (attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)) { err = stat_file(file, &st, fd); attrs->ia_atime = st.atime; attrs->ia_mtime = st.mtime; if (err != 0) return err; }
which will also need to be updated for this type change.
Sorry, I'm confused, this looks fine.
Ben.
The interpretation of on-disk timestamps in HFS and HFS+ differs between 32-bit and 64-bit kernels at the moment. Use 64-bit timestamps consistently so apply the current 64-bit behavior everyhere.
According to the official documentation for HFS+ [1], inode timestamps are supposed to cover the time range from 1904 to 2040 as originally used in classic MacOS.
The traditional Linux usage is to convert the timestamps into an unsigned 32-bit number based on the Unix epoch and from there to a time_t. On 32-bit systems, that wraps the time from 2038 to 1902, so the last two years of the valid time range become garbled. On 64-bit systems, all times before 1970 get turned into timestamps between 2038 and 2106, which is more convenient but also different from the documented behavior.
Looking at the Darwin sources [2], it seems that MacOS is inconsistent in yet another way: all timestamps are wrapped around to a 32-bit unsigned number when written to the disk, but when read back, all numeric values lower than 2082844800U are assumed to be invalid, so we cannot represent the times before 1970 or the times after 2040.
While all implementations seem to agree on the interpretation of values between 1970 and 2038, they often differ on the exact range they support when reading back values outside of the common range:
MacOS (traditional): 1904-2040 Apple Documentation: 1904-2040 MacOS X source comments: 1970-2040 MacOS X source code: 1970-2038 32-bit Linux: 1902-2038 64-bit Linux: 1970-2106 hfsfuse: 1970-2040 hfsutils (32 bit, old libc) 1902-2038 hfsutils (32 bit, new libc) 1970-2106 hfsutils (64 bit) 1904-2040 hfsplus-utils 1904-2040 hfsexplorer 1904-2040 7-zip 1904-2040
Out of the above, the range from 1970 to 2106 seems to be the most useful, as it allows using HFS and HFS+ beyond year 2038, and this matches the behavior that most users would see today on Linux, as few people run 32-bit kernels any more.
Link: [1] https://developer.apple.com/library/archive/technotes/tn/tn1150.html Link: [2] https://opensource.apple.com/source/hfs/hfs-407.30.1/core/MacOSStubs.c.auto.... Link: https://lore.kernel.org/lkml/20180711224625.airwna6gzyatoowe@eaf/ Cc: Viacheslav Dubeyko slava@dubeyko.com Suggested-by: "Ernesto A. Fernández" ernesto.mnd.fernandez@gmail.com Signed-off-by: Arnd Bergmann arnd@arndb.de --- v3: revert back to 1970-2106 time range fix bugs found in review merge both patches into one drop cc:stable tag v2: treat pre-1970 dates as invalid following MacOS X behavior, reword and expand changelog text --- fs/hfs/hfs_fs.h | 26 ++++++++++++++++++++------ fs/hfs/inode.c | 4 ++-- fs/hfsplus/hfsplus_fs.h | 26 +++++++++++++++++++++----- fs/hfsplus/inode.c | 12 ++++++------ 4 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index 6d0783e2e276..26733051ee50 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -242,19 +242,33 @@ extern void hfs_mark_mdb_dirty(struct super_block *sb); /* * There are two time systems. Both are based on seconds since * a particular time/date. - * Unix: unsigned lil-endian since 00:00 GMT, Jan. 1, 1970 + * Unix: signed little-endian since 00:00 GMT, Jan. 1, 1970 * mac: unsigned big-endian since 00:00 GMT, Jan. 1, 1904 * + * HFS implementations are highly inconsistent, this one matches the + * traditional behavior of 64-bit Linux, giving the most useful + * time range between 1970 and 2106, by treating any on-disk timestamp + * under 2082844800U (Jan 1 1970) as a time between 2040 and 2106. */ -#define __hfs_u_to_mtime(sec) cpu_to_be32(sec + 2082844800U - sys_tz.tz_minuteswest * 60) -#define __hfs_m_to_utime(sec) (be32_to_cpu(sec) - 2082844800U + sys_tz.tz_minuteswest * 60) +static inline time64_t __hfs_m_to_utime(__be32 mt) +{ + time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U); + + return ut + sys_tz.tz_minuteswest * 60; +}
+static inline __be32 __hfs_u_to_mtime(time64_t ut) +{ + ut -= sys_tz.tz_minuteswest * 60; + + return cpu_to_be32(lower_32_bits(ut) + 2082844800U); +} #define HFS_I(inode) (container_of(inode, struct hfs_inode_info, vfs_inode)) #define HFS_SB(sb) ((struct hfs_sb_info *)(sb)->s_fs_info)
-#define hfs_m_to_utime(time) (struct timespec){ .tv_sec = __hfs_m_to_utime(time) } -#define hfs_u_to_mtime(time) __hfs_u_to_mtime((time).tv_sec) -#define hfs_mtime() __hfs_u_to_mtime(get_seconds()) +#define hfs_m_to_utime(time) (struct timespec64){ .tv_sec = __hfs_m_to_utime(time) } +#define hfs_u_to_mtime(time) __hfs_u_to_mtime((time).tv_sec) +#define hfs_mtime() __hfs_u_to_mtime(ktime_get_real_seconds())
static inline const char *hfs_mdb_name(struct super_block *sb) { diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c index da243c84e93b..2f224b98ee94 100644 --- a/fs/hfs/inode.c +++ b/fs/hfs/inode.c @@ -351,7 +351,7 @@ static int hfs_read_inode(struct inode *inode, void *data) inode->i_mode &= ~hsb->s_file_umask; inode->i_mode |= S_IFREG; inode->i_ctime = inode->i_atime = inode->i_mtime = - timespec_to_timespec64(hfs_m_to_utime(rec->file.MdDat)); + hfs_m_to_utime(rec->file.MdDat); inode->i_op = &hfs_file_inode_operations; inode->i_fop = &hfs_file_operations; inode->i_mapping->a_ops = &hfs_aops; @@ -362,7 +362,7 @@ static int hfs_read_inode(struct inode *inode, void *data) HFS_I(inode)->fs_blocks = 0; inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask); inode->i_ctime = inode->i_atime = inode->i_mtime = - timespec_to_timespec64(hfs_m_to_utime(rec->dir.MdDat)); + hfs_m_to_utime(rec->dir.MdDat); inode->i_op = &hfs_dir_inode_operations; inode->i_fop = &hfs_dir_operations; break; diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index b8471bf05def..22d0a22c41a3 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -533,13 +533,29 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector, void *buf, void **data, int op, int op_flags); int hfsplus_read_wrapper(struct super_block *sb);
-/* time macros */ -#define __hfsp_mt2ut(t) (be32_to_cpu(t) - 2082844800U) -#define __hfsp_ut2mt(t) (cpu_to_be32(t + 2082844800U)) +/* + * time helpers: convert between 1904-base and 1970-base timestamps + * + * HFS+ implementations are highly inconsistent, this one matches the + * traditional behavior of 64-bit Linux, giving the most useful + * time range between 1970 and 2106, by treating any on-disk timestamp + * under 2082844800U (Jan 1 1970) as a time between 2040 and 2106. + */ +static inline time64_t __hfsp_mt2ut(__be32 mt) +{ + time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U); + + return ut; +} + +static inline __be32 __hfsp_ut2mt(time64_t ut) +{ + return cpu_to_be32(lower_32_bits(ut) + 2082844800U); +}
/* compatibility */ -#define hfsp_mt2ut(t) (struct timespec){ .tv_sec = __hfsp_mt2ut(t) } +#define hfsp_mt2ut(t) (struct timespec64){ .tv_sec = __hfsp_mt2ut(t) } #define hfsp_ut2mt(t) __hfsp_ut2mt((t).tv_sec) -#define hfsp_now2mt() __hfsp_ut2mt(get_seconds()) +#define hfsp_now2mt() __hfsp_ut2mt(ktime_get_real_seconds())
#endif diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index d131c8ea7eb6..94bd83b36644 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -504,9 +504,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) hfsplus_get_perms(inode, &folder->permissions, 1); set_nlink(inode, 1); inode->i_size = 2 + be32_to_cpu(folder->valence); - inode->i_atime = timespec_to_timespec64(hfsp_mt2ut(folder->access_date)); - inode->i_mtime = timespec_to_timespec64(hfsp_mt2ut(folder->content_mod_date)); - inode->i_ctime = timespec_to_timespec64(hfsp_mt2ut(folder->attribute_mod_date)); + inode->i_atime = hfsp_mt2ut(folder->access_date); + inode->i_mtime = hfsp_mt2ut(folder->content_mod_date); + inode->i_ctime = hfsp_mt2ut(folder->attribute_mod_date); HFSPLUS_I(inode)->create_date = folder->create_date; HFSPLUS_I(inode)->fs_blocks = 0; if (folder->flags & cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT)) { @@ -542,9 +542,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) init_special_inode(inode, inode->i_mode, be32_to_cpu(file->permissions.dev)); } - inode->i_atime = timespec_to_timespec64(hfsp_mt2ut(file->access_date)); - inode->i_mtime = timespec_to_timespec64(hfsp_mt2ut(file->content_mod_date)); - inode->i_ctime = timespec_to_timespec64(hfsp_mt2ut(file->attribute_mod_date)); + inode->i_atime = hfsp_mt2ut(file->access_date); + inode->i_mtime = hfsp_mt2ut(file->content_mod_date); + inode->i_ctime = hfsp_mt2ut(file->attribute_mod_date); HFSPLUS_I(inode)->create_date = file->create_date; } else { pr_err("bad catalog entry used to create inode\n");
On Fri, Nov 08, 2019 at 10:32:51PM +0100, Arnd Bergmann wrote:
The interpretation of on-disk timestamps in HFS and HFS+ differs between 32-bit and 64-bit kernels at the moment. Use 64-bit timestamps consistently so apply the current 64-bit behavior everyhere.
According to the official documentation for HFS+ [1], inode timestamps are supposed to cover the time range from 1904 to 2040 as originally used in classic MacOS.
The traditional Linux usage is to convert the timestamps into an unsigned 32-bit number based on the Unix epoch and from there to a time_t. On 32-bit systems, that wraps the time from 2038 to 1902, so the last two years of the valid time range become garbled. On 64-bit systems, all times before 1970 get turned into timestamps between 2038 and 2106, which is more convenient but also different from the documented behavior.
Looking at the Darwin sources [2], it seems that MacOS is inconsistent in yet another way: all timestamps are wrapped around to a 32-bit unsigned number when written to the disk, but when read back, all numeric values lower than 2082844800U are assumed to be invalid, so we cannot represent the times before 1970 or the times after 2040.
While all implementations seem to agree on the interpretation of values between 1970 and 2038, they often differ on the exact range they support when reading back values outside of the common range:
MacOS (traditional): 1904-2040 Apple Documentation: 1904-2040 MacOS X source comments: 1970-2040 MacOS X source code: 1970-2038 32-bit Linux: 1902-2038 64-bit Linux: 1970-2106 hfsfuse: 1970-2040 hfsutils (32 bit, old libc) 1902-2038 hfsutils (32 bit, new libc) 1970-2106 hfsutils (64 bit) 1904-2040 hfsplus-utils 1904-2040 hfsexplorer 1904-2040 7-zip 1904-2040
Out of the above, the range from 1970 to 2106 seems to be the most useful, as it allows using HFS and HFS+ beyond year 2038, and this matches the behavior that most users would see today on Linux, as few people run 32-bit kernels any more.
Link: [1] https://developer.apple.com/library/archive/technotes/tn/tn1150.html Link: [2] https://opensource.apple.com/source/hfs/hfs-407.30.1/core/MacOSStubs.c.auto.... Link: https://lore.kernel.org/lkml/20180711224625.airwna6gzyatoowe@eaf/ Cc: Viacheslav Dubeyko slava@dubeyko.com Suggested-by: "Ernesto A. Fernández" ernesto.mnd.fernandez@gmail.com Signed-off-by: Arnd Bergmann arnd@arndb.de
Reviewed-by: Ernesto A. Fernández ernesto.mnd.fernandez@gmail.com
On Nov 9, 2019, at 12:32 AM, Arnd Bergmann arnd@arndb.de wrote:
The interpretation of on-disk timestamps in HFS and HFS+ differs between 32-bit and 64-bit kernels at the moment. Use 64-bit timestamps consistently so apply the current 64-bit behavior everyhere.
According to the official documentation for HFS+ [1], inode timestamps are supposed to cover the time range from 1904 to 2040 as originally used in classic MacOS.
The traditional Linux usage is to convert the timestamps into an unsigned 32-bit number based on the Unix epoch and from there to a time_t. On 32-bit systems, that wraps the time from 2038 to 1902, so the last two years of the valid time range become garbled. On 64-bit systems, all times before 1970 get turned into timestamps between 2038 and 2106, which is more convenient but also different from the documented behavior.
Looking at the Darwin sources [2], it seems that MacOS is inconsistent in yet another way: all timestamps are wrapped around to a 32-bit unsigned number when written to the disk, but when read back, all numeric values lower than 2082844800U are assumed to be invalid, so we cannot represent the times before 1970 or the times after 2040.
While all implementations seem to agree on the interpretation of values between 1970 and 2038, they often differ on the exact range they support when reading back values outside of the common range:
MacOS (traditional): 1904-2040 Apple Documentation: 1904-2040 MacOS X source comments: 1970-2040 MacOS X source code: 1970-2038 32-bit Linux: 1902-2038 64-bit Linux: 1970-2106 hfsfuse: 1970-2040 hfsutils (32 bit, old libc) 1902-2038 hfsutils (32 bit, new libc) 1970-2106 hfsutils (64 bit) 1904-2040 hfsplus-utils 1904-2040 hfsexplorer 1904-2040 7-zip 1904-2040
Out of the above, the range from 1970 to 2106 seems to be the most useful, as it allows using HFS and HFS+ beyond year 2038, and this matches the behavior that most users would see today on Linux, as few people run 32-bit kernels any more.
Link: [1] https://developer.apple.com/library/archive/technotes/tn/tn1150.html Link: [2] https://opensource.apple.com/source/hfs/hfs-407.30.1/core/MacOSStubs.c.auto.... Link: https://lore.kernel.org/lkml/20180711224625.airwna6gzyatoowe@eaf/ Cc: Viacheslav Dubeyko slava@dubeyko.com Suggested-by: "Ernesto A. Fernández" ernesto.mnd.fernandez@gmail.com Signed-off-by: Arnd Bergmann arnd@arndb.de
v3: revert back to 1970-2106 time range fix bugs found in review merge both patches into one drop cc:stable tag v2: treat pre-1970 dates as invalid following MacOS X behavior, reword and expand changelog text
fs/hfs/hfs_fs.h | 26 ++++++++++++++++++++------ fs/hfs/inode.c | 4 ++-- fs/hfsplus/hfsplus_fs.h | 26 +++++++++++++++++++++----- fs/hfsplus/inode.c | 12 ++++++------ 4 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index 6d0783e2e276..26733051ee50 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -242,19 +242,33 @@ extern void hfs_mark_mdb_dirty(struct super_block *sb); /*
- There are two time systems. Both are based on seconds since
- a particular time/date.
- Unix: unsigned lil-endian since 00:00 GMT, Jan. 1, 1970
- Unix: signed little-endian since 00:00 GMT, Jan. 1, 1970
- mac: unsigned big-endian since 00:00 GMT, Jan. 1, 1904
- HFS implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
*/ -#define __hfs_u_to_mtime(sec) cpu_to_be32(sec + 2082844800U - sys_tz.tz_minuteswest * 60) -#define __hfs_m_to_utime(sec) (be32_to_cpu(sec) - 2082844800U + sys_tz.tz_minuteswest * 60)
I believe it makes sense to introduce some constant instead of hardcoded value (2082844800U and 60). It will be easier to understand the code without necessity to take a look into the comments. What do you think?
+static inline time64_t __hfs_m_to_utime(__be32 mt) +{
- time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U);
Ditto.
- return ut + sys_tz.tz_minuteswest * 60;
+}
+static inline __be32 __hfs_u_to_mtime(time64_t ut) +{
- ut -= sys_tz.tz_minuteswest * 60;
- return cpu_to_be32(lower_32_bits(ut) + 2082844800U);
Ditto.
+} #define HFS_I(inode) (container_of(inode, struct hfs_inode_info, vfs_inode)) #define HFS_SB(sb) ((struct hfs_sb_info *)(sb)->s_fs_info)
-#define hfs_m_to_utime(time) (struct timespec){ .tv_sec = __hfs_m_to_utime(time) } -#define hfs_u_to_mtime(time) __hfs_u_to_mtime((time).tv_sec) -#define hfs_mtime() __hfs_u_to_mtime(get_seconds()) +#define hfs_m_to_utime(time) (struct timespec64){ .tv_sec = __hfs_m_to_utime(time) } +#define hfs_u_to_mtime(time) __hfs_u_to_mtime((time).tv_sec) +#define hfs_mtime() __hfs_u_to_mtime(ktime_get_real_seconds())
static inline const char *hfs_mdb_name(struct super_block *sb) { diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c index da243c84e93b..2f224b98ee94 100644 --- a/fs/hfs/inode.c +++ b/fs/hfs/inode.c @@ -351,7 +351,7 @@ static int hfs_read_inode(struct inode *inode, void *data) inode->i_mode &= ~hsb->s_file_umask; inode->i_mode |= S_IFREG; inode->i_ctime = inode->i_atime = inode->i_mtime =
timespec_to_timespec64(hfs_m_to_utime(rec->file.MdDat));
hfs_m_to_utime(rec->file.MdDat);@@ -362,7 +362,7 @@ static int hfs_read_inode(struct inode *inode, void *data) HFS_I(inode)->fs_blocks = 0; inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask); inode->i_ctime = inode->i_atime = inode->i_mtime =
timespec_to_timespec64(hfs_m_to_utime(rec->dir.MdDat));
hfs_m_to_utime(rec->dir.MdDat);diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index b8471bf05def..22d0a22c41a3 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -533,13 +533,29 @@ int hfsplus_submit_bio(struct super_block *sb, sector_t sector, void *buf, void **data, int op, int op_flags); int hfsplus_read_wrapper(struct super_block *sb);
-/* time macros */ -#define __hfsp_mt2ut(t) (be32_to_cpu(t) - 2082844800U) -#define __hfsp_ut2mt(t) (cpu_to_be32(t + 2082844800U))
Ditto.
+/*
- time helpers: convert between 1904-base and 1970-base timestamps
- HFS+ implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
- */
+static inline time64_t __hfsp_mt2ut(__be32 mt) +{
- time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U);
Ditto.
- return ut;
+}
+static inline __be32 __hfsp_ut2mt(time64_t ut) +{
- return cpu_to_be32(lower_32_bits(ut) + 2082844800U);
Ditto.
+}
/* compatibility */ -#define hfsp_mt2ut(t) (struct timespec){ .tv_sec = __hfsp_mt2ut(t) } +#define hfsp_mt2ut(t) (struct timespec64){ .tv_sec = __hfsp_mt2ut(t) } #define hfsp_ut2mt(t) __hfsp_ut2mt((t).tv_sec) -#define hfsp_now2mt() __hfsp_ut2mt(get_seconds()) +#define hfsp_now2mt() __hfsp_ut2mt(ktime_get_real_seconds())
#endif diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index d131c8ea7eb6..94bd83b36644 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -504,9 +504,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) hfsplus_get_perms(inode, &folder->permissions, 1); set_nlink(inode, 1); inode->i_size = 2 + be32_to_cpu(folder->valence);
inode->i_atime = timespec_to_timespec64(hfsp_mt2ut(folder->access_date));inode->i_mtime = timespec_to_timespec64(hfsp_mt2ut(folder->content_mod_date));inode->i_ctime = timespec_to_timespec64(hfsp_mt2ut(folder->attribute_mod_date));
inode->i_atime = hfsp_mt2ut(folder->access_date);inode->i_mtime = hfsp_mt2ut(folder->content_mod_date);inode->i_ctime = hfsp_mt2ut(folder->attribute_mod_date);@@ -542,9 +542,9 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) init_special_inode(inode, inode->i_mode, be32_to_cpu(file->permissions.dev)); }
inode->i_atime = timespec_to_timespec64(hfsp_mt2ut(file->access_date));inode->i_mtime = timespec_to_timespec64(hfsp_mt2ut(file->content_mod_date));inode->i_ctime = timespec_to_timespec64(hfsp_mt2ut(file->attribute_mod_date));
inode->i_atime = hfsp_mt2ut(file->access_date);inode->i_mtime = hfsp_mt2ut(file->content_mod_date);inode->i_ctime = hfsp_mt2ut(file->attribute_mod_date);— 2.20.0
The patch looks pretty clean and good.
Thanks, Viacheslav Dubeyko.
On Wed, Nov 13, 2019 at 7:00 AM Viacheslav Dubeyko slava@dubeyko.com wrote:
On Nov 9, 2019, at 12:32 AM, Arnd Bergmann arnd@arndb.de wrote:
- There are two time systems. Both are based on seconds since
- a particular time/date.
- Unix: unsigned lil-endian since 00:00 GMT, Jan. 1, 1970
- Unix: signed little-endian since 00:00 GMT, Jan. 1, 1970
- mac: unsigned big-endian since 00:00 GMT, Jan. 1, 1904
- HFS implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
*/ -#define __hfs_u_to_mtime(sec) cpu_to_be32(sec + 2082844800U - sys_tz.tz_minuteswest * 60) -#define __hfs_m_to_utime(sec) (be32_to_cpu(sec) - 2082844800U + sys_tz.tz_minuteswest * 60)
I believe it makes sense to introduce some constant instead of hardcoded value (2082844800U and 60). It will be easier to understand the code without necessity to take a look into the comments. What do you think?
Every other user of sys_tz.tz_minuteswest uses a plain '60', I think that one is easy enough to understand from context. Naming the other constant is a good idea, I've now folded the change below into my patch.
Thanks for the review!
Arnd
8<----- diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index 26733051ee50..f71c384064c8 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -247,22 +247,24 @@ extern void hfs_mark_mdb_dirty(struct super_block *sb); * * HFS implementations are highly inconsistent, this one matches the * traditional behavior of 64-bit Linux, giving the most useful * time range between 1970 and 2106, by treating any on-disk timestamp - * under 2082844800U (Jan 1 1970) as a time between 2040 and 2106. + * under HFS_UTC_OFFSET (Jan 1 1970) as a time between 2040 and 2106. */ +#define HFS_UTC_OFFSET 2082844800U + static inline time64_t __hfs_m_to_utime(__be32 mt) { - time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U); + time64_t ut = (u32)(be32_to_cpu(mt) - HFS_UTC_OFFSET);
return ut + sys_tz.tz_minuteswest * 60; }
static inline __be32 __hfs_u_to_mtime(time64_t ut) { ut -= sys_tz.tz_minuteswest * 60;
- return cpu_to_be32(lower_32_bits(ut) + 2082844800U); + return cpu_to_be32(lower_32_bits(ut) + HFS_UTC_OFFSET); } #define HFS_I(inode) (container_of(inode, struct hfs_inode_info, vfs_inode)) #define HFS_SB(sb) ((struct hfs_sb_info *)(sb)->s_fs_info)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index 22d0a22c41a3..3b03fff68543 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -538,20 +538,22 @@ int hfsplus_read_wrapper(struct super_block *sb); * * HFS+ implementations are highly inconsistent, this one matches the * traditional behavior of 64-bit Linux, giving the most useful * time range between 1970 and 2106, by treating any on-disk timestamp - * under 2082844800U (Jan 1 1970) as a time between 2040 and 2106. + * under HFSPLUS_UTC_OFFSET (Jan 1 1970) as a time between 2040 and 2106. */ +#define HFSPLUS_UTC_OFFSET 2082844800U + static inline time64_t __hfsp_mt2ut(__be32 mt) { - time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U); + time64_t ut = (u32)(be32_to_cpu(mt) - HFSPLUS_UTC_OFFSET);
return ut; }
static inline __be32 __hfsp_ut2mt(time64_t ut) { - return cpu_to_be32(lower_32_bits(ut) + 2082844800U); + return cpu_to_be32(lower_32_bits(ut) + HFSPLUS_UTC_OFFSET); }
/* compatibility */ #define hfsp_mt2ut(t) (struct timespec64){ .tv_sec = __hfsp_mt2ut(t) }
On Nov 13, 2019, at 11:06 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wed, Nov 13, 2019 at 7:00 AM Viacheslav Dubeyko slava@dubeyko.com wrote:
On Nov 9, 2019, at 12:32 AM, Arnd Bergmann arnd@arndb.de wrote:
- There are two time systems. Both are based on seconds since
- a particular time/date.
- Unix: unsigned lil-endian since 00:00 GMT, Jan. 1, 1970
- Unix: signed little-endian since 00:00 GMT, Jan. 1, 1970
- mac: unsigned big-endian since 00:00 GMT, Jan. 1, 1904
- HFS implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
*/ -#define __hfs_u_to_mtime(sec) cpu_to_be32(sec + 2082844800U - sys_tz.tz_minuteswest * 60) -#define __hfs_m_to_utime(sec) (be32_to_cpu(sec) - 2082844800U + sys_tz.tz_minuteswest * 60)
I believe it makes sense to introduce some constant instead of hardcoded value (2082844800U and 60). It will be easier to understand the code without necessity to take a look into the comments. What do you think?
Every other user of sys_tz.tz_minuteswest uses a plain '60', I think that one is easy enough to understand from context. Naming the other constant is a good idea, I've now folded the change below into my patch.
Thanks for the review!
Arnd8<----- diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h index 26733051ee50..f71c384064c8 100644 --- a/fs/hfs/hfs_fs.h +++ b/fs/hfs/hfs_fs.h @@ -247,22 +247,24 @@ extern void hfs_mark_mdb_dirty(struct super_block *sb);
- HFS implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
- under HFS_UTC_OFFSET (Jan 1 1970) as a time between 2040 and 2106.
*/ +#define HFS_UTC_OFFSET 2082844800U
static inline time64_t __hfs_m_to_utime(__be32 mt) {
time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U);
time64_t ut = (u32)(be32_to_cpu(mt) - HFS_UTC_OFFSET); return ut + sys_tz.tz_minuteswest * 60;}
static inline __be32 __hfs_u_to_mtime(time64_t ut) { ut -= sys_tz.tz_minuteswest * 60;
return cpu_to_be32(lower_32_bits(ut) + 2082844800U);
return cpu_to_be32(lower_32_bits(ut) + HFS_UTC_OFFSET);} #define HFS_I(inode) (container_of(inode, struct hfs_inode_info, vfs_inode)) #define HFS_SB(sb) ((struct hfs_sb_info *)(sb)->s_fs_info)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index 22d0a22c41a3..3b03fff68543 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -538,20 +538,22 @@ int hfsplus_read_wrapper(struct super_block *sb);
- HFS+ implementations are highly inconsistent, this one matches the
- traditional behavior of 64-bit Linux, giving the most useful
- time range between 1970 and 2106, by treating any on-disk timestamp
- under 2082844800U (Jan 1 1970) as a time between 2040 and 2106.
- under HFSPLUS_UTC_OFFSET (Jan 1 1970) as a time between 2040 and 2106.
*/ +#define HFSPLUS_UTC_OFFSET 2082844800U
static inline time64_t __hfsp_mt2ut(__be32 mt) {
time64_t ut = (u32)(be32_to_cpu(mt) - 2082844800U);
time64_t ut = (u32)(be32_to_cpu(mt) - HFSPLUS_UTC_OFFSET); return ut;}
static inline __be32 __hfsp_ut2mt(time64_t ut) {
return cpu_to_be32(lower_32_bits(ut) + 2082844800U);
return cpu_to_be32(lower_32_bits(ut) + HFSPLUS_UTC_OFFSET);}
/* compatibility */ #define hfsp_mt2ut(t) (struct timespec64){ .tv_sec = __hfsp_mt2ut(t) }
Looks good for me. I like the patch.
Reviewed-by: Vyacheslav Dubeyko slava@dubeyko.com
Thanks, Vyacheslav Dubeyko.
The timespec structure and associated interfaces are deprecated and will be removed in the future because of the y2038 overflow.
The use of ktime_to_timespec() in timeout_to_jiffies() does not suffer from that overflow, but is easy to avoid by just converting the ktime_t into jiffies directly.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/msm/msm_drv.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 71547e756e29..740bf7c70d8f 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -454,8 +454,7 @@ static inline unsigned long timeout_to_jiffies(const ktime_t *timeout) remaining_jiffies = 0; } else { ktime_t rem = ktime_sub(*timeout, now); - struct timespec ts = ktime_to_timespec(rem); - remaining_jiffies = timespec_to_jiffies(&ts); + remaining_jiffies = ktime_divns(rem, NSEC_PER_SEC / HZ); }
return remaining_jiffies;
On Fri, Nov 08, 2019 at 10:32:52PM +0100, Arnd Bergmann wrote:
The timespec structure and associated interfaces are deprecated and will be removed in the future because of the y2038 overflow.
The use of ktime_to_timespec() in timeout_to_jiffies() does not suffer from that overflow, but is easy to avoid by just converting the ktime_t into jiffies directly.
This seems good to me. y2038 changes are the best changes.
Reviewed-by: Jordan Crouse jcrouse@codeaurora.org
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/msm/msm_drv.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 71547e756e29..740bf7c70d8f 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -454,8 +454,7 @@ static inline unsigned long timeout_to_jiffies(const ktime_t *timeout) remaining_jiffies = 0; } else { ktime_t rem = ktime_sub(*timeout, now);
struct timespec ts = ktime_to_timespec(rem);remaining_jiffies = timespec_to_jiffies(&ts);
remaining_jiffies = ktime_divns(rem, NSEC_PER_SEC / HZ);return remaining_jiffies; -- 2.20.0
struct timespec is being removed from the kernel because it often leads to code that is not y2038-safe.
In the etnaviv driver, monotonic timestamps are used, which do not suffer from overflow, but using ktime_t still leads to better code overall.
The conversion is straightforward for the most part, except for etnaviv_timeout_to_jiffies(), which needs to handle arguments larger than MAX_JIFFY_OFFSET on 32-bit architectures.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 19 +++++++++---------- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 21 +++++++++------------ drivers/gpu/drm/etnaviv/etnaviv_gem.c | 5 ++--- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4 ++-- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 4 ++-- 6 files changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1f9c01be40d7..1250c5e06329 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -282,16 +282,13 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data, args->flags, &args->handle); }
-#define TS(t) ((struct timespec){ \ - .tv_sec = (t).tv_sec, \ - .tv_nsec = (t).tv_nsec \ -}) - static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, struct drm_file *file) { struct drm_etnaviv_gem_cpu_prep *args = data; struct drm_gem_object *obj; + ktime_t timeout = ktime_set(args->timeout.tv_sec, + args->timeout.tv_nsec); int ret;
if (args->op & ~(ETNA_PREP_READ | ETNA_PREP_WRITE | ETNA_PREP_NOSYNC)) @@ -301,7 +298,7 @@ static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, if (!obj) return -ENOENT;
- ret = etnaviv_gem_cpu_prep(obj, args->op, &TS(args->timeout)); + ret = etnaviv_gem_cpu_prep(obj, args->op, timeout);
drm_gem_object_put_unlocked(obj);
@@ -354,7 +351,8 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, { struct drm_etnaviv_wait_fence *args = data; struct etnaviv_drm_private *priv = dev->dev_private; - struct timespec *timeout = &TS(args->timeout); + ktime_t timeout = ktime_set(args->timeout.tv_sec, + args->timeout.tv_nsec); struct etnaviv_gpu *gpu;
if (args->flags & ~(ETNA_WAIT_NONBLOCK)) @@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, return -ENXIO;
if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = NULL; + timeout = ktime_set(0, 0);
return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, timeout); @@ -403,7 +401,8 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, { struct etnaviv_drm_private *priv = dev->dev_private; struct drm_etnaviv_gem_wait *args = data; - struct timespec *timeout = &TS(args->timeout); + ktime_t timeout = ktime_set(args->timeout.tv_sec, + args->timeout.tv_nsec); struct drm_gem_object *obj; struct etnaviv_gpu *gpu; int ret; @@ -423,7 +422,7 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, return -ENOENT;
if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = NULL; + timeout = ktime_set(0, 0);
ret = etnaviv_gem_wait_bo(gpu, obj, timeout);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h index 32cfa5a48d42..57a4e247bbcf 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h @@ -60,8 +60,7 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev, int etnaviv_gem_prime_pin(struct drm_gem_object *obj); void etnaviv_gem_prime_unpin(struct drm_gem_object *obj); void *etnaviv_gem_vmap(struct drm_gem_object *obj); -int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, - struct timespec *timeout); +int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, ktime_t timeout); int etnaviv_gem_cpu_fini(struct drm_gem_object *obj); void etnaviv_gem_free_object(struct drm_gem_object *obj); int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file, @@ -106,22 +105,20 @@ static inline size_t size_vstruct(size_t nelem, size_t elem_size, size_t base) * We need to calculate the timeout in terms of number of jiffies * between the specified timeout and the current CLOCK_MONOTONIC time. */ -static inline unsigned long etnaviv_timeout_to_jiffies( - const struct timespec *timeout) +static inline unsigned long etnaviv_timeout_to_jiffies(ktime_t timeout) { - struct timespec64 ts, to; - - to = timespec_to_timespec64(*timeout); - - ktime_get_ts64(&ts); + s64 remain = ktime_to_ns(ktime_sub(timeout, ktime_get()));
/* timeouts before "now" have already expired */ - if (timespec64_compare(&to, &ts) <= 0) + if (remain < 0) return 0;
- ts = timespec64_sub(to, ts); +#ifndef CONFIG_64BIT + if (remain > ((s64)MAX_JIFFY_OFFSET * NSEC_PER_SEC / HZ)) + return MAX_JIFFY_OFFSET; +#endif
- return timespec64_to_jiffies(&ts); + return nsecs_to_jiffies(remain); }
#endif /* __ETNAVIV_DRV_H__ */ diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index cb1faaac380a..febe5196788e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -372,8 +372,7 @@ static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op) return DMA_BIDIRECTIONAL; }
-int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, - struct timespec *timeout) +int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, ktime_t timeout) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); struct drm_device *dev = obj->dev; @@ -431,7 +430,7 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj) }
int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj, - struct timespec *timeout) + ktime_t timeout) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index d6270acce619..a3461a554a6c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -112,7 +112,7 @@ struct etnaviv_gem_submit { void etnaviv_submit_put(struct etnaviv_gem_submit * submit);
int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj, - struct timespec *timeout); + ktime_t timeout); int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags, const struct etnaviv_gem_ops *ops, struct etnaviv_gem_object **res); void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index d47d1a8e0219..e42b1c4d902c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1132,7 +1132,7 @@ static void event_free(struct etnaviv_gpu *gpu, unsigned int event) * Cmdstream submission/retirement: */ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, - u32 id, struct timespec *timeout) + u32 id, ktime_t timeout) { struct dma_fence *fence; int ret; @@ -1179,7 +1179,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, * that lock in this function while waiting. */ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu, - struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout) + struct etnaviv_gem_object *etnaviv_obj, ktime_t timeout) { unsigned long remaining; long ret; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h index 8f9bd4edc96a..6d352a435427 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h @@ -169,9 +169,9 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m); void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu); void etnaviv_gpu_retire(struct etnaviv_gpu *gpu); int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, - u32 fence, struct timespec *timeout); + u32 fence, ktime_t timeout); int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu, - struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout); + struct etnaviv_gem_object *etnaviv_obj, ktime_t timeout); struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit); int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu); void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
Am Freitag, den 08.11.2019, 22:32 +0100 schrieb Arnd Bergmann:
struct timespec is being removed from the kernel because it often leads to code that is not y2038-safe.
In the etnaviv driver, monotonic timestamps are used, which do not suffer from overflow, but using ktime_t still leads to better code overall.
The conversion is straightforward for the most part, except for etnaviv_timeout_to_jiffies(), which needs to handle arguments larger than MAX_JIFFY_OFFSET on 32-bit architectures.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 19 +++++++++---------- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 21 +++++++++------------ drivers/gpu/drm/etnaviv/etnaviv_gem.c | 5 ++--- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4 ++-- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 4 ++-- 6 files changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1f9c01be40d7..1250c5e06329 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -282,16 +282,13 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data, args->flags, &args->handle); } -#define TS(t) ((struct timespec){ \
- .tv_sec = (t).tv_sec, \
- .tv_nsec = (t).tv_nsec \
-})
static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, struct drm_file *file) { struct drm_etnaviv_gem_cpu_prep *args = data; struct drm_gem_object *obj;
- ktime_t timeout = ktime_set(args->timeout.tv_sec,
args->timeout.tv_nsec);if (args->op & ~(ETNA_PREP_READ | ETNA_PREP_WRITE | ETNA_PREP_NOSYNC)) @@ -301,7 +298,7 @@ static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, if (!obj) return -ENOENT;
- ret = etnaviv_gem_cpu_prep(obj, args->op, &TS(args->timeout));
- ret = etnaviv_gem_cpu_prep(obj, args->op, timeout);
drm_gem_object_put_unlocked(obj); @@ -354,7 +351,8 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, { struct drm_etnaviv_wait_fence *args = data; struct etnaviv_drm_private *priv = dev->dev_private;
- struct timespec *timeout = &TS(args->timeout);
- ktime_t timeout = ktime_set(args->timeout.tv_sec,
args->timeout.tv_nsec);if (args->flags & ~(ETNA_WAIT_NONBLOCK)) @@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, return -ENXIO; if (args->flags & ETNA_WAIT_NONBLOCK)
timeout = NULL;
timeout = ktime_set(0, 0);
This is a change in behavior, as far as I can see. After this change the called internal function is not able to differentiate between a NONBLOCK call and a blocking call with 0 timeout. The difference being that on a busy object the NONBLOCK call will return -EBUSY, while a blocking call will return -ETIMEDOUT.
But then CLOCK_MONOTONIC starts at 0 and should not never wrap, right? If that's the case then we should never encounter a genuine 0 timeout and this change would be okay.
Regards, Lucas
return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, timeout); @@ -403,7 +401,8 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, { struct etnaviv_drm_private *priv = dev->dev_private; struct drm_etnaviv_gem_wait *args = data;
- struct timespec *timeout = &TS(args->timeout);
- ktime_t timeout = ktime_set(args->timeout.tv_sec,
args->timeout.tv_nsec);@@ -423,7 +422,7 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, return -ENOENT; if (args->flags & ETNA_WAIT_NONBLOCK)
timeout = NULL;
timeout = ktime_set(0, 0);ret = etnaviv_gem_wait_bo(gpu, obj, timeout); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h index 32cfa5a48d42..57a4e247bbcf 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h @@ -60,8 +60,7 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev, int etnaviv_gem_prime_pin(struct drm_gem_object *obj); void etnaviv_gem_prime_unpin(struct drm_gem_object *obj); void *etnaviv_gem_vmap(struct drm_gem_object *obj); -int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op,
struct timespec *timeout);+int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, ktime_t timeout); int etnaviv_gem_cpu_fini(struct drm_gem_object *obj); void etnaviv_gem_free_object(struct drm_gem_object *obj); int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file, @@ -106,22 +105,20 @@ static inline size_t size_vstruct(size_t nelem, size_t elem_size, size_t base)
- We need to calculate the timeout in terms of number of jiffies
- between the specified timeout and the current CLOCK_MONOTONIC time.
*/ -static inline unsigned long etnaviv_timeout_to_jiffies(
- const struct timespec *timeout)
+static inline unsigned long etnaviv_timeout_to_jiffies(ktime_t timeout) {
- struct timespec64 ts, to;
- to = timespec_to_timespec64(*timeout);
- ktime_get_ts64(&ts);
- s64 remain = ktime_to_ns(ktime_sub(timeout, ktime_get()));
/* timeouts before "now" have already expired */
- if (timespec64_compare(&to, &ts) <= 0)
- if (remain < 0) return 0;
- ts = timespec64_sub(to, ts);
+#ifndef CONFIG_64BIT
- if (remain > ((s64)MAX_JIFFY_OFFSET * NSEC_PER_SEC / HZ))
return MAX_JIFFY_OFFSET;+#endif
- return timespec64_to_jiffies(&ts);
- return nsecs_to_jiffies(remain);
} #endif /* __ETNAVIV_DRV_H__ */ diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index cb1faaac380a..febe5196788e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -372,8 +372,7 @@ static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op) return DMA_BIDIRECTIONAL; } -int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op,
struct timespec *timeout)+int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, ktime_t timeout) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); struct drm_device *dev = obj->dev; @@ -431,7 +430,7 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj) } int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
- struct timespec *timeout)
ktime_t timeout){ struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index d6270acce619..a3461a554a6c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -112,7 +112,7 @@ struct etnaviv_gem_submit { void etnaviv_submit_put(struct etnaviv_gem_submit * submit); int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
- struct timespec *timeout);
- ktime_t timeout);
int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags, const struct etnaviv_gem_ops *ops, struct etnaviv_gem_object **res); void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index d47d1a8e0219..e42b1c4d902c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1132,7 +1132,7 @@ static void event_free(struct etnaviv_gpu *gpu, unsigned int event)
- Cmdstream submission/retirement:
*/ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
- u32 id, struct timespec *timeout)
- u32 id, ktime_t timeout)
{ struct dma_fence *fence; int ret; @@ -1179,7 +1179,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
- that lock in this function while waiting.
*/ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
- struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout)
- struct etnaviv_gem_object *etnaviv_obj, ktime_t timeout)
{ unsigned long remaining; long ret; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h index 8f9bd4edc96a..6d352a435427 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h @@ -169,9 +169,9 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m); void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu); void etnaviv_gpu_retire(struct etnaviv_gpu *gpu); int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
- u32 fence, struct timespec *timeout);
- u32 fence, ktime_t timeout);
int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
- struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout);
- struct etnaviv_gem_object *etnaviv_obj, ktime_t timeout);
struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit); int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu); void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
On Sat, Nov 9, 2019 at 12:03 AM Lucas Stach l.stach@pengutronix.de wrote:
Am Freitag, den 08.11.2019, 22:32 +0100 schrieb Arnd Bergmann:
struct timespec is being removed from the kernel because it often leads to code that is not y2038-safe.
In the etnaviv driver, monotonic timestamps are used, which do not suffer from overflow, but using ktime_t still leads to better code overall.
The conversion is straightforward for the most part, except for etnaviv_timeout_to_jiffies(), which needs to handle arguments larger than MAX_JIFFY_OFFSET on 32-bit architectures.
Signed-off-by: Arnd Bergmann arnd@arndb.de
@@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, return -ENXIO;
if (args->flags & ETNA_WAIT_NONBLOCK)
timeout = NULL;
timeout = ktime_set(0, 0);This is a change in behavior, as far as I can see. After this change the called internal function is not able to differentiate between a NONBLOCK call and a blocking call with 0 timeout. The difference being that on a busy object the NONBLOCK call will return -EBUSY, while a blocking call will return -ETIMEDOUT.
Ah, good point. I created this patch a long time ago (cherry-picked it out of an older branch I had done), so I don't remember how I concluded this was a safe conversion, of if I missed that difference originally.
But then CLOCK_MONOTONIC starts at 0 and should not never wrap, right?
Yes, that is correct.
If that's the case then we should never encounter a genuine 0 timeout and this change would be okay.
That's quite likely, I'd say any program passing {0,0} as a timeout without ETNA_WAIT_NONBLOCK is already broken, but if we leave it like that, it would be best to describe the reasoning in the changelog.
Should I change the changelog, or change the patch to restore the current behavior instead?
I guess I could fold the change below into my patch to make it transparent to the application again.
Arnd
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1250c5e06329..162cedfb7f72 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -354,6 +354,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, ktime_t timeout = ktime_set(args->timeout.tv_sec, args->timeout.tv_nsec); struct etnaviv_gpu *gpu; + int ret;
if (args->flags & ~(ETNA_WAIT_NONBLOCK)) return -EINVAL; @@ -365,8 +366,12 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, if (!gpu) return -ENXIO;
- if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = ktime_set(0, 0); + if (args->flags & ETNA_WAIT_NONBLOCK) { + ret = etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, + ktime_set(0, 0)); + + return (ret == -ETIMEDOUT) ? -EBUSY : ret; + }
return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, timeout); @@ -421,10 +426,13 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, if (!obj) return -ENOENT;
- if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = ktime_set(0, 0); - - ret = etnaviv_gem_wait_bo(gpu, obj, timeout); + if (args->flags & ETNA_WAIT_NONBLOCK) { + ret = etnaviv_gem_wait_bo(gpu, obj, ktime_set(0, 0)); + if (ret == -ETIMEDOUT) + ret = -EBUSY; + } else { + ret = etnaviv_gem_wait_bo(gpu, obj, timeout); + }
drm_gem_object_put_unlocked(obj);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index e42b1c4d902c..fa6986c5a5fe 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1135,6 +1135,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, u32 id, ktime_t timeout) { struct dma_fence *fence; + unsigned long remaining; int ret;
/* @@ -1151,12 +1152,12 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, if (!fence) return 0;
- if (!timeout) { - /* No timeout was requested: just test for completion */ - ret = dma_fence_is_signaled(fence) ? 0 : -EBUSY; + if (!timeout || + (remaining = etnaviv_timeout_to_jiffies(timeout)) == 0) { + /* No timeout was requested, or timeout is already expired, + * just test for completion */ + ret = dma_fence_is_signaled(fence) ? 0 : -ETIMEDOUT; } else { - unsigned long remaining = etnaviv_timeout_to_jiffies(timeout); - ret = dma_fence_wait_timeout(fence, true, remaining); if (ret == 0) ret = -ETIMEDOUT; @@ -1185,7 +1186,7 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu, long ret;
if (!timeout) - return !is_active(etnaviv_obj) ? 0 : -EBUSY; + return !is_active(etnaviv_obj) ? 0 : -ETIMEDOUT;
remaining = etnaviv_timeout_to_jiffies(timeout);
On Sa, 2019-11-09 at 13:12 +0100, Arnd Bergmann wrote:
On Sat, Nov 9, 2019 at 12:03 AM Lucas Stach l.stach@pengutronix.de wrote:
Am Freitag, den 08.11.2019, 22:32 +0100 schrieb Arnd Bergmann:
struct timespec is being removed from the kernel because it often leads to code that is not y2038-safe.
In the etnaviv driver, monotonic timestamps are used, which do not suffer from overflow, but using ktime_t still leads to better code overall.
The conversion is straightforward for the most part, except for etnaviv_timeout_to_jiffies(), which needs to handle arguments larger than MAX_JIFFY_OFFSET on 32-bit architectures.
Signed-off-by: Arnd Bergmann arnd@arndb.de @@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, return -ENXIO;
if (args->flags & ETNA_WAIT_NONBLOCK)
timeout = NULL;
timeout = ktime_set(0, 0);This is a change in behavior, as far as I can see. After this change the called internal function is not able to differentiate between a NONBLOCK call and a blocking call with 0 timeout. The difference being that on a busy object the NONBLOCK call will return -EBUSY, while a blocking call will return -ETIMEDOUT.
Ah, good point. I created this patch a long time ago (cherry-picked it out of an older branch I had done), so I don't remember how I concluded this was a safe conversion, of if I missed that difference originally.
But then CLOCK_MONOTONIC starts at 0 and should not never wrap, right?
Yes, that is correct.
If that's the case then we should never encounter a genuine 0 timeout and this change would be okay.
That's quite likely, I'd say any program passing {0,0} as a timeout without ETNA_WAIT_NONBLOCK is already broken, but if we leave it like that, it would be best to describe the reasoning in the changelog.
Should I change the changelog, or change the patch to restore the current behavior instead?
I guess I could fold the change below into my patch to make it transparent to the application again.
If we assume 0 to never be a valid timeout, due to monotonic clock starting at 0 and never wrapping then I think we shouldn't introduce any additional code complexity to fix up the return value for this specific case. I'm not aware of any etnaviv userspace being broken in this way to rely on the return value for an invalid timeout input.
Please just amend the commit message to mention the change in behavior and why we think it is safe to do.
Regards, Lucas
Arnddiff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1250c5e06329..162cedfb7f72 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -354,6 +354,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, ktime_t timeout = ktime_set(args->timeout.tv_sec, args->timeout.tv_nsec); struct etnaviv_gpu *gpu;
int ret; if (args->flags & ~(ETNA_WAIT_NONBLOCK)) return -EINVAL;@@ -365,8 +366,12 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, if (!gpu) return -ENXIO;
if (args->flags & ETNA_WAIT_NONBLOCK)timeout = ktime_set(0, 0);
if (args->flags & ETNA_WAIT_NONBLOCK) {ret = etnaviv_gpu_wait_fence_interruptible(gpu, args->fence,ktime_set(0, 0));return (ret == -ETIMEDOUT) ? -EBUSY : ret;} return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, timeout);@@ -421,10 +426,13 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, if (!obj) return -ENOENT;
if (args->flags & ETNA_WAIT_NONBLOCK)timeout = ktime_set(0, 0);ret = etnaviv_gem_wait_bo(gpu, obj, timeout);
if (args->flags & ETNA_WAIT_NONBLOCK) {ret = etnaviv_gem_wait_bo(gpu, obj, ktime_set(0, 0));if (ret == -ETIMEDOUT)ret = -EBUSY;} else {ret = etnaviv_gem_wait_bo(gpu, obj, timeout);} drm_gem_object_put_unlocked(obj);diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index e42b1c4d902c..fa6986c5a5fe 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1135,6 +1135,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, u32 id, ktime_t timeout) { struct dma_fence *fence;
unsigned long remaining; int ret; /*@@ -1151,12 +1152,12 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, if (!fence) return 0;
if (!timeout) {/* No timeout was requested: just test for completion */ret = dma_fence_is_signaled(fence) ? 0 : -EBUSY;
if (!timeout ||(remaining = etnaviv_timeout_to_jiffies(timeout)) == 0) {/* No timeout was requested, or timeout is already expired,* just test for completion */ret = dma_fence_is_signaled(fence) ? 0 : -ETIMEDOUT; } else {
unsigned long remaining = etnaviv_timeout_to_jiffies(timeout);ret = dma_fence_wait_timeout(fence, true, remaining); if (ret == 0) ret = -ETIMEDOUT;@@ -1185,7 +1186,7 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu, long ret;
if (!timeout)
return !is_active(etnaviv_obj) ? 0 : -EBUSY;
return !is_active(etnaviv_obj) ? 0 : -ETIMEDOUT; remaining = etnaviv_timeout_to_jiffies(timeout);
On Mon, Nov 11, 2019 at 10:55 AM Lucas Stach l.stach@pengutronix.de wrote:
If that's the case then we should never encounter a genuine 0 timeout and this change would be okay.
That's quite likely, I'd say any program passing {0,0} as a timeout without ETNA_WAIT_NONBLOCK is already broken, but if we leave it like that, it would be best to describe the reasoning in the changelog.
Should I change the changelog, or change the patch to restore the current behavior instead?
I guess I could fold the change below into my patch to make it transparent to the application again.
If we assume 0 to never be a valid timeout, due to monotonic clock starting at 0 and never wrapping then I think we shouldn't introduce any additional code complexity to fix up the return value for this specific case. I'm not aware of any etnaviv userspace being broken in this way to rely on the return value for an invalid timeout input.
Please just amend the commit message to mention the change in behavior and why we think it is safe to do.
Russell had some additional concerns that he raised on IRC, and I did a new simpler implementation of the patch, plus a related bugfix.
Please have a look at those.
Arnd
The ohci driver uses the get_seconds() function to implement the 32-bit CSR_BUS_TIME register. This was added in 2010 commit a48777e03ad5 ("firewire: add CSR BUS_TIME support").
As get_seconds() returns a 32-bit value (on 32-bit architectures), it seems like a good fit for that register, but it is also deprecated because of the y2038/y2106 overflow problem, and should be replaced throughout the kernel with either ktime_get_real_seconds() or ktime_get_seconds().
I'm using the latter here, which uses monotonic time. This has the advantage of behaving better during concurrent settimeofday() updates or leap second adjustments and won't overflow a 32-bit integer, but the downside of using CLOCK_MONOTONIC instead of CLOCK_REALTIME is that the observed values are not related to external clocks.
If we instead need UTC but can live with clock jumps or overflows, then we should use ktime_get_real_seconds() instead, retaining the existing behavior.
Reviewed-by: Clemens Ladisch clemens@ladisch.de Cc: Stefan Richter stefanr@s5r6.in-berlin.de Link: https://lore.kernel.org/lkml/20180711124923.1205200-1-arnd@arndb.de/ Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/firewire/ohci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 522f3addb5bd..33269316f111 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1752,7 +1752,7 @@ static u32 update_bus_time(struct fw_ohci *ohci)
if (unlikely(!ohci->bus_time_running)) { reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_cycle64Seconds); - ohci->bus_time = (lower_32_bits(get_seconds()) & ~0x7f) | + ohci->bus_time = (lower_32_bits(ktime_get_seconds()) & ~0x7f) | (cycle_time_seconds & 0x40); ohci->bus_time_running = true; }
On Nov 08 Arnd Bergmann wrote:
The ohci driver uses the get_seconds() function to implement the 32-bit CSR_BUS_TIME register. This was added in 2010 commit a48777e03ad5 ("firewire: add CSR BUS_TIME support").
As get_seconds() returns a 32-bit value (on 32-bit architectures), it seems like a good fit for that register, but it is also deprecated because of the y2038/y2106 overflow problem, and should be replaced throughout the kernel with either ktime_get_real_seconds() or ktime_get_seconds().
I'm using the latter here, which uses monotonic time. This has the advantage of behaving better during concurrent settimeofday() updates or leap second adjustments and won't overflow a 32-bit integer, but the downside of using CLOCK_MONOTONIC instead of CLOCK_REALTIME is that the observed values are not related to external clocks.
If we instead need UTC but can live with clock jumps or overflows, then we should use ktime_get_real_seconds() instead, retaining the existing behavior.
Reviewed-by: Clemens Ladisch clemens@ladisch.de Cc: Stefan Richter stefanr@s5r6.in-berlin.de Link: https://lore.kernel.org/lkml/20180711124923.1205200-1-arnd@arndb.de/ Signed-off-by: Arnd Bergmann arnd@arndb.de
Committed to linux1394.git:for-next. Thank you for the patch and your extraordinary patience.
drivers/firewire/ohci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 522f3addb5bd..33269316f111 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1752,7 +1752,7 @@ static u32 update_bus_time(struct fw_ohci *ohci) if (unlikely(!ohci->bus_time_running)) { reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_cycle64Seconds);
ohci->bus_time = (lower_32_bits(get_seconds()) & ~0x7f) |
ohci->bus_time = (lower_32_bits(ktime_get_seconds()) & ~0x7f) | (cycle_time_seconds & 0x40);
-
Arnd Bergmann -
Ben Hutchings -
Deepa Dinamani -
Ernesto A. Fernández -
Jordan Crouse -
Lucas Stach -
Max Filippov -
Pablo Neira Ayuso -
Phil Sutter -
Stefan Richter -
Viacheslav Dubeyko