[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

[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

[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

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

[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

[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

[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

[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

[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

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

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,						\
 }

Stricter module param and sysfs permission checks

[Rusty Russell]

CC's trimmed, this is not a virtio issue.

Joe Perches <joe@perches.com> writes:
> On Wed, 2014-03-19 at 17:07 +1030, Rusty Russell wrote:
>>   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?
...
>>  	/* 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.

Yeah, that was introduced by Alexey Dobriyan in 2006.  It didn't go
through me, though :(  New check is better.

allmodconfig says this breaks some things:

drivers/mtd/devices/docg3.c:
 	__ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, dps0_insert_key), \
	__ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), \

drivers/regulator/virtual.c:
static DEVICE_ATTR(min_microvolts, 0666, show_min_uV, set_min_uV);
static DEVICE_ATTR(max_microvolts, 0666, show_max_uV, set_max_uV);
static DEVICE_ATTR(min_microamps, 0666, show_min_uA, set_min_uA);
static DEVICE_ATTR(max_microamps, 0666, show_max_uA, set_max_uA);
static DEVICE_ATTR(mode, 0666, show_mode, set_mode);

drivers/hid/hid-lg4ff.c:
static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IRWXO, lg4ff_range_show, lg4ff_range_store);

drivers/scsi/pm8001/pm8001_ctl.c:
static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO,
	pm8001_show_update_fw, pm8001_store_update_fw);

... plus some staging.

So I've left that for a future patch.

Thanks,
Rusty.

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,						\
 }

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,						\
 }

Re: Stricter module param and sysfs permission checks

[Dave Jones]

On Thu, Mar 20, 2014 at 01:43:44PM +1030, Rusty Russell wrote:

 > drivers/mtd/devices/docg3.c:
 >  	__ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, dps0_insert_key), \
 > 	__ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), \
 > 
 > drivers/scsi/pm8001/pm8001_ctl.c:
 > static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO,
 > 	pm8001_show_update_fw, pm8001_store_update_fw);

Why on earth are these world writable ?

	Dave
 

Re: Stricter module param and sysfs permission checks

[Alexey Dobriyan]

On Thu, Mar 20, 2014 at 7:27 AM, Dave Jones <davej@redhat.com> wrote:
> On Thu, Mar 20, 2014 at 01:43:44PM +1030, Rusty Russell wrote:
>
>  > drivers/mtd/devices/docg3.c:
>  >      __ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, dps0_insert_key), \
>  >      __ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), \
>  >
>  > drivers/scsi/pm8001/pm8001_ctl.c:
>  > static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO,
>  >      pm8001_show_update_fw, pm8001_store_update_fw);
>
> Why on earth are these world writable ?

Yup.

I like new and improved checks.
Rusty, add my "& 2" check everywhere!

Re: Stricter module param and sysfs permission checks

[Robert Jarzmik]

Dave Jones <davej@redhat.com> writes:

> On Thu, Mar 20, 2014 at 01:43:44PM +1030, Rusty Russell wrote:
>
>  > drivers/mtd/devices/docg3.c:
>  >  	__ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, dps0_insert_key), \
>  > 	__ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), \
>  > 
>  > drivers/scsi/pm8001/pm8001_ctl.c:
>  > static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO,
>  > 	pm8001_show_update_fw, pm8001_store_update_fw);
>
> Why on earth are these world writable ?
For docg3, this attributes are used to input a "password" into the flash chip,
to unlock parts of the flash memory. By unlock I mean that a sector read will
return the actual sector when unlocked, and only 0xff if not read unlocked.

As to the "why writable" by "others", the legacy reason is that when I wrote
that code I had in mind that a casual user count :
 - input the code : "echo secret > dps0_protection_key"
 - mount /usermount

That's not a good reason, I know, and changing that to remove the "other" write
permission is fine by me.

Cheers.

-- 
Robert

Re: Stricter module param and sysfs permission checks

[Gobinda Charan Maji]

Robert Jarzmik <robert.jarzmik <at> free.fr> writes:

> 
> Dave Jones <davej <at> redhat.com> writes:
> 
> > On Thu, Mar 20, 2014 at 01:43:44PM +1030, Rusty Russell wrote:
> >
> >  > drivers/mtd/devices/docg3.c:
> >  >  	__ATTR(f##id##_dps0_protection_key, S_IWUGO, NULL, 
dps0_insert_key), \
> >  > 	__ATTR(f##id##_dps1_protection_key, S_IWUGO, NULL, dps1_insert_key), 
\
> >  > 
> >  > drivers/scsi/pm8001/pm8001_ctl.c:
> >  > static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUGO,
> >  > 	pm8001_show_update_fw, pm8001_store_update_fw);
> >
> > Why on earth are these world writable ?
> For docg3, this attributes are used to input a "password" into the flash 
chip,
> to unlock parts of the flash memory. By unlock I mean that a sector read 
will
> return the actual sector when unlocked, and only 0xff if not read 
unlocked.
> 
> As to the "why writable" by "others", the legacy reason is that when I 
wrote
> that code I had in mind that a casual user count :
>  - input the code : "echo secret > dps0_protection_key"
>  - mount /usermount
> 
> That's not a good reason, I know, and changing that to remove the "other" 
write
> permission is fine by me.
> 
> Cheers.
> 

Hi All,

As per the newly added restriction (User perms >= group perms >= other 
perms) is concerned, there is an inconsistency in the permission. Say for 
example, permission value is "0432". Here User has only READ permission 
whereas Group has both WRITE and EXECUTE permission and Other has WRITE 
permission. I think it is not good to give Group and Other at least WRITE 
permission whereas User itself has no WRITE permission.

May be, it's better to check those three permissions bit wise rather than as 
a whole. Please rethink about my point and let me know your opinion.

Thanks,
Gobinda

Re: Stricter module param and sysfs permission checks

[Gobinda Charan Maji]

Gobinda Charan Maji <gobinda.cemk07 <at> gmail.com> writes:



> 
> Hi All,
> 
> As per the newly added restriction (User perms >= group perms >= other 
> perms) is concerned, there is an inconsistency in the permission. Say for 
> example, permission value is "0432". Here User has only READ permission 
> whereas Group has both WRITE and EXECUTE permission and Other has WRITE 
> permission. I think it is not good to give Group and Other at least WRITE 
> permission whereas User itself has no WRITE permission.
> 
> May be, it's better to check those three permissions bit wise rather than 
as 
> a whole. Please rethink about my point and let me know your opinion.
> 
> Thanks,
> Gobinda
> 
> 

Hi All,

I could not get any response yet.

Hi Rusty,

Please at least give me a reply even if my concept seems to be incorrect to 
you.

Thanks in advance,
Gobinda

Re: Stricter module param and sysfs permission checks

[Gobinda Charan Maji]

Gobinda Charan Maji <gobinda.cemk07 <at> gmail.com> writes:

> 
> Hi All,
> 
> I could not get any response yet.
> 
> Hi Rusty,
> 
> Please at least give me a reply even if my concept seems to be incorrect to 
> you.
> 
> Thanks in advance,
> Gobinda
> 
> 

Hi All,

I am new to this mail chain. I could not get any reply yet. May be I have 
posted my query at a wrong place. If this mail is visible to anyone of you 
please give me a reply and kindly suggest me the appropriate place to post my 
query. I'll be highly grateful you...

Thanks in advance,
Gobinda

Re: Stricter module param and sysfs permission checks

[Gobinda Charan Maji]

Robert Jarzmik <robert.jarzmik <at> free.fr> writes:

As per the newly added restriction (User perms >= group perms >= other 
perms) is concerned, there is an inconsistency in the permission. Say for 
example, permission value is "0432". Here User has only READ permission 
whereas Group has both WRITE and EXECUTE permission and Other has WRITE 
permission. I think it is not good to give Group and Other at least WRITE 
permission whereas User itself has no WRITE permission.

May be, it's better to check those three permissions bit wise rather than as 
a whole. Please rethink about my point and let me know your opinion.

Thanks,
Gobinda