[PATCH] virtio-blk: make the queue depth configurable

[PATCH] virtio-blk: make the queue depth configurable

[Theodore Ts'o]

The current virtio block sets a queue depth of 64.  With a
sufficiently fast device, using a queue depth of 256 can double the
IOPS which can be sustained.  So make the queue depth something which
can be set at module load time or via a kernel boot-time parameter.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtio-dev@lists.oasis-open.org
Cc: virtualization@lists.linux-foundation.org
Cc: fes@google.com
---
 drivers/block/virtio_blk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 6a680d4..0c9e57f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -481,6 +481,9 @@ static struct blk_mq_ops virtio_mq_ops = {
 	.free_hctx	= blk_mq_free_single_hw_queue,
 };
 
+static int queue_depth = 64;
+module_param(queue_depth, int, 444);
+
 static struct blk_mq_reg virtio_mq_reg = {
 	.ops		= &virtio_mq_ops,
 	.nr_hw_queues	= 1,
@@ -551,6 +554,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 		goto out_free_vq;
 	}
 
+	virtio_mq_reg.queue_depth = queue_depth;
 	virtio_mq_reg.cmd_size =
 		sizeof(struct virtblk_req) +
 		sizeof(struct scatterlist) * sg_elems;
-- 
1.9.0

Re: [PATCH] virtio-blk: make the queue depth configurable

[Joe Perches]

On Fri, 2014-03-14 at 13:31 -0400, Theodore Ts'o wrote:
> The current virtio block sets a queue depth of 64.  With a
> sufficiently fast device, using a queue depth of 256 can double the
> IOPS which can be sustained.  So make the queue depth something which
> can be set at module load time or via a kernel boot-time parameter.
[]
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
[]
> @@ -481,6 +481,9 @@ static struct blk_mq_ops virtio_mq_ops = {
>  	.free_hctx	= blk_mq_free_single_hw_queue,
>  };
>  
> +static int queue_depth = 64;
> +module_param(queue_depth, int, 444);

444?  Really Ted?

0444

Re: [PATCH] virtio-blk: make the queue depth configurable

[Theodore Ts'o]

On Fri, Mar 14, 2014 at 10:38:40AM -0700, Joe Perches wrote:
> > +static int queue_depth = 64;
> > +module_param(queue_depth, int, 444);
> 
> 444?  Really Ted?

Oops, *blush*.   Thanks for catching that.

					- Ted

Re: [PATCH] virtio-blk: make the queue depth configurable

[Rusty Russell]

Theodore Ts'o <tytso@mit.edu> writes:
> On Fri, Mar 14, 2014 at 10:38:40AM -0700, Joe Perches wrote:
>> > +static int queue_depth = 64;
>> > +module_param(queue_depth, int, 444);
>> 
>> 444?  Really Ted?
>
> Oops, *blush*.   Thanks for catching that.

Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
now:

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 175f6995d1af..626b85888a6b 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -188,6 +188,9 @@ struct kparam_array
 	/* Default value instead of permissions? */			\
 	static int __param_perm_check_##name __attribute__((unused)) =	\
 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
+	/* User perms >= group perms >= other perms. */			\
+	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
+	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))		\
 	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\

Re: [PATCH] virtio-blk: make the queue depth configurable

[Joe Perches]

On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:

> Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
> now:

Good idea.

> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
[]
> @@ -188,6 +188,9 @@ struct kparam_array
>  	/* Default value instead of permissions? */			\
>  	static int __param_perm_check_##name __attribute__((unused)) =	\
>  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
> +	/* User perms >= group perms >= other perms. */			\
> +	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
> +	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))		\
>  	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
>  	static const char __param_str_##name[] = prefix #name;		\
>  	static struct kernel_param __moduleparam_const __param_##name	\

It might make sense to separate this octal permissions
test into a new macro for other checks in macros like
CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.

Maybe something like:

#define VERIFY_OCTAL_PERMISSIONS(perm)					\
	static int __param_perm_check_##name __attribute__((unused)) =	\
	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
	/* User perms >= group perms >= other perms. */			\
	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7));	\

Re: [PATCH] virtio-blk: make the queue depth configurable

[Joe Perches]

On Sun, 2014-03-16 at 22:00 -0700, Joe Perches wrote:
> On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:
> 
> > Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
> > now:
> 
> Good idea.
> 
> > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> []
> > @@ -188,6 +188,9 @@ struct kparam_array
> >  	/* Default value instead of permissions? */			\
> >  	static int __param_perm_check_##name __attribute__((unused)) =	\
> >  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
> > +	/* User perms >= group perms >= other perms. */			\
> > +	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
> > +	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))		\
> >  	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
> >  	static const char __param_str_##name[] = prefix #name;		\
> >  	static struct kernel_param __moduleparam_const __param_##name	\
> 
> It might make sense to separate this octal permissions
> test into a new macro for other checks in macros like
> CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.
> 
> Maybe something like:
> 
> #define VERIFY_OCTAL_PERMISSIONS(perm)					\
> 	static int __param_perm_check_##name __attribute__((unused)) =	\
> 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
> 	/* User perms >= group perms >= other perms. */			\
> 	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
> 	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7));	\

Maybe this is better:

#define VERIFY_OCTAL_PERMISSIONS(perms)					\
({									\
	if (__builtin_constant_p(perms)) {				\
		BUILD_BUG_ON((perms) < 0);				\
		BUILD_BUG_ON((perms) > 0777);				\
		/* User perms >= group perms >= other perms */		\
		BUILD_BUG_ON(((perms) >> 6) < (((perms) >> 3) & 7));	\
		BUILD_BUG_ON((((perms) >> 3) & 7) < ((perms) & 7));	\
	}								\
	;								\
})

Re: [PATCH] virtio-blk: make the queue depth configurable

[Rusty Russell]

Joe Perches <joe@perches.com> writes:
> On Sun, 2014-03-16 at 22:00 -0700, Joe Perches wrote:
>> On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:
>> 
>> > Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
>> > now:
>> 
>> Good idea.
>> 
>> > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> []
>> > @@ -188,6 +188,9 @@ struct kparam_array
>> >  	/* Default value instead of permissions? */			\
>> >  	static int __param_perm_check_##name __attribute__((unused)) =	\
>> >  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
>> > +	/* User perms >= group perms >= other perms. */			\
>> > +	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
>> > +	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))		\
>> >  	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
>> >  	static const char __param_str_##name[] = prefix #name;		\
>> >  	static struct kernel_param __moduleparam_const __param_##name	\
>> 
>> It might make sense to separate this octal permissions
>> test into a new macro for other checks in macros like
>> CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.

OK, I took your bikeshed and re-painted it below.

> #define VERIFY_OCTAL_PERMISSIONS(perms)					\
> ({									\
> 	if (__builtin_constant_p(perms)) {				\
> 		BUILD_BUG_ON((perms) < 0);				\
> 		BUILD_BUG_ON((perms) > 0777);				\
> 		/* User perms >= group perms >= other perms */		\
> 		BUILD_BUG_ON(((perms) >> 6) < (((perms) >> 3) & 7));	\
> 		BUILD_BUG_ON((((perms) >> 3) & 7) < ((perms) & 7));	\
> 	}								\
> 	;								\
> })

Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.

Summary of http://lkml.org/lkml/2014/3/14/363 :

  Ted: module_param(queue_depth, int, 444)
  Joe: 0444!
  Rusty: User perms >= group perms >= other perms?
  Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?

Side effect of stricter permissions means removing the unnecessary
S_IFREG from drivers/pci/slot.c.

Suggested-by: Joe Perches <joe@perches.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7dd62fa9d0bd..396c200b9ddb 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -116,11 +116,11 @@ static void pci_slot_release(struct kobject *kobj)
 }
 
 static struct pci_slot_attribute pci_slot_attr_address =
-	__ATTR(address, (S_IFREG | S_IRUGO), address_read_file, NULL);
+	__ATTR(address, S_IRUGO, address_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_max_speed =
-	__ATTR(max_bus_speed, (S_IFREG | S_IRUGO), max_speed_read_file, NULL);
+	__ATTR(max_bus_speed, S_IRUGO, max_speed_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_cur_speed =
-	__ATTR(cur_bus_speed, (S_IFREG | S_IRUGO), cur_speed_read_file, NULL);
+	__ATTR(cur_bus_speed, S_IRUGO, cur_speed_read_file, NULL);
 
 static struct attribute *pci_slot_default_attrs[] = {
 	&pci_slot_attr_address.attr,
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 471090093c67..945afeb3058a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -842,4 +842,13 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
 #endif
 
+/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
+#define VERIFY_OCTAL_PERMISSIONS(perms)					\
+	((__builtin_constant_p(perms) ?					\
+	  BUILD_BUG_ON_ZERO((perms) < 0) +				\
+	  BUILD_BUG_ON_ZERO((perms) > 0777) +				\
+	  /* User perms >= group perms >= other perms */		\
+	  BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
+	  BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)): 0) +	\
+	 (perms))
 #endif
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 175f6995d1af..204a67743804 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -186,14 +186,12 @@ struct kparam_array
    parameters. */
 #define __module_param_call(prefix, name, ops, arg, perm, level)	\
 	/* Default value instead of permissions? */			\
-	static int __param_perm_check_##name __attribute__((unused)) =	\
-	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
-	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
-	static const char __param_str_##name[] = prefix #name;		\
+	static const char __param_str_##name[] = prefix #name; \
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, ops, perm, level, { arg } }
+	= { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),	\
+	    level, { arg } }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 30b2ebee6439..f517e6e488c8 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -71,7 +71,8 @@ struct attribute_group {
  */
 
 #define __ATTR(_name, _mode, _show, _store) {				\
-	.attr = {.name = __stringify(_name), .mode = _mode },		\
+	.attr = {.name = __stringify(_name),				\
+		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
 	.show	= _show,						\
 	.store	= _store,						\
 }

Re: [PATCH] virtio-blk: make the queue depth configurable

[Joe Perches]

On Wed, 2014-03-19 at 17:07 +1030, Rusty Russell wrote:

> Summary of http://lkml.org/lkml/2014/3/14/363 :
> 
>   Ted: module_param(queue_depth, int, 444)
>   Joe: 0444!
>   Rusty: User perms >= group perms >= other perms?
>   Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?

<smile>

Adding:

Rusty: Bikeshedding
Joe: Missing ((perm) & 2) ?

[]

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 471090093c67..945afeb3058a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -842,4 +842,13 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
>  #endif
>  
> +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
> +#define VERIFY_OCTAL_PERMISSIONS(perms)					\
> +	((__builtin_constant_p(perms) ?					\
> +	  BUILD_BUG_ON_ZERO((perms) < 0) +				\
> +	  BUILD_BUG_ON_ZERO((perms) > 0777) +				\
> +	  /* User perms >= group perms >= other perms */		\
> +	  BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
> +	  BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)): 0) +	\
> +	 (perms))
>  #endif
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 175f6995d1af..204a67743804 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -186,14 +186,12 @@ struct kparam_array
>     parameters. */
>  #define __module_param_call(prefix, name, ops, arg, perm, level)	\
>  	/* Default value instead of permissions? */			\
> -	static int __param_perm_check_##name __attribute__((unused)) =	\
> -	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\

Now missing test for ((perm) & 2)

Dunno if that was ever necessary.

> -	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
> -	static const char __param_str_##name[] = prefix #name;		\
> +	static const char __param_str_##name[] = prefix #name; \
>  	static struct kernel_param __moduleparam_const __param_##name	\
>  	__used								\
>      __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
> -	= { __param_str_##name, ops, perm, level, { arg } }
> +	= { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),	\
> +	    level, { arg } }

Re: [PATCH] virtio-blk: make the queue depth configurable

[Greg Kroah-Hartman]

On Wed, Mar 19, 2014 at 05:07:50PM +1030, Rusty Russell wrote:
> Joe Perches <joe@perches.com> writes:
> > On Sun, 2014-03-16 at 22:00 -0700, Joe Perches wrote:
> >> On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:
> >> 
> >> > Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
> >> > now:
> >> 
> >> Good idea.
> >> 
> >> > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> >> []
> >> > @@ -188,6 +188,9 @@ struct kparam_array
> >> >  	/* Default value instead of permissions? */			\
> >> >  	static int __param_perm_check_##name __attribute__((unused)) =	\
> >> >  	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
> >> > +	/* User perms >= group perms >= other perms. */			\
> >> > +	+ BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))	\
> >> > +	+ BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))		\
> >> >  	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
> >> >  	static const char __param_str_##name[] = prefix #name;		\
> >> >  	static struct kernel_param __moduleparam_const __param_##name	\
> >> 
> >> It might make sense to separate this octal permissions
> >> test into a new macro for other checks in macros like
> >> CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.
> 
> OK, I took your bikeshed and re-painted it below.
> 
> > #define VERIFY_OCTAL_PERMISSIONS(perms)					\
> > ({									\
> > 	if (__builtin_constant_p(perms)) {				\
> > 		BUILD_BUG_ON((perms) < 0);				\
> > 		BUILD_BUG_ON((perms) > 0777);				\
> > 		/* User perms >= group perms >= other perms */		\
> > 		BUILD_BUG_ON(((perms) >> 6) < (((perms) >> 3) & 7));	\
> > 		BUILD_BUG_ON((((perms) >> 3) & 7) < ((perms) & 7));	\
> > 	}								\
> > 	;								\
> > })
> 
> Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.
> 
> Summary of http://lkml.org/lkml/2014/3/14/363 :
> 
>   Ted: module_param(queue_depth, int, 444)
>   Joe: 0444!
>   Rusty: User perms >= group perms >= other perms?
>   Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?
> 
> Side effect of stricter permissions means removing the unnecessary
> S_IFREG from drivers/pci/slot.c.
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] virtio-blk: make the queue depth configurable

[Bjorn Helgaas]

On Wed, Mar 19, 2014 at 12:37 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Joe Perches <joe@perches.com> writes:
>> On Sun, 2014-03-16 at 22:00 -0700, Joe Perches wrote:
>>> On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:
>>>
>>> > Erk, our tests are insufficient.  Testbuilding an allmodconfig with this
>>> > now:
>>>
>>> Good idea.
>>>
>>> > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>>> []
>>> > @@ -188,6 +188,9 @@ struct kparam_array
>>> >    /* Default value instead of permissions? */                     \
>>> >    static int __param_perm_check_##name __attribute__((unused)) =  \
>>> >    BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))  \
>>> > +  /* User perms >= group perms >= other perms. */                 \
>>> > +  + BUILD_BUG_ON_ZERO(((perm) >> 6) < (((perm) >> 3) & 7))        \
>>> > +  + BUILD_BUG_ON_ZERO((((perm) >> 3) & 7) < ((perm) & 7))         \
>>> >    + BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);   \
>>> >    static const char __param_str_##name[] = prefix #name;          \
>>> >    static struct kernel_param __moduleparam_const __param_##name   \
>>>
>>> It might make sense to separate this octal permissions
>>> test into a new macro for other checks in macros like
>>> CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.
>
> OK, I took your bikeshed and re-painted it below.
>
>> #define VERIFY_OCTAL_PERMISSIONS(perms)                                       \
>> ({                                                                    \
>>       if (__builtin_constant_p(perms)) {                              \
>>               BUILD_BUG_ON((perms) < 0);                              \
>>               BUILD_BUG_ON((perms) > 0777);                           \
>>               /* User perms >= group perms >= other perms */          \
>>               BUILD_BUG_ON(((perms) >> 6) < (((perms) >> 3) & 7));    \
>>               BUILD_BUG_ON((((perms) >> 3) & 7) < ((perms) & 7));     \
>>       }                                                               \
>>       ;                                                               \
>> })
>
> Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.
>
> Summary of http://lkml.org/lkml/2014/3/14/363 :
>
>   Ted: module_param(queue_depth, int, 444)
>   Joe: 0444!
>   Rusty: User perms >= group perms >= other perms?
>   Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?
>
> Side effect of stricter permissions means removing the unnecessary
> S_IFREG from drivers/pci/slot.c.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Bjorn Helgaas <bhelgaas@google.com> for drivers/pci/slot.c

It looks like fs/ocfs2/cluster/sys.c and fs/ocfs2/stackglue.c also use
S_IFREG in a similar way.

> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 7dd62fa9d0bd..396c200b9ddb 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -116,11 +116,11 @@ static void pci_slot_release(struct kobject *kobj)
>  }
>
>  static struct pci_slot_attribute pci_slot_attr_address =
> -       __ATTR(address, (S_IFREG | S_IRUGO), address_read_file, NULL);
> +       __ATTR(address, S_IRUGO, address_read_file, NULL);
>  static struct pci_slot_attribute pci_slot_attr_max_speed =
> -       __ATTR(max_bus_speed, (S_IFREG | S_IRUGO), max_speed_read_file, NULL);
> +       __ATTR(max_bus_speed, S_IRUGO, max_speed_read_file, NULL);
>  static struct pci_slot_attribute pci_slot_attr_cur_speed =
> -       __ATTR(cur_bus_speed, (S_IFREG | S_IRUGO), cur_speed_read_file, NULL);
> +       __ATTR(cur_bus_speed, S_IRUGO, cur_speed_read_file, NULL);
>
>  static struct attribute *pci_slot_default_attrs[] = {
>         &pci_slot_attr_address.attr,
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 471090093c67..945afeb3058a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -842,4 +842,13 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
>  #endif
>
> +/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
> +#define VERIFY_OCTAL_PERMISSIONS(perms)                                        \
> +       ((__builtin_constant_p(perms) ?                                 \
> +         BUILD_BUG_ON_ZERO((perms) < 0) +                              \
> +         BUILD_BUG_ON_ZERO((perms) > 0777) +                           \
> +         /* User perms >= group perms >= other perms */                \
> +         BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +    \
> +         BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)): 0) + \
> +        (perms))
>  #endif
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 175f6995d1af..204a67743804 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -186,14 +186,12 @@ struct kparam_array
>     parameters. */
>  #define __module_param_call(prefix, name, ops, arg, perm, level)       \
>         /* Default value instead of permissions? */                     \
> -       static int __param_perm_check_##name __attribute__((unused)) =  \
> -       BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))  \
> -       + BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);   \
> -       static const char __param_str_##name[] = prefix #name;          \
> +       static const char __param_str_##name[] = prefix #name; \
>         static struct kernel_param __moduleparam_const __param_##name   \
>         __used                                                          \
>      __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
> -       = { __param_str_##name, ops, perm, level, { arg } }
> +       = { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),    \
> +           level, { arg } }
>
>  /* Obsolete - use module_param_cb() */
>  #define module_param_call(name, set, get, arg, perm)                   \
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 30b2ebee6439..f517e6e488c8 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -71,7 +71,8 @@ struct attribute_group {
>   */
>
>  #define __ATTR(_name, _mode, _show, _store) {                          \
> -       .attr = {.name = __stringify(_name), .mode = _mode },           \
> +       .attr = {.name = __stringify(_name),                            \
> +                .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },             \
>         .show   = _show,                                                \
>         .store  = _store,                                               \
>  }

Re: [PATCH] virtio-blk: make the queue depth configurable

[Joe Perches]

Couple more bikesheddy things:

Is there ever a reason to use a non __builtin_const_p(perms)?

Maybe that should be a BUILD_BUG_ON too
	BUILD_BUG_ON(!builtin_const_p_perms)

My brain of little size gets confused by the
	BUILD_BUG_ON_ZERO(foo) +
vs
	BUILD_BUG_ON(foo);
as it just seems like more text for the same content.

Is there any value on the "_ZERO(foo) +" I don't understand?

Re: [PATCH] virtio-blk: make the queue depth configurable

[Rusty Russell]

Bjorn Helgaas <bhelgaas@google.com> writes:
> On Wed, Mar 19, 2014 at 12:37 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Side effect of stricter permissions means removing the unnecessary
>> S_IFREG from drivers/pci/slot.c.
>>
>> Suggested-by: Joe Perches <joe@perches.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com> for drivers/pci/slot.c
>
> It looks like fs/ocfs2/cluster/sys.c and fs/ocfs2/stackglue.c also use
> S_IFREG in a similar way.

Thanks, allmodconfig picked that up, and some others.  See below.

Cheers,
Rusty.

Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.

Summary of http://lkml.org/lkml/2014/3/14/363 :

  Ted: module_param(queue_depth, int, 444)
  Joe: 0444!
  Rusty: User perms >= group perms >= other perms?
  Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?

Side effect of stricter permissions means removing the unnecessary
S_IFREG from several callers.

Suggested-by: Joe Perches <joe@perches.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> for drivers/pci/slot.c
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7dd62fa9d0bd..396c200b9ddb 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -116,11 +116,11 @@ static void pci_slot_release(struct kobject *kobj)
 }
 
 static struct pci_slot_attribute pci_slot_attr_address =
-	__ATTR(address, (S_IFREG | S_IRUGO), address_read_file, NULL);
+	__ATTR(address, S_IRUGO, address_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_max_speed =
-	__ATTR(max_bus_speed, (S_IFREG | S_IRUGO), max_speed_read_file, NULL);
+	__ATTR(max_bus_speed, S_IRUGO, max_speed_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_cur_speed =
-	__ATTR(cur_bus_speed, (S_IFREG | S_IRUGO), cur_speed_read_file, NULL);
+	__ATTR(cur_bus_speed, S_IRUGO, cur_speed_read_file, NULL);
 
 static struct attribute *pci_slot_default_attrs[] = {
 	&pci_slot_attr_address.attr,
diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index b96a49b37d66..77cf5eeeabd1 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -568,7 +568,7 @@ static ssize_t cuse_class_waiting_show(struct device *dev,
 
 	return sprintf(buf, "%d\n", atomic_read(&cc->fc.num_waiting));
 }
-static DEVICE_ATTR(waiting, S_IFREG | 0400, cuse_class_waiting_show, NULL);
+static DEVICE_ATTR(waiting, 0400, cuse_class_waiting_show, NULL);
 
 static ssize_t cuse_class_abort_store(struct device *dev,
 				      struct device_attribute *attr,
@@ -579,7 +579,7 @@ static ssize_t cuse_class_abort_store(struct device *dev,
 	fuse_abort_conn(&cc->fc);
 	return count;
 }
-static DEVICE_ATTR(abort, S_IFREG | 0200, NULL, cuse_class_abort_store);
+static DEVICE_ATTR(abort, 0200, NULL, cuse_class_abort_store);
 
 static struct attribute *cuse_class_dev_attrs[] = {
 	&dev_attr_waiting.attr,
diff --git a/fs/ocfs2/cluster/sys.c b/fs/ocfs2/cluster/sys.c
index a4b07730b2e1..b7f57271d49c 100644
--- a/fs/ocfs2/cluster/sys.c
+++ b/fs/ocfs2/cluster/sys.c
@@ -41,7 +41,7 @@ static ssize_t version_show(struct kobject *kobj, struct kobj_attribute *attr,
 	return snprintf(buf, PAGE_SIZE, "%u\n", O2NM_API_VERSION);
 }
 static struct kobj_attribute attr_version =
-	__ATTR(interface_revision, S_IFREG | S_IRUGO, version_show, NULL);
+	__ATTR(interface_revision, S_IRUGO, version_show, NULL);
 
 static struct attribute *o2cb_attrs[] = {
 	&attr_version.attr,
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index 1324e6600e57..25e9f7b5bad3 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -494,7 +494,7 @@ static ssize_t ocfs2_max_locking_protocol_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_max_locking_protocol =
-	__ATTR(max_locking_protocol, S_IFREG | S_IRUGO,
+	__ATTR(max_locking_protocol, S_IRUGO,
 	       ocfs2_max_locking_protocol_show, NULL);
 
 static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj,
@@ -526,7 +526,7 @@ static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_loaded_cluster_plugins =
-	__ATTR(loaded_cluster_plugins, S_IFREG | S_IRUGO,
+	__ATTR(loaded_cluster_plugins, S_IRUGO,
 	       ocfs2_loaded_cluster_plugins_show, NULL);
 
 static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj,
@@ -548,7 +548,7 @@ static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_active_cluster_plugin =
-	__ATTR(active_cluster_plugin, S_IFREG | S_IRUGO,
+	__ATTR(active_cluster_plugin, S_IRUGO,
 	       ocfs2_active_cluster_plugin_show, NULL);
 
 static ssize_t ocfs2_cluster_stack_show(struct kobject *kobj,
@@ -597,7 +597,7 @@ static ssize_t ocfs2_cluster_stack_store(struct kobject *kobj,
 
 
 static struct kobj_attribute ocfs2_attr_cluster_stack =
-	__ATTR(cluster_stack, S_IFREG | S_IRUGO | S_IWUSR,
+	__ATTR(cluster_stack, S_IRUGO | S_IWUSR,
 	       ocfs2_cluster_stack_show,
 	       ocfs2_cluster_stack_store);
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 471090093c67..945afeb3058a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -842,4 +842,13 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
 #endif
 
+/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
+#define VERIFY_OCTAL_PERMISSIONS(perms)					\
+	((__builtin_constant_p(perms) ?					\
+	  BUILD_BUG_ON_ZERO((perms) < 0) +				\
+	  BUILD_BUG_ON_ZERO((perms) > 0777) +				\
+	  /* User perms >= group perms >= other perms */		\
+	  BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
+	  BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)): 0) +	\
+	 (perms))
 #endif
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 175f6995d1af..204a67743804 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -186,14 +186,12 @@ struct kparam_array
    parameters. */
 #define __module_param_call(prefix, name, ops, arg, perm, level)	\
 	/* Default value instead of permissions? */			\
-	static int __param_perm_check_##name __attribute__((unused)) =	\
-	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
-	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
-	static const char __param_str_##name[] = prefix #name;		\
+	static const char __param_str_##name[] = prefix #name; \
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, ops, perm, level, { arg } }
+	= { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),	\
+	    level, { arg } }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 30b2ebee6439..f517e6e488c8 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -71,7 +71,8 @@ struct attribute_group {
  */
 
 #define __ATTR(_name, _mode, _show, _store) {				\
-	.attr = {.name = __stringify(_name), .mode = _mode },		\
+	.attr = {.name = __stringify(_name),				\
+		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
 	.show	= _show,						\
 	.store	= _store,						\
 }

Re: [PATCH] virtio-blk: make the queue depth configurable

[Rusty Russell]

Joe Perches <joe@perches.com> writes:
> Couple more bikesheddy things:
>
> Is there ever a reason to use a non __builtin_const_p(perms)?

It's a bit conservative, and anyway, the test is useless since AFAICT
BUILD_BUG_ON() is a noop if !__builtin_const_p().  I removed it
and re-tested.

> Maybe that should be a BUILD_BUG_ON too
> 	BUILD_BUG_ON(!builtin_const_p_perms)
>
> My brain of little size gets confused by the
> 	BUILD_BUG_ON_ZERO(foo) +
> vs
> 	BUILD_BUG_ON(foo);
> as it just seems like more text for the same content.
>
> Is there any value on the "_ZERO(foo) +" I don't understand?

I don't like to use statement expressions unless we really need them,
and BUILD_BUG_ON_ZERO() was build for this purpose.

Cheers,
Rusty.

Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.

Summary of http://lkml.org/lkml/2014/3/14/363 :

  Ted: module_param(queue_depth, int, 444)
  Joe: 0444!
  Rusty: User perms >= group perms >= other perms?
  Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?

Side effect of stricter permissions means removing the unnecessary
S_IFREG from several callers.

Suggested-by: Joe Perches <joe@perches.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> for drivers/pci/slot.c
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7dd62fa9d0bd..396c200b9ddb 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -116,11 +116,11 @@ static void pci_slot_release(struct kobject *kobj)
 }
 
 static struct pci_slot_attribute pci_slot_attr_address =
-	__ATTR(address, (S_IFREG | S_IRUGO), address_read_file, NULL);
+	__ATTR(address, S_IRUGO, address_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_max_speed =
-	__ATTR(max_bus_speed, (S_IFREG | S_IRUGO), max_speed_read_file, NULL);
+	__ATTR(max_bus_speed, S_IRUGO, max_speed_read_file, NULL);
 static struct pci_slot_attribute pci_slot_attr_cur_speed =
-	__ATTR(cur_bus_speed, (S_IFREG | S_IRUGO), cur_speed_read_file, NULL);
+	__ATTR(cur_bus_speed, S_IRUGO, cur_speed_read_file, NULL);
 
 static struct attribute *pci_slot_default_attrs[] = {
 	&pci_slot_attr_address.attr,
diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index b96a49b37d66..77cf5eeeabd1 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -568,7 +568,7 @@ static ssize_t cuse_class_waiting_show(struct device *dev,
 
 	return sprintf(buf, "%d\n", atomic_read(&cc->fc.num_waiting));
 }
-static DEVICE_ATTR(waiting, S_IFREG | 0400, cuse_class_waiting_show, NULL);
+static DEVICE_ATTR(waiting, 0400, cuse_class_waiting_show, NULL);
 
 static ssize_t cuse_class_abort_store(struct device *dev,
 				      struct device_attribute *attr,
@@ -579,7 +579,7 @@ static ssize_t cuse_class_abort_store(struct device *dev,
 	fuse_abort_conn(&cc->fc);
 	return count;
 }
-static DEVICE_ATTR(abort, S_IFREG | 0200, NULL, cuse_class_abort_store);
+static DEVICE_ATTR(abort, 0200, NULL, cuse_class_abort_store);
 
 static struct attribute *cuse_class_dev_attrs[] = {
 	&dev_attr_waiting.attr,
diff --git a/fs/ocfs2/cluster/sys.c b/fs/ocfs2/cluster/sys.c
index a4b07730b2e1..b7f57271d49c 100644
--- a/fs/ocfs2/cluster/sys.c
+++ b/fs/ocfs2/cluster/sys.c
@@ -41,7 +41,7 @@ static ssize_t version_show(struct kobject *kobj, struct kobj_attribute *attr,
 	return snprintf(buf, PAGE_SIZE, "%u\n", O2NM_API_VERSION);
 }
 static struct kobj_attribute attr_version =
-	__ATTR(interface_revision, S_IFREG | S_IRUGO, version_show, NULL);
+	__ATTR(interface_revision, S_IRUGO, version_show, NULL);
 
 static struct attribute *o2cb_attrs[] = {
 	&attr_version.attr,
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index 1324e6600e57..25e9f7b5bad3 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -494,7 +494,7 @@ static ssize_t ocfs2_max_locking_protocol_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_max_locking_protocol =
-	__ATTR(max_locking_protocol, S_IFREG | S_IRUGO,
+	__ATTR(max_locking_protocol, S_IRUGO,
 	       ocfs2_max_locking_protocol_show, NULL);
 
 static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj,
@@ -526,7 +526,7 @@ static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_loaded_cluster_plugins =
-	__ATTR(loaded_cluster_plugins, S_IFREG | S_IRUGO,
+	__ATTR(loaded_cluster_plugins, S_IRUGO,
 	       ocfs2_loaded_cluster_plugins_show, NULL);
 
 static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj,
@@ -548,7 +548,7 @@ static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj,
 }
 
 static struct kobj_attribute ocfs2_attr_active_cluster_plugin =
-	__ATTR(active_cluster_plugin, S_IFREG | S_IRUGO,
+	__ATTR(active_cluster_plugin, S_IRUGO,
 	       ocfs2_active_cluster_plugin_show, NULL);
 
 static ssize_t ocfs2_cluster_stack_show(struct kobject *kobj,
@@ -597,7 +597,7 @@ static ssize_t ocfs2_cluster_stack_store(struct kobject *kobj,
 
 
 static struct kobj_attribute ocfs2_attr_cluster_stack =
-	__ATTR(cluster_stack, S_IFREG | S_IRUGO | S_IWUSR,
+	__ATTR(cluster_stack, S_IRUGO | S_IWUSR,
 	       ocfs2_cluster_stack_show,
 	       ocfs2_cluster_stack_store);
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 471090093c67..4679eddc110a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -842,4 +842,12 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
 #endif
 
+/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
+#define VERIFY_OCTAL_PERMISSIONS(perms)					\
+	(BUILD_BUG_ON_ZERO((perms) < 0) +				\
+	 BUILD_BUG_ON_ZERO((perms) > 0777) +				\
+	 /* User perms >= group perms >= other perms */			\
+	 BUILD_BUG_ON_ZERO(((perms) >> 6) < (((perms) >> 3) & 7)) +	\
+	 BUILD_BUG_ON_ZERO((((perms) >> 3) & 7) < ((perms) & 7)) +	\
+	 (perms))
 #endif
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 175f6995d1af..204a67743804 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -186,14 +186,12 @@ struct kparam_array
    parameters. */
 #define __module_param_call(prefix, name, ops, arg, perm, level)	\
 	/* Default value instead of permissions? */			\
-	static int __param_perm_check_##name __attribute__((unused)) =	\
-	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
-	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
-	static const char __param_str_##name[] = prefix #name;		\
+	static const char __param_str_##name[] = prefix #name; \
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, ops, perm, level, { arg } }
+	= { __param_str_##name, ops, VERIFY_OCTAL_PERMISSIONS(perm),	\
+	    level, { arg } }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm)			\
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 30b2ebee6439..f517e6e488c8 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -71,7 +71,8 @@ struct attribute_group {
  */
 
 #define __ATTR(_name, _mode, _show, _store) {				\
-	.attr = {.name = __stringify(_name), .mode = _mode },		\
+	.attr = {.name = __stringify(_name),				\
+		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
 	.show	= _show,						\
 	.store	= _store,						\
 }