fs: O_* bit numbers uniqueness check
Subsystems (4)
PATCH 1/1 fs: O_* bit numbers uniqueness check
The O_* bit numbers are defined in 20+ arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.
--- fs/fcntl.c | 14 ++++++++++++-- include/asm-generic/fcntl.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)
fs/fcntl.c +12 -2
@@ -709,11 +709,21 @@ void kill_fasync(struct fasync_struct ** }EXPORT_SYMBOL (kill_fasync); -static int __init fasync_init (void ) +static int __init fcntl_init (void ) { +/* please add new bits here to ensure allocation uniqueness */ +BUG_ON (20 !=hweight32 ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | +FMODE_EXEC |FMODE_NONOTIFY )); + fasync_cache =kmem_cache_create ("fasync_cache" ,sizeof (struct fasync_struct ),0 ,SLAB_PANIC ,NULL );return 0 ; } -module_init (fasync_init) +module_init (fcntl_init)
include/asm-generic/fcntl.h +2 -0
@@ -4,6 +4,8 @@#include <linux/types.h> /* +* When introducing new O_* bits, please check its uniqueness in fcntl_init(). +* * FMODE_EXEC is 0x20 * FMODE_NONOTIFY is 0x1000000 * These cannot be used by userspace O_* until internal and external open
fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 716
Quoted 4 lines
fs/fcntl.c +3
-0+/* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR |
I wonder if there's a way to make this BUILD_BUG_ON(), so the problem is caught at compile time (a change like adding an O_ flag is often compile tested only for obscure archs). One could create a compile-time macro-ized version of hweight32() and use that, I guess, although I'm not sure it's worth the ugliness.
Failing that maybe this should be WARN_ON()? I'd be annoyed if my arch wouldn't boot because some strange new O_ flag happened to collide.
- R.
Quoted 12 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.CC David Miller <[email protected]>CC Stephen Rothwell <[email protected]>CC Al Viro <[email protected]>CC Christoph Hellwig <[email protected]>CC Eric Paris <[email protected]>Signed-off-by Wu Fengguang <[email protected]>---
{
fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 723
Quoted 10 lines
fs/fcntl.c +10
-0+/* please add new bits here to ensure allocation uniqueness */ +BUG_ON (20 !=hweight32 ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | +FMODE_EXEC |FMODE_NONOTIFY )); +
I cannot test it, but given O_RDONLY is 0, are you sure 20 bits are actually set ? --
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 fs: O_* bit numbers uniqueness check
In reply to Roland DreierQuoted 10 lines · Roland Dreier
Review
Quoted 4 lines
(quoted diff) +3
-0+ /* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR |I wonder if there's a way to make this
BUILD_BUG_ON(), so the problem is caught at compile time (a change like adding an O_ flag is often compile tested only for obscure archs). One could create a compile-time macro-ized version ofhweight32()and use that, I guess, although I'm not sure it's worth the ugliness.
Yeah, I chose the boot time check because of hweight32()..
Quoted 2 lines
Failing that maybe this should be
WARN_ON()? I'd be annoyed if my arch wouldn't boot because some strange new O_ flag happened to collide.
Good suggestion! Here is the updated patch.
Andrew, this patch relies on the newly introduced O_RANDOM and Eric's FMODE_NONOTIFY.
Thanks,
Fengguang
---
fs: O_* bit numbers uniqueness check
The O_* bit numbers are defined in 20+ arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.
v2: change to WARN_ON() as suggested by Roland
--- fs/fcntl.c | 14 ++++++++++++-- include/asm-generic/fcntl.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)
fs/fcntl.c +12 -2
@@ -709,11 +709,21 @@ void kill_fasync(struct fasync_struct ** }EXPORT_SYMBOL (kill_fasync); -static int __init fasync_init (void ) +static int __init fcntl_init (void ) { +/* please add new bits here to ensure allocation uniqueness */ +WARN_ON (20 !=hweight32 ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | +FMODE_EXEC |FMODE_NONOTIFY )); + fasync_cache =kmem_cache_create ("fasync_cache" ,sizeof (struct fasync_struct ),0 ,SLAB_PANIC ,NULL );return 0 ; } -module_init (fasync_init) +module_init (fcntl_init)
include/asm-generic/fcntl.h +2 -0
@@ -4,6 +4,8 @@#include <linux/types.h> /* +* When introducing new O_* bits, please check its uniqueness in fcntl_init(). +* * FMODE_EXEC is 0x20 * FMODE_NONOTIFY is 0x1000000 * These cannot be used by userspace O_* until internal and external open
Quoted 17 lines · Eric Dumazet
Quoted 16 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.CC David Miller <[email protected]>CC Stephen Rothwell <[email protected]>CC Al Viro <[email protected]>CC Christoph Hellwig <[email protected]>CC Eric Paris <[email protected]>Signed-off-by Wu Fengguang <[email protected]>---
{fs/fcntl.c +4
-0+/* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR | + O_CREAT | O_EXCL | O_NOCTTY |
Quoted 8 lines
fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 723
Quoted 6 lines
fs/fcntl.c +6
-0+O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | + FMODE_EXEC | FMODE_NONOTIFY)); +I cannot test it, but given
O_RDONLYis 0, are you sure 20 bits are actually set ?
Yes, I tested it. The tricky one is O_SYNC, which actually has two bits..
Thanks,
Fengguang
Quoted 2 lines
Yeah, I chose the boot time check because of
hweight32()..
One could do something like
#define HWEIGHT32(x) (!!((x) & (1u << 0)) + !!((x) & (1u << 1)) + //... !!((x) & (1u << 31)));
that would probably work with BUILD_BUG_ON().
But as I said maybe it's too ugly.
- R.
Quoted 5 lines · Roland Dreier
Quoted 2 lines
Yeah, I chose the boot time check because of
hweight32()..One could do something like
Quoted 8 lines
#define HWEIGHT32(x) (!!((x) & (1u << 0)) + !!((x) & (1u << 1)) + //... !!((x) & (1u << 31)));that would probably work with
BUILD_BUG_ON().But as I said maybe it's too ugly.
Yes, it'll feel like this. Then you'll have to fix the possible error in order to compile ;)
---
fs: O_* bit numbers uniqueness check
The O_* bit numbers are defined in 20+ arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.
v3: change to BUILD_BUG_ON() as suggested by Roland
--- fs/fcntl.c | 14 ++++++++++++-- include/asm-generic/fcntl.h | 2 ++ include/linux/bitops.h | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-)
fs/fcntl.c +12 -2
@@ -709,11 +709,21 @@ void kill_fasync(struct fasync_struct ** }EXPORT_SYMBOL (kill_fasync); -static int __init fasync_init (void ) +static int __init fcntl_init (void ) { +/* please add new bits here to ensure allocation uniqueness */ +BUILD_BUG_ON (20 !=HWEIGHT32 ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | +FMODE_EXEC |FMODE_NONOTIFY )); + fasync_cache =kmem_cache_create ("fasync_cache" ,sizeof (struct fasync_struct ),0 ,SLAB_PANIC ,NULL );return 0 ; } -module_init (fasync_init) +module_init (fcntl_init)
include/asm-generic/fcntl.h +2 -0
@@ -4,6 +4,8 @@#include <linux/types.h> /* +* When introducing new O_* bits, please check its uniqueness in fcntl_init(). +* * FMODE_EXEC is 0x20 * FMODE_NONOTIFY is 0x1000000 * These cannot be used by userspace O_* until internal and external open
include/linux/bitops.h +33 -0
@@ -8,6 +8,39 @@#define BIT_WORD (nr) ((nr) / BITS_PER_LONG)#define BITS_PER_BYTE 8#define BITS_TO_LONGS (nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) + +#define HWEIGHT32 (x) (!!((x) & (1u << 0)) + \ + !!((x) & (1u << 1)) + \ + !!((x) & (1u << 2)) + \ + !!((x) & (1u << 3)) + \ + !!((x) & (1u << 4)) + \ + !!((x) & (1u << 5)) + \ + !!((x) & (1u << 6)) + \ + !!((x) & (1u << 7)) + \ + !!((x) & (1u << 8)) + \ + !!((x) & (1u << 9)) + \ + !!((x) & (1u << 10)) + \ + !!((x) & (1u << 11)) + \ + !!((x) & (1u << 12)) + \ + !!((x) & (1u << 13)) + \ + !!((x) & (1u << 14)) + \ + !!((x) & (1u << 15)) + \ + !!((x) & (1u << 16)) + \ + !!((x) & (1u << 17)) + \ + !!((x) & (1u << 18)) + \ + !!((x) & (1u << 19)) + \ + !!((x) & (1u << 20)) + \ + !!((x) & (1u << 21)) + \ + !!((x) & (1u << 22)) + \ + !!((x) & (1u << 23)) + \ + !!((x) & (1u << 24)) + \ + !!((x) & (1u << 25)) + \ + !!((x) & (1u << 26)) + \ + !!((x) & (1u << 27)) + \ + !!((x) & (1u << 28)) + \ + !!((x) & (1u << 29)) + \ + !!((x) & (1u << 30)) + \ + !!((x) & (1u << 31)))#endif /*
Quoted 27 lines · Wu Fengguang
Quoted 17 lines · Eric Dumazet
Quoted 16 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.CC David Miller <[email protected]>CC Stephen Rothwell <[email protected]>CC Al Viro <[email protected]>CC Christoph Hellwig <[email protected]>CC Eric Paris <[email protected]>Signed-off-by Wu Fengguang <[email protected]>---
{fs/fcntl.c +4
-0+/* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR | + O_CREAT | O_EXCL | O_NOCTTY |Quoted 8 lines
fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 723
Quoted 6 lines
fs/fcntl.c +6
-0+O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | + FMODE_EXEC | FMODE_NONOTIFY)); +I cannot test it, but given
O_RDONLYis 0, are you sure 20 bits are actually set ?
Quoted 1 line
Yes, I tested it. The tricky one is
O_SYNC, which actually has two bits..
What if a new architecture wants to use a single bit value (since it does not need backwards compatibility)?
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Quoted 3 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.
The right fix is to stop overlyaing the FMODE_ flags into the O_ namespace. Al has been working towards this.
Quoted 10 lines · Roland Dreier
fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 716
Quoted 4 lines
fs/fcntl.c +3
-0+/* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR |I wonder if there's a way to make this
BUILD_BUG_ON(), so the problem is caught at compile time (a change like adding an O_ flag is often compile tested only for obscure archs). One could create a compile-time macro-ized version ofhweight32()and use that, I guess, although I'm not sure it's worth the ugliness.
Not ugly at all:
#define hweight32(x) __builtin_popcount(x)
Checked GCC 3.4.3 / 4.1 / 4.4: It expands as a compile-time constant if the argument is a compile-time constant, so can be used in BUILD_BUG_ON() and even for array sizes etc.
If GCC's __builtin_popcount() isn't good enough for non-constant values, the macro would be more involved:
#define hweight32(x) (__builtin_constant_p(x) ? __builtin_popcount(x) \
: hweight32_nonconstant(x))-- Jamie
Quoted 29 lines · Andreas Schwab
Quoted 27 lines · Wu Fengguang
Quoted 25 lines · Eric Dumazet
Quoted 12 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.CC David Miller <[email protected]>CC Stephen Rothwell <[email protected]>CC Al Viro <[email protected]>CC Christoph Hellwig <[email protected]>CC Eric Paris <[email protected]>Signed-off-by Wu Fengguang <[email protected]>---
{fs/fcntl.c void kill_fasync(struct fasync_struct ** · line 723
Quoted 10 lines
fs/fcntl.c +10
-0+/* please add new bits here to ensure allocation uniqueness */ +BUG_ON (20 !=hweight32 ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | +O_SYNC |FASYNC |O_DIRECT | +O_LARGEFILE |O_DIRECTORY |O_NOFOLLOW | +O_NOATIME |O_CLOEXEC |O_RANDOM | +FMODE_EXEC |FMODE_NONOTIFY )); +I cannot test it, but given
O_RDONLYis 0, are you sure 20 bits are actually set ?
Quoted 4 lines
Quoted 1 line
Yes, I tested it. The tricky one is
O_SYNC, which actually has two bits..What if a new architecture wants to use a single bit value (since it does not need backwards compatibility)?
You mean to test __O_SYNC | O_DSYNC instead of O_SYNC?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 20 lines · Jamie Lokier
Quoted 10 lines · Roland Dreier
Review
Quoted 4 lines
(quoted diff) +3
-0+ /* please add new bits here to ensure allocation uniqueness */ + BUG_ON(20 != hweight32( + O_RDONLY | O_WRONLY | O_RDWR |I wonder if there's a way to make this
BUILD_BUG_ON(), so the problem is caught at compile time (a change like adding an O_ flag is often compile tested only for obscure archs). One could create a compile-time macro-ized version ofhweight32()and use that, I guess, although I'm not sure it's worth the ugliness.Not ugly at all:
#define hweight32(x)
__builtin_popcount(x)Checked GCC 3.4.3 / 4.1 / 4.4: It expands as a compile-time constant if the argument is a compile-time constant, so can be used in
BUILD_BUG_ON()and even for array sizes etc.
Quoted 5 lines
If GCC's
__builtin_popcount()isn't good enough for non-constant values, the macro would be more involved:#define hweight32(x) (__builtin_constant_p(x) ? __builtin_popcount(x) \ : hweight32_nonconstant(x))
Jamie, Thanks for the hint! Would you sign this patch?
---
bitops: compile time optimization for hweight_long(CONSTANT)
This allows use of hweight_long() in BUILD_BUG_ON(). Suggested by Jamie.
--- include/linux/bitops.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
include/linux/bitops.h +8 -4
@@ -40,10 +40,14 @@ static __inline__ int get_count_order(unreturn order; } -static inline unsigned long hweight_long (unsigned long w) -{ -return sizeof (w) ==4 ?hweight32 (w) :hweight64 (w); -} +#define hweight_long (x) \ +( \ + __builtin_constant_p(x) ? \ + __builtin_popcountl(x) : \ + (sizeof(x) <= 4 ? \ + hweight32(x) : \ + hweight64(x)) \ +)/** * rol32 - rotate a 32-bit value left
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Wu Fengguang
The O_* bit numbers are defined in 20+
arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.The right fix is to stop overlyaing the
FMODE_flags into the O_ namespace. Al has been working towards this.
I thinks they are orthogonal problems -- this patch watches for the possible conflicting O_* in so many archs.
Anyway here is the new version without the FMODE_ and O_RANDOM.
Thanks,
Fengguang
---
fs: O_* bit numbers uniqueness check
The O_* bit numbers are defined in 20+ arch/*, and hence can silently overlap. Add a boot time check to ensure the uniqueness as suggested by David Miller.
v4: use the nice hweight_long() as suggested by Jamie
split O_SYNC to (__O_SYNC | O_DSYNC) as suggested by Andreas take away FMODE_* and O_RANDOM
v3: change to BUILD_BUG_ON() as suggested by Roland
--- fs/fcntl.c | 14 ++++++++++++-- include/asm-generic/fcntl.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)
fs/fcntl.c +12 -2
@@ -709,11 +709,21 @@ void kill_fasync(struct fasync_struct ** }EXPORT_SYMBOL (kill_fasync); -static int __init fasync_init (void ) +static int __init fcntl_init (void ) { +/* please add new bits here to ensure allocation uniqueness */ +BUILD_BUG_ON (17 !=hweight_long ( +O_RDONLY |O_WRONLY |O_RDWR | +O_CREAT |O_EXCL |O_NOCTTY | +O_TRUNC |O_APPEND |O_NONBLOCK | + __O_SYNC |O_DSYNC |FASYNC | +O_DIRECT |O_LARGEFILE |O_DIRECTORY | +O_NOFOLLOW |O_NOATIME |O_CLOEXEC + )); + fasync_cache =kmem_cache_create ("fasync_cache" ,sizeof (struct fasync_struct ),0 ,SLAB_PANIC ,NULL );return 0 ; } -module_init (fasync_init) +module_init (fcntl_init)
include/asm-generic/fcntl.h +2 -0
@@ -4,6 +4,8 @@#include <linux/types.h> /* +* When introducing new O_* bits, please check its uniqueness in fcntl_init(). +* * FMODE_EXEC is 0x20 * FMODE_NONOTIFY is 0x1000000 * These cannot be used by userspace O_* until internal and external open