Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224571 > unrolled thread

[RFC][PATCH 0/5] Fixes for abs() usage on 64bit values

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values John Stultz <john.stultz@linaro.org> - 2015-09-15 03:10 +0200
    [RFC][PATCH 1/5] clocksource: Fix abs() usage w/ 64bit values John Stultz <john.stultz@linaro.org> - 2015-09-15 03:10 +0200
    [RFC][PATCH 2/5] time: Fix abs() usage with 64-bit values. John Stultz <john.stultz@linaro.org> - 2015-09-15 03:10 +0200
    Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Tejun Heo <tj@kernel.org> - 2015-09-15 04:00 +0200
      Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values John Stultz <john.stultz@linaro.org> - 2015-09-15 05:30 +0200
        Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Tejun Heo <tj@kernel.org> - 2015-09-15 05:50 +0200
          Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Jeff Epler <jepler@unpythonic.net> - 2015-09-15 14:50 +0200
          Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Andrew Morton <akpm@linux-foundation.org> - 2015-09-15 23:30 +0200
            Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Michal Nazarewicz <mina86@mina86.com> - 2015-09-16 01:00 +0200
        Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Ingo Molnar <mingo@kernel.org> - 2015-09-15 07:30 +0200
          Re: [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-16 01:50 +0200

#1224571 — [RFC][PATCH 0/5] Fixes for abs() usage on 64bit values

As noted in include/linux/kernel.h:
 "abs() should not be used for 64-bit types (s64, u64, long long)
 - use abs64() for those."

Unfortunately, there are quite a number of places where abs()
was used w/ 64bit values in the kernel, and the results are
then silently capped to 32-bit values on 32-bit systems.

This series tries to address the problematic sites I found,
and then introduces a patch which modifies abs() so that the
build will fail if a 64-bit type is passed to it on a 32-bit
machine.

I'm sure there are additional sites that will need fixing,
but hopefully this will make them easy to find.

Comments and feedback would be greatly appreciated!

Thanks
-john

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Joe Perches <joe@perches.com>


John Stultz (5):
  clocksource: Fix abs() usage w/ 64bit values
  time: Fix abs() usage with 64-bit values.
  ext4: Fix abs() usage in  ext4_mb_check_group_pa
  percpu: Fix abs() usage in percpu_counter_compare()
  abs(): Provide build error on passing 64bit value to abs()

 fs/ext4/mballoc.c         | 4 ++--
 include/linux/kernel.h    | 3 +++
 kernel/time/clocksource.c | 2 +-
 kernel/time/timekeeping.c | 2 +-
 lib/percpu_counter.c      | 2 +-
 5 files changed, 8 insertions(+), 5 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1224572 — [RFC][PATCH 1/5] clocksource: Fix abs() usage w/ 64bit values

This patch fixes one cases where abs() was being used with 64-bit
nanosecond values, where the result may be capped at 32-bits.

This potentially could cause watchdog false negatives on 32-bit
systems, so this patch addresses the issue by using abs64().

Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/clocksource.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 841b72f..3a38775 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -217,7 +217,7 @@ static void clocksource_watchdog(unsigned long data)
 			continue;
 
 		/* Check the deviation from the watchdog clocksource. */
-		if ((abs(cs_nsec - wd_nsec) > WATCHDOG_THRESHOLD)) {
+		if (abs64(cs_nsec - wd_nsec) > WATCHDOG_THRESHOLD) {
 			pr_warn("timekeeping watchdog: Marking clocksource '%s' as unstable because the skew is too large:\n",
 				cs->name);
 			pr_warn("                      '%s' wd_now: %llx wd_last: %llx mask: %llx\n",
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224574 — [RFC][PATCH 2/5] time: Fix abs() usage with 64-bit values.

This patch fixes a usage of abs() with a 64-bit value which could
truncate the result to 32-bits, by replacing it with abs64().

In this case, its unlikely any issue could have been caused by
this, since we're calculating a second delta, which wouldn't be
larger then 32-bit.

Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3739ac6..3172823f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1464,7 +1464,7 @@ int timekeeping_suspend(void)
 		 */
 		delta = timespec64_sub(tk_xtime(tk), timekeeping_suspend_time);
 		delta_delta = timespec64_sub(delta, old_delta);
-		if (abs(delta_delta.tv_sec) >= 2) {
+		if (abs64(delta_delta.tv_sec) >= 2) {
 			/*
 			 * if delta_delta is too large, assume time correction
 			 * has occurred and set old_delta to the current delta.
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224586

Hello,

On Mon, Sep 14, 2015 at 06:05:19PM -0700, John Stultz wrote:
> As noted in include/linux/kernel.h:
>  "abs() should not be used for 64-bit types (s64, u64, long long)
>  - use abs64() for those."
> 
> Unfortunately, there are quite a number of places where abs()
> was used w/ 64bit values in the kernel, and the results are
> then silently capped to 32-bit values on 32-bit systems.

I don't get it.  Why can't we just do the following?

#define abs(x)									\
({										\
	 typeof(x) __x = (x);							\
	 __x < 0 ? -__x : __x;							\
})

The current macros are kinda broken because they'd end up converting
u32 or u64 values which are over the max values of signed counterparts
to their complements.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224620

On Mon, Sep 14, 2015 at 6:49 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Mon, Sep 14, 2015 at 06:05:19PM -0700, John Stultz wrote:
>> As noted in include/linux/kernel.h:
>>  "abs() should not be used for 64-bit types (s64, u64, long long)
>>  - use abs64() for those."
>>
>> Unfortunately, there are quite a number of places where abs()
>> was used w/ 64bit values in the kernel, and the results are
>> then silently capped to 32-bit values on 32-bit systems.
>
> I don't get it.  Why can't we just do the following?
>
> #define abs(x)                                                                  \
> ({                                                                              \
>          typeof(x) __x = (x);                                                   \
>          __x < 0 ? -__x : __x;                                                  \
> })
>

Yea. The above make sense to me, but I suspect there's some very
subtle reason for the existing separated logic.
But I'd have to defer to akpm for hints on that.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224625

Hello,

On Mon, Sep 14, 2015 at 08:27:08PM -0700, John Stultz wrote:
> Yea. The above make sense to me, but I suspect there's some very
> subtle reason for the existing separated logic.
> But I'd have to defer to akpm for hints on that.

Hmmm... people could be using it for calculating the distance between
two unsigned values and in that case the original behavior would be
the correct one.  e.g.

 unsigned diff, a, b;
 diff = abs(a - b);

u8 and u16 being treated differently from u32 and u64 makes sense too
because u6 and u16 are always casted to unsigned for calculations
anyway.  Maintaining the current behavior and combining the two isn't
difficult tho.  Sth like the following could work.

 #define abs(x)							\
 ({								\
	typeof(x) __ret;					\
	if (sizeof(x) <= sizeof(s32)) {				\
		s32 __x = (x);					\
		__ret = __x < 0 ? -__x : __x;			\
	} else {						\
		s64 __x = (x);					\
		__ret = __x < 0 ? -__x : __x;			\
	}
	__ret;
 })

It might trigger some printf format warnings due to the change in
return type but I think the end results would be the same as the
combination of the current abs() and abs64().

Anyways, let's please get abs() working for all types, one way or the
other.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225050

On Mon, Sep 14, 2015 at 11:46:32PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Mon, Sep 14, 2015 at 08:27:08PM -0700, John Stultz wrote:
> > Yea. The above make sense to me, but I suspect there's some very
> > subtle reason for the existing separated logic.
> > But I'd have to defer to akpm for hints on that.
> 
> Hmmm... people could be using it for calculating the distance between
> two unsigned values and in that case the original behavior would be
> the correct one.  e.g.
> 
>  unsigned diff, a, b;
>  diff = abs(a - b);

This kind of construct can overflow whether a and b are signed or
unsigned.  Only a two-argument function can correctly return the
absolute difference of two integers.

At my day job, we arranged for
abs(unsigned type) to be a compile-time error and supplied a two args
function absdiff for use in these situations -- absdiff(a,b) is the same
as abs(a-b) except it avoids overflow and returns an unsigned type.

(though I have no doubt that some instances of abs(a - b) still exist
where a and b are signed and the intermediate could still overflow...)

Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225555

On Mon, 14 Sep 2015 23:46:32 -0400 Tejun Heo <tj@kernel.org> wrote:

> Anyways, let's please get abs() working for all types, one way or the
> other.

That would be by far the best solution, of course.

This seems to work OK:

--- a/include/linux/kernel.h~a
+++ a/include/linux/kernel.h
@@ -207,8 +207,11 @@ extern int _cond_resched(void);
  * for those.
  */
 #define abs(x) ({						\
-		long ret;					\
-		if (sizeof(x) == sizeof(long)) {		\
+		s64 ret;					\
+		if (sizeof(x) == sizeof(s64)) {			\
+			s64 __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		} else if (sizeof(x) == sizeof(long)) {		\
 			long __x = (x);				\
 			ret = (__x < 0) ? -__x : __x;		\
 		} else {					\

Test case:

--- /dev/null
+++ a/lib/xx.c
@@ -0,0 +1,33 @@
+#include <linux/kernel.h>
+
+#define newabs(x) ({						\
+		s64 ret;					\
+		if (sizeof(x) == sizeof(s64)) {			\
+			s64 __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		} else if (sizeof(x) == sizeof(long)) {		\
+			long __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		} else {					\
+			int __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		}						\
+		ret;						\
+	})
+
+#define oldabs(x) ({						\
+		long ret;					\
+		if (sizeof(x) == sizeof(long)) {		\
+			long __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		} else {					\
+			int __x = (x);				\
+			ret = (__x < 0) ? -__x : __x;		\
+		}						\
+		ret;						\
+	})
+
+int foo(int x)
+{
+	return oldabs(x);
+}
diff -puN lib/Makefile~b lib/Makefile
--- a/lib/Makefile~b
+++ a/lib/Makefile
@@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
 	 sha1.o md5.o irq_regs.o argv_split.o \
 	 proportions.o flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
-	 earlycpio.o seq_buf.o nmi_backtrace.o
+	 earlycpio.o seq_buf.o nmi_backtrace.o xx.o
 
 obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
 lib-$(CONFIG_MMU) += ioremap.o


on i386, xx.o's text is 68 bytes with either newabs() or oldabs().


lib/percpu_counter.o's text does get larger with newabs().  That's
because __percpu_counter_compare() is doing abs() on an s64, doh.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225572

On Tue, Sep 15 2015, Andrew Morton wrote:
> On Mon, 14 Sep 2015 23:46:32 -0400 Tejun Heo <tj@kernel.org> wrote:
>
>> Anyways, let's please get abs() working for all types, one way or the
>> other.
>
> That would be by far the best solution, of course.
>
> This seems to work OK:
>
> --- a/include/linux/kernel.h~a
> +++ a/include/linux/kernel.h
> @@ -207,8 +207,11 @@ extern int _cond_resched(void);
>   * for those.
>   */
>  #define abs(x) ({						\
> -		long ret;					\
> -		if (sizeof(x) == sizeof(long)) {		\
> +		s64 ret;					\
> +		if (sizeof(x) == sizeof(s64)) {			\
> +			s64 __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		} else if (sizeof(x) == sizeof(long)) {		\
>  			long __x = (x);				\
>  			ret = (__x < 0) ? -__x : __x;		\
>  		} else {					\

If the return type is an issue, we can use __builtin_choose_expr, no?

#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), abs64(x), ({ \
		long ret;					\
		if (sizeof(x) == sizeof(long)) {		\
			long __x = (x);				\
			ret = (__x < 0) ? -__x : __x;		\
		} else {					\
			int __x = (x);				\
			ret = (__x < 0) ? -__x : __x;		\
		}						\
		ret;						\
	}))

This is awkward but will make even printk happy.

>
> Test case:
>
> --- /dev/null
> +++ a/lib/xx.c
> @@ -0,0 +1,33 @@
> +#include <linux/kernel.h>
> +
> +#define newabs(x) ({						\
> +		s64 ret;					\
> +		if (sizeof(x) == sizeof(s64)) {			\
> +			s64 __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		} else if (sizeof(x) == sizeof(long)) {		\
> +			long __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		} else {					\
> +			int __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		}						\
> +		ret;						\
> +	})
> +
> +#define oldabs(x) ({						\
> +		long ret;					\
> +		if (sizeof(x) == sizeof(long)) {		\
> +			long __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		} else {					\
> +			int __x = (x);				\
> +			ret = (__x < 0) ? -__x : __x;		\
> +		}						\
> +		ret;						\
> +	})
> +
> +int foo(int x)
> +{
> +	return oldabs(x);
> +}
> diff -puN lib/Makefile~b lib/Makefile
> --- a/lib/Makefile~b
> +++ a/lib/Makefile
> @@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
>  	 sha1.o md5.o irq_regs.o argv_split.o \
>  	 proportions.o flex_proportions.o ratelimit.o show_mem.o \
>  	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
> -	 earlycpio.o seq_buf.o nmi_backtrace.o
> +	 earlycpio.o seq_buf.o nmi_backtrace.o xx.o
>  
>  obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
>  lib-$(CONFIG_MMU) += ioremap.o
>
>
> on i386, xx.o's text is 68 bytes with either newabs() or oldabs().
>
>
> lib/percpu_counter.o's text does get larger with newabs().  That's
> because __percpu_counter_compare() is doing abs() on an s64, doh.
>

-- 
Best regards,                                            _     _
.o. | Liege of Serenely Enlightened Majesty of         o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>-----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224645

* John Stultz <john.stultz@linaro.org> wrote:

> On Mon, Sep 14, 2015 at 6:49 PM, Tejun Heo <tj@kernel.org> wrote:
> > Hello,
> >
> > On Mon, Sep 14, 2015 at 06:05:19PM -0700, John Stultz wrote:
> >> As noted in include/linux/kernel.h:
> >>  "abs() should not be used for 64-bit types (s64, u64, long long)
> >>  - use abs64() for those."
> >>
> >> Unfortunately, there are quite a number of places where abs()
> >> was used w/ 64bit values in the kernel, and the results are
> >> then silently capped to 32-bit values on 32-bit systems.
> >
> > I don't get it.  Why can't we just do the following?
> >
> > #define abs(x)                                                                  \
> > ({                                                                              \
> >          typeof(x) __x = (x);                                                   \
> >          __x < 0 ? -__x : __x;                                                  \
> > })
> >
> 
> Yea. The above make sense to me, but I suspect there's some very
> subtle reason for the existing separated logic.
> But I'd have to defer to akpm for hints on that.

On one hand there's a real cost from abs() bugs: the fact that abs() trims the 
high bits silently led to a (serious) timekeeping bug on 32-bit kernels, that was 
not found for almost 2 years:

  2619d7e9c92d time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

On the other hand, there's literally hundreds of abs() usages in the kernel - I 
think it would be a lot safer to just introduce a build time warning and migrate 
the few affected ones over to abs64() (i.e. what John has done), than to silently 
change semantics in an all-or-nothing fashion, even if arguably many (most?) of 
the 64-bit values passed to abs() are probably bugs.

This has another advantage: we'll see all the bugs that occured so far, and can 
judge their effect on a case by case basis. There's value in that kind of gradual 
approach as well.

Once we've gone through that fixing process (for 1-2 kernel releases) we could 
perhaps do the change and unify abs() and abs64(): users who really want 32-bit 
trimming in the future can do the cast explicitly.

Linus, any preferences?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225591

On Mon, Sep 14, 2015 at 10:20 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Linus, any preferences?

I like the "auto-expand to 64-bit when necessary", but yes, that thing
needs to continue to use a signed type. Using __builtin_choose_expr()
would seem to be the right thing to do (not Andrew's version that
makes the return type be s64 unconditionally).

Just a quick grep shows that we currently use "abs()" on unsigned long
and we expect it to be a signed comparison with zero, so the "simple"
typeof that John suggested definitely will not work.

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web