Make sure abs() always works on a signed argument
PATCH 1/1 Make sure abs() always works on a signed argument
The abs() macro is commonly used to calculate the modulus of a difference of two numbers. With the introduction of "long" inside the definition of abs() in the commit a49c59c042c63b432307c1bbf7dac5a104c786e6, some of the abs() calls in the kernel started returning unexpected values (for instance, see abs() usage in drivers/video/modedb.c and the resulting problems reported in https://bugs.gentoo.org/show_bug.cgi?id=296539).
The problem is apparent if the argument of abs() is a difference of two 32-bit integers, at least one of which is unsigned. The result is then assumed to be an unsigned integer, which gets cast to a positive long. The return value of abs() is then this large positive integer, instead of the expected small positive integer representing the modulus of the argument.
Example:
u32 a = 0, b = 1; u32 c = abs(a - b);
'c' will end up with a value of 0xffffffff instead of the expected 0x1.
To fix this problem, explicitly cast the argument of abs() to signed, so that it gets properly expanded to long in case the original argument was an int.
--
include/linux/kernel.h +1 -1
@@ -158,7 +158,7 @@ extern int _cond_resched(void);#define might_sleep_if (cond) do { if (cond) might_sleep(); } while (0)#define abs (x) ({ \ - long __x = (x); \ + long __x = (signed)(x); \ (__x < 0) ? -__x : __x; \ })
Quoted 3 lines · Michal Januszewski
To fix this problem, explicitly cast the argument of
abs()to signed, so that it gets properly expanded to long in case the original argument was an int.
This will of course re-introduce the truncation, and effectively reverts a49c59c0.
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84 5EC7 45C6 250E 6F00 984E "And now for something completely different."
PATCH 1/1 Make sure abs() works as expected with both ints and longs
The abs() macro is commonly used to calculate the modulus of a difference of two numbers. With the introduction of "long" inside the definition of abs() in the commit a49c59c042c63b432307c1bbf7dac5a104c786e6, some of the abs() calls in the kernel started returning unexpected values (for instance, see abs() usage in drivers/video/modedb.c).
The problem is apparent if the argument of abs() is a difference of two 32-bit integers, at least one of which is unsigned. The result is then assumed to be an unsigned integer, which gets cast to a positive long. The return value of abs() is then this large positive integer, instead of the expected small positive integer representing the modulus of the argument.
Example:
u32 a = 0, b = 1; u32 c = abs(a - b);
'c' will end up with a value of 0xffffffff instead of the expected 0x1.
To fix this problem, modify the abs() macro so that it detects the size of the argument, and if it's not larger than 32 bits, uses an int instead of a long.
---
OK, please disregard my previous patch. How about this new one? A solution with sizeof() was mentioned as a possibility in the original discussion of a49c59c0, but it looks like it was never actually presented and considered.
include/linux/kernel.h +8 -4
@@ -157,10 +157,14 @@ extern int _cond_resched(void);#define might_sleep_if (cond) do { if (cond) might_sleep(); } while (0) -#define abs (x) ({ \ - long __x = (x); \ - (__x < 0) ? -__x : __x; \ - }) +#define abs (x) ( \ + (sizeof(x) <= 4) ? ({ \ + int __x = (x); \ + (__x < 0) ? -__x : __x; \ + }) : ({ \ + long __x = (x); \ + (__x < 0) ? -__x : __x; \ + }))#ifdef CONFIG_PROVE_LOCKING void might_fault (void );