I've gone through the remaining uses of time_t etc and come up with a set of 90 patches of varying complexity and importance, to the point of being able to remove the old time_t/timeval/timespec from the kernel headers completely.
This set includes the eight patches that I think should be merged right away and backported into stable kernels if possible.
Please apply individual patches to the respective maintainer trees for either v5.4 or v5.5 as appropriate.
For reference, the full series of 90 patches can be found at https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y...
Arnd
Arnd Bergmann (8): y2038: timex: remove incorrect time_t truncation timekeeping: optimize ns_to_timespec64 powerpc: fix vdso32 for ppc64le ipmi: kill off 'timespec' usage again netfilter: xt_time: use time64_t lp: fix sparc64 LPSETTIMEOUT ioctl ppdev: fix PPGETTIME/PPSETTIME ioctls Input: input_event: fix struct padding on sparc64
arch/powerpc/kernel/vdso32/gettimeofday.S | 2 +- drivers/char/ipmi/ipmi_si_intf.c | 40 ++++++++--------------- drivers/char/lp.c | 4 +++ drivers/char/ppdev.c | 16 ++++++--- drivers/input/evdev.c | 3 ++ drivers/input/misc/uinput.c | 3 ++ include/uapi/linux/input.h | 1 + kernel/time/ntp.c | 2 +- kernel/time/time.c | 21 +++++++----- net/netfilter/xt_time.c | 19 ++++++----- 10 files changed, 61 insertions(+), 50 deletions(-)
Cc: Benjamin Herrenschmidt benh@kernel.crashing.org Cc: Paul Mackerras paulus@samba.org Cc: Michael Ellerman mpe@ellerman.id.au Cc: Corey Minyard minyard@acm.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Sudip Mukherjee sudipm.mukherjee@gmail.com Cc: Dmitry Torokhov dmitry.torokhov@gmail.com Cc: John Stultz john.stultz@linaro.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Stephen Boyd sboyd@kernel.org Cc: Pablo Neira Ayuso pablo@netfilter.org Cc: Jozsef Kadlecsik kadlec@netfilter.org Cc: Florian Westphal fw@strlen.de Cc: "David S. Miller" davem@davemloft.net Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org Cc: openipmi-developer@lists.sourceforge.net Cc: linux-input@vger.kernel.org Cc: netfilter-devel@vger.kernel.org Cc: coreteam@netfilter.org Cc: netdev@vger.kernel.org Cc: sparclinux@vger.kernel.org
A cast to 'time_t' was accidentally left in place during the conversion of __do_adjtimex() to 64-bit timestamps, so the resulting value is incorrectly truncated.
Remove the cast so the 64-bit time gets propagated correctly.
Cc: stable@vger.kernel.org Fixes: ead25417f82e ("timex: use __kernel_timex internally") Signed-off-by: Arnd Bergmann arnd@arndb.de --- kernel/time/ntp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index 65eb796610dc..069ca78fb0bf 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -771,7 +771,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts, /* fill PPS status fields */ pps_fill_timex(txc);
- txc->time.tv_sec = (time_t)ts->tv_sec; + txc->time.tv_sec = ts->tv_sec; txc->time.tv_usec = ts->tv_nsec; if (!(time_status & STA_NANO)) txc->time.tv_usec = ts->tv_nsec / NSEC_PER_USEC;
Thanks for fixing the bug.
Acked-by: Deepa Dinamani deepa.kernel@gmail.com
I noticed that ns_to_timespec64() calls div_s64_rem(), which is a rather slow function on 32-bit architectures, as it cannot take advantage of the do_div() optimizations for constant arguments.
This open-codes the div_s64_rem() function here, so we can pass a constant divider into the optimized div_u64_rem() function.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- kernel/time/time.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/kernel/time/time.c b/kernel/time/time.c index 5c54ca632d08..45a358953f09 100644 --- a/kernel/time/time.c +++ b/kernel/time/time.c @@ -550,18 +550,21 @@ EXPORT_SYMBOL(set_normalized_timespec64); */ struct timespec64 ns_to_timespec64(const s64 nsec) { - struct timespec64 ts; + struct timespec64 ts = { 0, 0 }; s32 rem;
- if (!nsec) - return (struct timespec64) {0, 0}; - - ts.tv_sec = div_s64_rem(nsec, NSEC_PER_SEC, &rem); - if (unlikely(rem < 0)) { - ts.tv_sec--; - rem += NSEC_PER_SEC; + if (likely(nsec > 0)) { + ts.tv_sec = div_u64_rem(nsec, NSEC_PER_SEC, &rem); + ts.tv_nsec = rem; + } else if (nsec < 0) { + /* + * With negative times, tv_sec points to the earlier + * second, and tv_nsec counts the nanoseconds since + * then, so tv_nsec is always a positive number. + */ + ts.tv_sec = -div_u64_rem(-nsec - 1, NSEC_PER_SEC, &rem) - 1; + ts.tv_nsec = NSEC_PER_SEC - rem - 1; } - ts.tv_nsec = rem;
return ts; }
The arithmetic looks right to me.
Acked-by: Deepa Dinamani deepa.kernel@gmail.com
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- arch/powerpc/kernel/vdso32/gettimeofday.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/vdso32/gettimeofday.S b/arch/powerpc/kernel/vdso32/gettimeofday.S index becd9f8767ed..4327665ad86f 100644 --- a/arch/powerpc/kernel/vdso32/gettimeofday.S +++ b/arch/powerpc/kernel/vdso32/gettimeofday.S @@ -13,7 +13,7 @@ #include <asm/unistd.h>
/* Offset for the low 32-bit part of a field of long type */ -#ifdef CONFIG_PPC64 +#if defined(CONFIG_PPC64) && defined(CONFIG_CPU_BIG_ENDIAN) #define LOPART 4 #define TSPEC_TV_SEC TSPC64_TV_SEC+LOPART #else
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
ppc64le doesn't have 32-bit compat so this is only theoretical.
Ben.
Signed-off-by: Arnd Bergmann arnd@arndb.de
arch/powerpc/kernel/vdso32/gettimeofday.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/vdso32/gettimeofday.S b/arch/powerpc/kernel/vdso32/gettimeofday.S index becd9f8767ed..4327665ad86f 100644 --- a/arch/powerpc/kernel/vdso32/gettimeofday.S +++ b/arch/powerpc/kernel/vdso32/gettimeofday.S @@ -13,7 +13,7 @@ #include <asm/unistd.h> /* Offset for the low 32-bit part of a field of long type */ -#ifdef CONFIG_PPC64 +#if defined(CONFIG_PPC64) && defined(CONFIG_CPU_BIG_ENDIAN) #define LOPART 4 #define TSPEC_TV_SEC TSPC64_TV_SEC+LOPART #else
On Wed, Nov 20, 2019 at 8:13 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
ppc64le doesn't have 32-bit compat so this is only theoretical.
That is probably true. I only looked at the kernel, which today still supports compat mode for ppc64le, but I saw the patches to disable it, and I don't think anyone has even attempted building user space for it.
Arnd
On Wed, 2019-11-20 at 20:35 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 8:13 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
ppc64le doesn't have 32-bit compat so this is only theoretical.
That is probably true. I only looked at the kernel, which today still supports compat mode for ppc64le, but I saw the patches to disable it, and I don't think anyone has even attempted building user space for it.
COMPAT is still enabled for some reason, but VDSO32 isn't (since 4.2).
Ben.
On Wed, Nov 20, 2019 at 10:49 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Wed, 2019-11-20 at 20:35 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 8:13 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
ppc64le doesn't have 32-bit compat so this is only theoretical.
That is probably true. I only looked at the kernel, which today still supports compat mode for ppc64le, but I saw the patches to disable it, and I don't think anyone has even attempted building user space for it.
COMPAT is still enabled for some reason, but VDSO32 isn't (since 4.2).
Ok, I had missed that detail. Should I just drop my patch then?
Arnd
On Thu, 2019-11-21 at 11:02 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 10:49 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Wed, 2019-11-20 at 20:35 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 8:13 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
On little-endian 32-bit application running on 64-bit kernels, the current vdso would read the wrong half of the xtime seconds field. Change it to return the lower half like it does on big-endian.
ppc64le doesn't have 32-bit compat so this is only theoretical.
That is probably true. I only looked at the kernel, which today still supports compat mode for ppc64le, but I saw the patches to disable it, and I don't think anyone has even attempted building user space for it.
COMPAT is still enabled for some reason, but VDSO32 isn't (since 4.2).
Ok, I had missed that detail. Should I just drop my patch then?
I think so.
Ben.
'struct timespec' is getting removed from the kernel. The usage in ipmi was fixed before in commit 48862ea2ce86 ("ipmi: Update timespec usage to timespec64"), but unfortunately it crept back in.
The busy looping code can better use ktime_t anyway, so use that there to simplify the implementation.
Fixes: cbb19cb1eef0 ("ipmi_si: Convert timespec64 to timespec") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/char/ipmi/ipmi_si_intf.c | 40 +++++++++++--------------------- 1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 6b9a0593d2eb..c7cc8538b84a 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -265,10 +265,10 @@ static void cleanup_ipmi_si(void); #ifdef DEBUG_TIMING void debug_timestamp(char *msg) { - struct timespec t; + struct timespec64 t;
- ktime_get_ts(&t); - pr_debug("**%s: %ld.%9.9ld\n", msg, (long) t.tv_sec, t.tv_nsec); + ktime_get_ts64(&t); + pr_debug("**%s: %lld.%9.9ld\n", msg, t.tv_sec, t.tv_nsec); } #else #define debug_timestamp(x) @@ -935,38 +935,25 @@ static void set_run_to_completion(void *send_info, bool i_run_to_completion) }
/* - * Use -1 in the nsec value of the busy waiting timespec to tell that - * we are spinning in kipmid looking for something and not delaying - * between checks + * Use -1 as a special constant to tell that we are spinning in kipmid + * looking for something and not delaying between checks */ -static inline void ipmi_si_set_not_busy(struct timespec *ts) -{ - ts->tv_nsec = -1; -} -static inline int ipmi_si_is_busy(struct timespec *ts) -{ - return ts->tv_nsec != -1; -} - +#define IPMI_TIME_NOT_BUSY ns_to_ktime(-1ull) static inline bool ipmi_thread_busy_wait(enum si_sm_result smi_result, const struct smi_info *smi_info, - struct timespec *busy_until) + ktime_t *busy_until) { unsigned int max_busy_us = 0;
if (smi_info->si_num < num_max_busy_us) max_busy_us = kipmid_max_busy_us[smi_info->si_num]; if (max_busy_us == 0 || smi_result != SI_SM_CALL_WITH_DELAY) - ipmi_si_set_not_busy(busy_until); - else if (!ipmi_si_is_busy(busy_until)) { - ktime_get_ts(busy_until); - timespec_add_ns(busy_until, max_busy_us * NSEC_PER_USEC); + *busy_until = IPMI_TIME_NOT_BUSY; + else if (*busy_until == IPMI_TIME_NOT_BUSY) { + *busy_until = ktime_get() + max_busy_us * NSEC_PER_USEC; } else { - struct timespec now; - - ktime_get_ts(&now); - if (unlikely(timespec_compare(&now, busy_until) > 0)) { - ipmi_si_set_not_busy(busy_until); + if (unlikely(ktime_get() > *busy_until)) { + *busy_until = IPMI_TIME_NOT_BUSY; return false; } } @@ -988,9 +975,8 @@ static int ipmi_thread(void *data) struct smi_info *smi_info = data; unsigned long flags; enum si_sm_result smi_result; - struct timespec busy_until = { 0, 0 }; + ktime_t busy_until = IPMI_TIME_NOT_BUSY;
- ipmi_si_set_not_busy(&busy_until); set_user_nice(current, MAX_NICE); while (!kthread_should_stop()) { int busy_wait;
On Fri, Nov 08, 2019 at 09:34:27PM +0100, Arnd Bergmann wrote:
'struct timespec' is getting removed from the kernel. The usage in ipmi was fixed before in commit 48862ea2ce86 ("ipmi: Update timespec usage to timespec64"), but unfortunately it crept back in.
The busy looping code can better use ktime_t anyway, so use that there to simplify the implementation.
Thanks, this is a big improvement. I have this queued, but if you are going to submit this, I can remove it, and:
Reviewed-by: Corey Minyard cminyard@mvista.com
Do you think this should go in to 5.4?
-corey
Fixes: cbb19cb1eef0 ("ipmi_si: Convert timespec64 to timespec") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/char/ipmi/ipmi_si_intf.c | 40 +++++++++++--------------------- 1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 6b9a0593d2eb..c7cc8538b84a 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -265,10 +265,10 @@ static void cleanup_ipmi_si(void); #ifdef DEBUG_TIMING void debug_timestamp(char *msg) {
- struct timespec t;
- struct timespec64 t;
- ktime_get_ts(&t);
- pr_debug("**%s: %ld.%9.9ld\n", msg, (long) t.tv_sec, t.tv_nsec);
- ktime_get_ts64(&t);
- pr_debug("**%s: %lld.%9.9ld\n", msg, t.tv_sec, t.tv_nsec);
} #else #define debug_timestamp(x) @@ -935,38 +935,25 @@ static void set_run_to_completion(void *send_info, bool i_run_to_completion) } /*
- Use -1 in the nsec value of the busy waiting timespec to tell that
- we are spinning in kipmid looking for something and not delaying
- between checks
- Use -1 as a special constant to tell that we are spinning in kipmid
*/
- looking for something and not delaying between checks
-static inline void ipmi_si_set_not_busy(struct timespec *ts) -{
- ts->tv_nsec = -1;
-} -static inline int ipmi_si_is_busy(struct timespec *ts) -{
- return ts->tv_nsec != -1;
-}
+#define IPMI_TIME_NOT_BUSY ns_to_ktime(-1ull) static inline bool ipmi_thread_busy_wait(enum si_sm_result smi_result, const struct smi_info *smi_info,
struct timespec *busy_until)
ktime_t *busy_until){ unsigned int max_busy_us = 0; if (smi_info->si_num < num_max_busy_us) max_busy_us = kipmid_max_busy_us[smi_info->si_num]; if (max_busy_us == 0 || smi_result != SI_SM_CALL_WITH_DELAY)
ipmi_si_set_not_busy(busy_until);- else if (!ipmi_si_is_busy(busy_until)) {
ktime_get_ts(busy_until);timespec_add_ns(busy_until, max_busy_us * NSEC_PER_USEC);
*busy_until = IPMI_TIME_NOT_BUSY;- else if (*busy_until == IPMI_TIME_NOT_BUSY) {
*busy_until = ktime_get() + max_busy_us * NSEC_PER_USEC;
struct timespec now;ktime_get_ts(&now);if (unlikely(timespec_compare(&now, busy_until) > 0)) {ipmi_si_set_not_busy(busy_until);
if (unlikely(ktime_get() > *busy_until)) {*busy_until = IPMI_TIME_NOT_BUSY; return false;@@ -988,9 +975,8 @@ static int ipmi_thread(void *data) struct smi_info *smi_info = data; unsigned long flags; enum si_sm_result smi_result;
- struct timespec busy_until = { 0, 0 };
- ktime_t busy_until = IPMI_TIME_NOT_BUSY;
- ipmi_si_set_not_busy(&busy_until); set_user_nice(current, MAX_NICE); while (!kthread_should_stop()) { int busy_wait;
-- 2.20.0
On Fri, Nov 8, 2019 at 11:11 PM Corey Minyard cminyard@mvista.com wrote:
On Fri, Nov 08, 2019 at 09:34:27PM +0100, Arnd Bergmann wrote:
'struct timespec' is getting removed from the kernel. The usage in ipmi was fixed before in commit 48862ea2ce86 ("ipmi: Update timespec usage to timespec64"), but unfortunately it crept back in.
The busy looping code can better use ktime_t anyway, so use that there to simplify the implementation.
Thanks, this is a big improvement. I have this queued, but if you are going to submit this, I can remove it, and:
Reviewed-by: Corey Minyard cminyard@mvista.com
I'd prefer to have this go through your tree, one less thing for me to worry about (out of the 90 patches).
Do you think this should go in to 5.4?
Up to you, it probably depends on how well you can test that the change is correct beyond the review.
Arnd
The current xt_time driver suffers from the y2038 overflow on 32-bit architectures, when the time of day calculations break.
Also, on both 32-bit and 64-bit architectures, there is a problem with info->date_start/stop, which is part of the user ABI and overflows in in 2106.
Fix the first issue by using time64_t and explicit calls to div_u64() and div_u64_rem(), and document the seconds issue.
The explicit 64-bit division is unfortunately slower on 32-bit architectures, but doing it as unsigned lets us use the optimized division-through-multiplication path in most configurations. This should be fine, as the code already does not allow any negative time of day values.
Using u32 seconds values consistently would probably also work and be a little more efficient, but that doesn't feel right as it would propagate the y2106 overflow to more place rather than fewer.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- net/netfilter/xt_time.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c index 8dbb4d48f2ed..67cb98489415 100644 --- a/net/netfilter/xt_time.c +++ b/net/netfilter/xt_time.c @@ -77,12 +77,12 @@ static inline bool is_leap(unsigned int y) * This is done in three separate functions so that the most expensive * calculations are done last, in case a "simple match" can be found earlier. */ -static inline unsigned int localtime_1(struct xtm *r, time_t time) +static inline unsigned int localtime_1(struct xtm *r, time64_t time) { unsigned int v, w;
/* Each day has 86400s, so finding the hour/minute is actually easy. */ - v = time % SECONDS_PER_DAY; + div_u64_rem(time, SECONDS_PER_DAY, &v); r->second = v % 60; w = v / 60; r->minute = w % 60; @@ -90,13 +90,13 @@ static inline unsigned int localtime_1(struct xtm *r, time_t time) return v; }
-static inline void localtime_2(struct xtm *r, time_t time) +static inline void localtime_2(struct xtm *r, time64_t time) { /* * Here comes the rest (weekday, monthday). First, divide the SSTE * by seconds-per-day to get the number of _days_ since the epoch. */ - r->dse = time / 86400; + r->dse = div_u64(time, SECONDS_PER_DAY);
/* * 1970-01-01 (w=0) was a Thursday (4). @@ -105,7 +105,7 @@ static inline void localtime_2(struct xtm *r, time_t time) r->weekday = (4 + r->dse - 1) % 7 + 1; }
-static void localtime_3(struct xtm *r, time_t time) +static void localtime_3(struct xtm *r, time64_t time) { unsigned int year, i, w = r->dse;
@@ -160,7 +160,7 @@ time_mt(const struct sk_buff *skb, struct xt_action_param *par) const struct xt_time_info *info = par->matchinfo; unsigned int packet_time; struct xtm current_time; - s64 stamp; + time64_t stamp;
/* * We need real time here, but we can neither use skb->tstamp @@ -173,14 +173,14 @@ time_mt(const struct sk_buff *skb, struct xt_action_param *par) * 1. match before 13:00 * 2. match after 13:00 * - * If you match against processing time (get_seconds) it + * If you match against processing time (ktime_get_real_seconds) it * may happen that the same packet matches both rules if * it arrived at the right moment before 13:00, so it would be * better to check skb->tstamp and set it via __net_timestamp() * if needed. This however breaks outgoing packets tx timestamp, * and causes them to get delayed forever by fq packet scheduler. */ - stamp = get_seconds(); + stamp = ktime_get_real_seconds();
if (info->flags & XT_TIME_LOCAL_TZ) /* Adjust for local timezone */ @@ -193,6 +193,9 @@ time_mt(const struct sk_buff *skb, struct xt_action_param *par) * - 'now' is in the weekday mask * - 'now' is in the daytime range time_start..time_end * (and by default, libxt_time will set these so as to match) + * + * note: info->date_start/stop are unsigned 32-bit values that + * can hold values beyond y2038, but not after y2106. */
if (stamp < info->date_start || stamp > info->date_stop)
On Fri, Nov 08, 2019 at 09:34:28PM +0100, Arnd Bergmann wrote:
The current xt_time driver suffers from the y2038 overflow on 32-bit architectures, when the time of day calculations break.
Also, on both 32-bit and 64-bit architectures, there is a problem with info->date_start/stop, which is part of the user ABI and overflows in in 2106.
Fix the first issue by using time64_t and explicit calls to div_u64() and div_u64_rem(), and document the seconds issue.
The explicit 64-bit division is unfortunately slower on 32-bit architectures, but doing it as unsigned lets us use the optimized division-through-multiplication path in most configurations. This should be fine, as the code already does not allow any negative time of day values.
Using u32 seconds values consistently would probably also work and be a little more efficient, but that doesn't feel right as it would propagate the y2106 overflow to more place rather than fewer.
Applied, thanks.
The layout of struct timeval is different on sparc64 from anything else, and the patch I did long ago failed to take this into account.
Change it now to handle sparc64 user space correctly again.
Quite likely nobody cares about parallel ports on sparc64, but there is no reason not to fix it.
Cc: stable@vger.kernel.org Fixes: 9a450484089d ("lp: support 64-bit time_t user space") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/char/lp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index 7c9269e3477a..bd95aba1f9fe 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
+ /* sparc64 suseconds_t is 32-bit only */ + if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall()) + karg[1] >>= 32; + return lp_set_timeout(minor, karg[0], karg[1]); }
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
The layout of struct timeval is different on sparc64 from anything else, and the patch I did long ago failed to take this into account.
Change it now to handle sparc64 user space correctly again.
Quite likely nobody cares about parallel ports on sparc64, but there is no reason not to fix it.
Cc: stable@vger.kernel.org Fixes: 9a450484089d ("lp: support 64-bit time_t user space") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/char/lp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index 7c9269e3477a..bd95aba1f9fe 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
- /* sparc64 suseconds_t is 32-bit only */
- if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
karg[1] >>= 32;- return lp_set_timeout(minor, karg[0], karg[1]);
}
It seems like it would make way more sense to use __kernel_old_timeval. Then you don't have to explicitly handle the sparc64 oddity.
As it is, this still over-reads from user-space which might result in a spurious -EFAULT.
Ben.
On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
The layout of struct timeval is different on sparc64 from anything else, and the patch I did long ago failed to take this into account.
Change it now to handle sparc64 user space correctly again.
Quite likely nobody cares about parallel ports on sparc64, but there is no reason not to fix it.
Cc: stable@vger.kernel.org Fixes: 9a450484089d ("lp: support 64-bit time_t user space") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/char/lp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index 7c9269e3477a..bd95aba1f9fe 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
/* sparc64 suseconds_t is 32-bit only */if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())karg[1] >>= 32;return lp_set_timeout(minor, karg[0], karg[1]);}
It seems like it would make way more sense to use __kernel_old_timeval.
Right, that would work. I tried to keep the patch small here, changing it to __kernel_old_timeval would require make it all more complicated since it would still need to check some conditional to tell the difference between sparc32 and sparc64.
I think this patch (relative to the version I posted) would work the same:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index bd95aba1f9fe..86994421ee97 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,13 +713,19 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
- /* sparc64 suseconds_t is 32-bit only */ - if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall()) - karg[1] >>= 32; - return lp_set_timeout(minor, karg[0], karg[1]); }
+static int lp_set_timeout(unsigned int minor, void __user *arg) +{ + __kernel_old_timeval tv; + + if (copy_from_user(tv, arg, sizeof(karg))) + return -EFAULT; + + return lp_set_timeout(minor, tv->tv_sec, tv->tv_usec); +} + static long lp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd, mutex_lock(&lp_mutex); switch (cmd) { case LPSETTIMEOUT_OLD: - if (BITS_PER_LONG == 32) { - ret = lp_set_timeout32(minor, (void __user *)arg); - break; - } - /* fall through - for 64-bit */ + ret = lp_set_timeout(minor, (void __user *)arg); + break; case LPSETTIMEOUT_NEW: ret = lp_set_timeout64(minor, (void __user *)arg); break;
Do you like that better? One difference here is the handling of LPSETTIMEOUT_NEW on sparc64, which would continue to use the 64/64 layout rather than the 64/32/pad layout, but that should be ok, since sparc64 user space using ppdev (if any exists) would use LPSETTIMEOUT_OLD, not LPSETTIMEOUT_NEW.
Then you don't have to explicitly handle the sparc64 oddity.
As it is, this still over-reads from user-space which might result in a spurious -EFAULT.
I think you got this wrong: sparc64 like most architectures naturally aligns 64-bit members, so 'struct timeval' still uses 16 bytes including the four padding bytes at the end, it just has the nanoseconds in a different position from all other big-endian architectures.
Arnd
On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
The layout of struct timeval is different on sparc64 from anything else, and the patch I did long ago failed to take this into account.
Change it now to handle sparc64 user space correctly again.
Quite likely nobody cares about parallel ports on sparc64, but there is no reason not to fix it.
Cc: stable@vger.kernel.org Fixes: 9a450484089d ("lp: support 64-bit time_t user space") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/char/lp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index 7c9269e3477a..bd95aba1f9fe 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
/* sparc64 suseconds_t is 32-bit only */if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())karg[1] >>= 32;return lp_set_timeout(minor, karg[0], karg[1]);}
It seems like it would make way more sense to use __kernel_old_timeval.
Right, that would work. I tried to keep the patch small here, changing it to __kernel_old_timeval would require make it all more complicated since it would still need to check some conditional to tell the difference between sparc32 and sparc64.
Right.
I think this patch (relative to the version I posted) would work the same:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index bd95aba1f9fe..86994421ee97 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -713,13 +713,19 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
/* sparc64 suseconds_t is 32-bit only */if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())karg[1] >>= 32;return lp_set_timeout(minor, karg[0], karg[1]);}
+static int lp_set_timeout(unsigned int minor, void __user *arg)
That function name is already used! Maybe this should be lp_set_timeout_old()?
+{
__kernel_old_timeval tv;if (copy_from_user(tv, arg, sizeof(karg)))return -EFAULT;return lp_set_timeout(minor, tv->tv_sec, tv->tv_usec);+}
static long lp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd, mutex_lock(&lp_mutex); switch (cmd) { case LPSETTIMEOUT_OLD:
if (BITS_PER_LONG == 32) {ret = lp_set_timeout32(minor, (void __user *)arg);break;}/* fall through - for 64-bit */
ret = lp_set_timeout(minor, (void __user *)arg);break; case LPSETTIMEOUT_NEW: ret = lp_set_timeout64(minor, (void __user *)arg); break;Do you like that better?
Yes. Aside from the duplicate function name, it looks correct and cleaner than the current version.
One difference here is the handling of LPSETTIMEOUT_NEW on sparc64, which would continue to use the 64/64 layout rather than the 64/32/pad layout, but that should be ok, since sparc64 user space using ppdev (if any exists) would use LPSETTIMEOUT_OLD, not LPSETTIMEOUT_NEW.
Right, that's a little weird but appears to be consistent with "new" socket timestamps.
Then you don't have to explicitly handle the sparc64 oddity.
As it is, this still over-reads from user-space which might result in a spurious -EFAULT.
I think you got this wrong: sparc64 like most architectures naturally aligns 64-bit members, so 'struct timeval' still uses 16 bytes including the four padding bytes at the end, it just has the nanoseconds in a different position from all other big-endian architectures.
Oh of course, yes.
Ben.
On Wed, Nov 20, 2019 at 11:10 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
return lp_set_timeout(minor, karg[0], karg[1]);}
+static int lp_set_timeout(unsigned int minor, void __user *arg)
That function name is already used! Maybe this should be lp_set_timeout_old()?
Yes, that's what I used after actually compile-testing and running into a couple of issues with my draft.
@@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd, mutex_lock(&lp_mutex); switch (cmd) { case LPSETTIMEOUT_OLD:
if (BITS_PER_LONG == 32) {ret = lp_set_timeout32(minor, (void __user *)arg);break;}/* fall through - for 64-bit */
ret = lp_set_timeout(minor, (void __user *)arg);break; case LPSETTIMEOUT_NEW: ret = lp_set_timeout64(minor, (void __user *)arg); break;Do you like that better?
Yes. Aside from the duplicate function name, it looks correct and cleaner than the current version.
As Greg has already merged the original patch, and that version works just as well, I'd probably just leave what I did at first. One benefit is that in case we decide to kill off sparc64 support before drivers/char/lp.c, the special case can be removed more easily.
I don't think either of them is going any time soon, but working on y2038 patches has made me think ahead longer term ;-)
If you still think we should change it I can send the below patch (now actually build-tested) with your Ack.
Arnd --- commit 93efbb1768a5071a0a98bf4627f0104075cf83a6 (HEAD -> y2038) Author: Arnd Bergmann arnd@arndb.de Date: Thu Nov 21 14:45:14 2019 +0100
lp: clean up set_timeout handling
As Ben Hutchings noticed, we can avoid the special case for sparc64 by dealing with '__kernel_old_timeval' arguments separately from the fixed-length 32-bit and 64-bit arguments.
Note that the behavior for LPSETTIMEOUT_NEW changes on sparc64 to expect the same argument as other architectures, but this is ok because sparc64 users would pass LPSETTIMEOUT_OLD anyway.
Suggested-by: Ben Hutchings ben.hutchings@codethink.co.uk Signed-off-by: Arnd Bergmann arnd@arndb.de
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index bd95aba1f9fe..cc17d5a387c5 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -696,14 +696,14 @@ static int lp_set_timeout(unsigned int minor, s64 tv_sec, long tv_usec) return 0; }
-static int lp_set_timeout32(unsigned int minor, void __user *arg) +static int lp_set_timeout_old(unsigned int minor, void __user *arg) { - s32 karg[2]; + struct __kernel_old_timeval tv;
- if (copy_from_user(karg, arg, sizeof(karg))) + if (copy_from_user(&tv, arg, sizeof(tv))) return -EFAULT;
- return lp_set_timeout(minor, karg[0], karg[1]); + return lp_set_timeout(minor, tv.tv_sec, tv.tv_usec); }
static int lp_set_timeout64(unsigned int minor, void __user *arg) @@ -713,10 +713,6 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg) if (copy_from_user(karg, arg, sizeof(karg))) return -EFAULT;
- /* sparc64 suseconds_t is 32-bit only */ - if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall()) - karg[1] >>= 32; - return lp_set_timeout(minor, karg[0], karg[1]); }
@@ -730,11 +726,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd, mutex_lock(&lp_mutex); switch (cmd) { case LPSETTIMEOUT_OLD: - if (BITS_PER_LONG == 32) { - ret = lp_set_timeout32(minor, (void __user *)arg); - break; - } - /* fall through - for 64-bit */ + ret = lp_set_timeout_old(minor, (void __user *)arg); + break; case LPSETTIMEOUT_NEW: ret = lp_set_timeout64(minor, (void __user *)arg); break; @@ -748,6 +741,16 @@ static long lp_ioctl(struct file *file, unsigned int cmd, }
#ifdef CONFIG_COMPAT +static int lp_set_timeout32(unsigned int minor, void __user *arg) +{ + s32 karg[2]; + + if (copy_from_user(karg, arg, sizeof(karg))) + return -EFAULT; + + return lp_set_timeout(minor, karg[0], karg[1]); +} + static long lp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) {
On Thu, 2019-11-21 at 15:04 +0100, Arnd Bergmann wrote: [...]
As Greg has already merged the original patch, and that version works just as well, I'd probably just leave what I did at first. One benefit is that in case we decide to kill off sparc64 support before drivers/char/lp.c, the special case can be removed more easily.
I don't think either of them is going any time soon, but working on y2038 patches has made me think ahead longer term ;-)
If you still think we should change it I can send the below patch (now actually build-tested) with your Ack.
[...]
I would like it, but since you convinced me the current version works correctly it's obvious lower priority than the other changes you have.
Ben.
Going through the uses of timeval in the user space API, I noticed two bugs in ppdev that were introduced in the y2038 conversion:
* The range check was accidentally moved from ppsettime to ppgettime
* On sparc64, the microseconds are in the other half of the 64-bit word.
Fix both, and mark the fix for stable backports.
Cc: stable@vger.kernel.org Fixes: 3b9ab374a1e6 ("ppdev: convert to y2038 safe") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/char/ppdev.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index c86f18aa8985..34bb88fe0b0a 100644 --- a/drivers/char/ppdev.c +++ b/drivers/char/ppdev.c @@ -619,20 +619,27 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (copy_from_user(time32, argp, sizeof(time32))) return -EFAULT;
+ if ((time32[0] < 0) || (time32[1] < 0)) + return -EINVAL; + return pp_set_timeout(pp->pdev, time32[0], time32[1]);
case PPSETTIME64: if (copy_from_user(time64, argp, sizeof(time64))) return -EFAULT;
+ if ((time64[0] < 0) || (time64[1] < 0)) + return -EINVAL; + + if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall()) + time64[1] >>= 32; + return pp_set_timeout(pp->pdev, time64[0], time64[1]);
case PPGETTIME32: jiffies_to_timespec64(pp->pdev->timeout, &ts); time32[0] = ts.tv_sec; time32[1] = ts.tv_nsec / NSEC_PER_USEC; - if ((time32[0] < 0) || (time32[1] < 0)) - return -EINVAL;
if (copy_to_user(argp, time32, sizeof(time32))) return -EFAULT; @@ -643,8 +650,9 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) jiffies_to_timespec64(pp->pdev->timeout, &ts); time64[0] = ts.tv_sec; time64[1] = ts.tv_nsec / NSEC_PER_USEC; - if ((time64[0] < 0) || (time64[1] < 0)) - return -EINVAL; + + if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall()) + time64[1] <<= 32;
if (copy_to_user(argp, time64, sizeof(time64))) return -EFAULT;
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
Going through the uses of timeval in the user space API, I noticed two bugs in ppdev that were introduced in the y2038 conversion:
The range check was accidentally moved from ppsettime to ppgettime
On sparc64, the microseconds are in the other half of the 64-bit word.
Fix both, and mark the fix for stable backports.
Like the patch for lpdev, this also doesn't completely fix sparc64.
Ben.
Cc: stable@vger.kernel.org Fixes: 3b9ab374a1e6 ("ppdev: convert to y2038 safe") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/char/ppdev.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index c86f18aa8985..34bb88fe0b0a 100644 --- a/drivers/char/ppdev.c +++ b/drivers/char/ppdev.c @@ -619,20 +619,27 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (copy_from_user(time32, argp, sizeof(time32))) return -EFAULT;
if ((time32[0] < 0) || (time32[1] < 0))return -EINVAL;- return pp_set_timeout(pp->pdev, time32[0], time32[1]);
case PPSETTIME64: if (copy_from_user(time64, argp, sizeof(time64))) return -EFAULT;
if ((time64[0] < 0) || (time64[1] < 0))return -EINVAL;if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())time64[1] >>= 32;- return pp_set_timeout(pp->pdev, time64[0], time64[1]);
case PPGETTIME32: jiffies_to_timespec64(pp->pdev->timeout, &ts); time32[0] = ts.tv_sec; time32[1] = ts.tv_nsec / NSEC_PER_USEC;
if ((time32[0] < 0) || (time32[1] < 0))return -EINVAL;if (copy_to_user(argp, time32, sizeof(time32))) return -EFAULT; @@ -643,8 +650,9 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) jiffies_to_timespec64(pp->pdev->timeout, &ts); time64[0] = ts.tv_sec; time64[1] = ts.tv_nsec / NSEC_PER_USEC;
if ((time64[0] < 0) || (time64[1] < 0))return -EINVAL;
if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())time64[1] <<= 32;if (copy_to_user(argp, time64, sizeof(time64))) return -EFAULT;
On Wed, Nov 20, 2019 at 8:29 PM Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
Going through the uses of timeval in the user space API, I noticed two bugs in ppdev that were introduced in the y2038 conversion:
The range check was accidentally moved from ppsettime to ppgettime
On sparc64, the microseconds are in the other half of the 64-bit word.
Fix both, and mark the fix for stable backports.
Like the patch for lpdev, this also doesn't completely fix sparc64.
I think the same applies as in the other patch:
- it actually works correctly because of the alignment - it's already merged in linux-next - if you wish, I can add another cleanup patch on top, but I'd prefer to just leave it as it is.
Arnd
Going through all uses of timeval, I noticed that we screwed up input_event in the previous attempts to fix it:
The time fields now match between kernel and user space, but all following fields are in the wrong place.
Add the required padding that is implied by the glibc timeval definition to fix the layout, and add explicit initialization to avoid leaking kernel stack data.
Cc: sparclinux@vger.kernel.org Cc: "David S. Miller" davem@davemloft.net Cc: stable@vger.kernel.org Fixes: 141e5dcaa735 ("Input: input_event - fix the CONFIG_SPARC64 mixup") Fixes: 2e746942ebac ("Input: input_event - provide override for sparc64") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/input/evdev.c | 3 +++ drivers/input/misc/uinput.c | 3 +++ include/uapi/linux/input.h | 1 + 3 files changed, 7 insertions(+)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index d7dd6fcf2db0..24a90793caf0 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -228,6 +228,9 @@ static void __pass_event(struct evdev_client *client, event->input_event_sec; client->buffer[client->tail].input_event_usec = event->input_event_usec; +#ifdef CONFIG_SPARC64 + client->buffer[client->tail].__pad = 0; +#endif client->buffer[client->tail].type = EV_SYN; client->buffer[client->tail].code = SYN_DROPPED; client->buffer[client->tail].value = 0; diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 84051f20b18a..1d8c09e9fd47 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -80,6 +80,9 @@ static int uinput_dev_event(struct input_dev *dev, ktime_get_ts64(&ts); udev->buff[udev->head].input_event_sec = ts.tv_sec; udev->buff[udev->head].input_event_usec = ts.tv_nsec / NSEC_PER_USEC; +#ifdef CONFIG_SPARC64 + udev->buff[udev->head].__pad = 0; +#endif udev->head = (udev->head + 1) % UINPUT_BUFFER_SIZE;
wake_up_interruptible(&udev->waitq); diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h index f056b2a00d5c..9a61c28ed3ae 100644 --- a/include/uapi/linux/input.h +++ b/include/uapi/linux/input.h @@ -34,6 +34,7 @@ struct input_event { __kernel_ulong_t __sec; #if defined(__sparc__) && defined(__arch64__) unsigned int __usec; + unsigned int __pad; #else __kernel_ulong_t __usec; #endif
Hi Arnd,
On Fri, Nov 08, 2019 at 09:34:31PM +0100, Arnd Bergmann wrote:
Going through all uses of timeval, I noticed that we screwed up input_event in the previous attempts to fix it:
The time fields now match between kernel and user space, but all following fields are in the wrong place.
Add the required padding that is implied by the glibc timeval definition to fix the layout, and add explicit initialization to avoid leaking kernel stack data.
Cc: sparclinux@vger.kernel.org Cc: "David S. Miller" davem@davemloft.net Cc: stable@vger.kernel.org Fixes: 141e5dcaa735 ("Input: input_event - fix the CONFIG_SPARC64 mixup") Fixes: 2e746942ebac ("Input: input_event - provide override for sparc64") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/input/evdev.c | 3 +++ drivers/input/misc/uinput.c | 3 +++ include/uapi/linux/input.h | 1 + 3 files changed, 7 insertions(+)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index d7dd6fcf2db0..24a90793caf0 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -228,6 +228,9 @@ static void __pass_event(struct evdev_client *client, event->input_event_sec; client->buffer[client->tail].input_event_usec = event->input_event_usec; +#ifdef CONFIG_SPARC64
client->buffer[client->tail].__pad = 0;+#endif client->buffer[client->tail].type = EV_SYN; client->buffer[client->tail].code = SYN_DROPPED; client->buffer[client->tail].value = 0;
I do not like ifdefs here, do you think we could write:
client->buffer[client->tail] = (struct input_event) { .input_event_sec = event->input_event_sec, .input_event_usec = event->input_event_usec, .type = EV_SYN, .code = SYN_DROPPED, };
to ensure all padded fields are initialized? This is not hot path as we do not expect queue to overfill too often.
Thanks.
On Mon, Nov 11, 2019 at 7:28 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:
I do not like ifdefs here, do you think we could write:
client->buffer[client->tail] = (struct input_event) { .input_event_sec = event->input_event_sec, .input_event_usec = event->input_event_usec, .type = EV_SYN, .code = SYN_DROPPED, };to ensure all padded fields are initialized? This is not hot path as we do not expect queue to overfill too often.
Good idea, changed both instances now. Thanks for taking a look!
Arnd
-
Arnd Bergmann -
Ben Hutchings -
Corey Minyard -
Deepa Dinamani -
Dmitry Torokhov -
Pablo Neira Ayuso