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

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

[John Stultz]

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

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

[John Stultz]

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

[tip:timers/urgent] clocksource: Fix abs() usage w/ 64bit values

[tip-bot for John Stultz]

Commit-ID:  67dfae0cd72fec5cd158b6e5fb1647b7dbe0834c
Gitweb:     http://git.kernel.org/tip/67dfae0cd72fec5cd158b6e5fb1647b7dbe0834c
Author:     John Stultz <john.stultz@linaro.org>
AuthorDate: Mon, 14 Sep 2015 18:05:20 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 2 Oct 2015 22:53:01 +0200

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

Signed-off-by: John Stultz <john.stultz@linaro.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/1442279124-7309-2-git-send-email-john.stultz@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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",

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

[John Stultz]

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

[RFC][PATCH 3/5] ext4: Fix abs() usage in ext4_mb_check_group_pa

[John Stultz]

The ext4_fsblk_t type is a long long, which should not be used
with abs(), as is done in ext4_mb_check_group_pa().

This patch modifies ext4_mb_check_group_pa() to use abs64()
instead.

Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-ext4@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 fs/ext4/mballoc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 34b610e..4780ab1 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3331,8 +3331,8 @@ ext4_mb_check_group_pa(ext4_fsblk_t goal_block,
 		atomic_inc(&pa->pa_count);
 		return pa;
 	}
-	cur_distance = abs(goal_block - cpa->pa_pstart);
-	new_distance = abs(goal_block - pa->pa_pstart);
+	cur_distance = abs64(goal_block - cpa->pa_pstart);
+	new_distance = abs64(goal_block - pa->pa_pstart);
 
 	if (cur_distance <= new_distance)
 		return cpa;
-- 
1.9.1

[RFC][PATCH 4/5] percpu: Fix abs() usage in percpu_counter_compare()

[John Stultz]

s64 values should not be used with abs(), as is one
in __percpu_counter_compare(), since it may cap the
result to 32-bits.

This patch modifies __percpu_counter_compare() to
use abs64() instead.

Cc: Dave Chinner <dchinner@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 lib/percpu_counter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index f051d69..3d1aba9 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -203,7 +203,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
 
 	count = percpu_counter_read(fbc);
 	/* Check to see if rough count will be sufficient for comparison */
-	if (abs(count - rhs) > (batch * num_online_cpus())) {
+	if (abs64(count - rhs) > (batch * num_online_cpus())) {
 		if (count > rhs)
 			return 1;
 		else
-- 
1.9.1

[RFC][PATCH 5/5] abs(): Provide build error on passing 64bit value to abs()

[John Stultz]

As noted in the comment above abs():
 "abs() should not be used for 64-bit types (s64, u64, long long)
  - use abs64()  for those."

Unfortunately, its quite easy to pass 64-bit values to abs()
accidentally, and the compiler provides no warning when the
returned value is erroniously capped at 32-bits.

So this patch tries to make it easier to detect when 64-bit
values are passed to abs() by generating a build error.

Obviously, since this causes build errors, this patch is last
in the series, and I tried to fix up all of the issues I ran
into in my build testing. But there are likely still some out
there.

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>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/kernel.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410..6f01151 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -208,6 +208,9 @@ extern int _cond_resched(void);
  */
 #define abs(x) ({						\
 		long ret;					\
+		compiletime_assert(				\
+			!(sizeof(typeof(x)) > sizeof(long)),	\
+			"abs() should not be used for 64-bit types - use abs64()");\
 		if (sizeof(x) == sizeof(long)) {		\
 			long __x = (x);				\
 			ret = (__x < 0) ? -__x : __x;		\
-- 
1.9.1

Re: [RFC][PATCH 5/5] abs(): Provide build error on passing 64bit value to abs()

[Ingo Molnar]

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

> As noted in the comment above abs():
>  "abs() should not be used for 64-bit types (s64, u64, long long)
>   - use abs64()  for those."
> 
> Unfortunately, its quite easy to pass 64-bit values to abs()
> accidentally, and the compiler provides no warning when the
> returned value is erroniously capped at 32-bits.
> 
> So this patch tries to make it easier to detect when 64-bit
> values are passed to abs() by generating a build error.
> 
> Obviously, since this causes build errors, this patch is last
> in the series, and I tried to fix up all of the issues I ran
> into in my build testing. But there are likely still some out
> there.
> 
> 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>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/linux/kernel.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 5582410..6f01151 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -208,6 +208,9 @@ extern int _cond_resched(void);
>   */
>  #define abs(x) ({						\
>  		long ret;					\
> +		compiletime_assert(				\
> +			!(sizeof(typeof(x)) > sizeof(long)),	\
> +			"abs() should not be used for 64-bit types - use abs64()");\
>  		if (sizeof(x) == sizeof(long)) {		\
>  			long __x = (x);				\
>  			ret = (__x < 0) ? -__x : __x;		\

I think this should be a compiletime_warning() - that will be visible enough.

Thanks,

	Ingo

Re: [RFC][PATCH 5/5] abs(): Provide build error on passing 64bit value to abs()

[Linus Torvalds]

On Mon, Sep 14, 2015 at 10:22 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> I think this should be a compiletime_warning() - that will be visible enough.

So the problem with this is that by now most kernel developers are on 64-bit.

And that "sizeof(typeof(x)) > sizeof(long))" would effectively never
trigger on 64-bit architectures, so almost no core developers would
see it. Yes, it would be caught by buildbots etc, but that's really
not very convenient. The new errors would be noticed too late, because
the actual *developers* wouldn't see them.

(Not to mention that the "typeof()" in that expression is redundant ;)

So I think the "auto-expand to 's64' using __builtin_choose_expr()" is
the preferable model, and get rid of abs64() entirely. It has very few
uses.

                  Linus

[PATCH] kernel.h: make abs() work with 64-bit types

[Michal Nazarewicz]

For 64-bit arguments, abs macro casts it to an int which leads to lost
precision and may cause incorrect results.  To deal with 64-bit types
abs64 macro has been introduced but still there are places where abs
macro is used incorrectly.

To deal with the problem, expand abs macro such that it operates on s64
type when dealing with 64-bit types while still returning long when
dealing with smaller types.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/kernel.h | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

On Tue, Sep 15 2015, Linus Torvalds wrote:
> So I think the "auto-expand to 's64' using __builtin_choose_expr()" is
> the preferable model, and get rid of abs64() entirely. It has very few
> uses.

Compile tested (with ‘make allmodconfig && make bzImage modules’ on
x86_64) only.

The 32-bit case could be further ‘simplified’ with:

		typeof(__builtin_choose_expr(sizeof(x) == sizeof(long), \
		                             1L, 1)) __x = (x);         \
		(long)(__x < 0 ? -__x : __x);  

but I don’t suppose the few saved lines (and hacker-cred ;) ) are worth
added confusion or risk of angry developer attacking me with a blunt
instrument after they had to debug the code for previous fortnight.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410..f985d16 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -200,28 +200,31 @@ extern int _cond_resched(void);
 
 #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
 
-/*
- * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
- * input types abs() returns a signed long.
- * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
- * for those.
+/**
+ * abs - return absolute value of an argument
+ * @x: the value.  If it is unsigned type, it is converted to signed type first
+ *   (s64, long or int depending on its size).
+ *
+ * Return: an absolute value of x.  If x is 64-bit, macro's return type is s64,
+ *   otherwise it is signed long.
  */
-#define abs(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;						\
-	})
-
-#define abs64(x) ({				\
-		s64 __x = (x);			\
-		(__x < 0) ? -__x : __x;		\
-	})
+#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({	\
+		s64 __x = (x);						\
+		(__x < 0) ? -__x : __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;							\
+	}))
+
+/* Deprecated, use abs instead. */
+#define abs64(x) abs((s64)(x))
 
 /**
  * reciprocal_scale - "scale" a value into range [0, ep_ro)
-- 
2.6.0.rc0.131.gf624c3d

Re: [PATCH] kernel.h: make abs() work with 64-bit types

[John Stultz]

On Wed, Sep 16, 2015 at 5:57 AM, Michal Nazarewicz <mina86@mina86.com> wrote:
> For 64-bit arguments, abs macro casts it to an int which leads to lost
> precision and may cause incorrect results.  To deal with 64-bit types
> abs64 macro has been introduced but still there are places where abs
> macro is used incorrectly.
>
> To deal with the problem, expand abs macro such that it operates on s64
> type when dealing with 64-bit types while still returning long when
> dealing with smaller types.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Sorry this is a little late, but I did run this patch through my tests
on a 32bit system (against 4.2, since my localized fix has already
landed in Linus' head) to make sure it resolved the bug I saw and it
did.

So,
Tested-by: John Stultz <john.stultz@linaro.org>

thanks!
-john

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

[Tejun Heo]

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

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

[John Stultz]

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

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

[Tejun Heo]

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

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

[Jeff Epler]

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

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

[Andrew Morton]

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.

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

[Michal Nazarewicz]

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

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

[Ingo Molnar]

* 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

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

[Linus Torvalds]

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