Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1163708 > unrolled thread
| Started by | Rusty Russell <rusty@rustcorp.com.au> |
|---|---|
| First post | 2015-06-12 03:50 +0200 |
| Last post | 2015-06-12 05:30 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] module: make perm const, change BUG_ON to BUILD_BUG_ON Rusty Russell <rusty@rustcorp.com.au> - 2015-06-12 03:50 +0200
Re: [PATCH] module: make perm const, change BUG_ON to BUILD_BUG_ON Dan Streetman <ddstreet@ieee.org> - 2015-06-12 05:30 +0200
| From | Rusty Russell <rusty@rustcorp.com.au> |
|---|---|
| Date | 2015-06-12 03:50 +0200 |
| Subject | Re: [PATCH] module: make perm const, change BUG_ON to BUILD_BUG_ON |
| Message-ID | <pAmgx-7p8-5@gated-at.bofh.it> |
Dan Streetman <ddstreet@ieee.org> writes:
> Change the struct kernel_param.perm field to a const, as it should never
> be changed. Add inline functions that return a const value, which are
> compiled out, for kparam_[un]block_sysfs_r/w() macros to use; and change
> the BUG_ON() in those macros to BUILD_BUG_ON().
Nice win!
I think it's slightly clearer if we just declare:
static __always_unused const u16 __param_perm_##name = (perm).
And indeed, gcc (at least 4.9) eliminates it correctly.
So, here's your patch with that mod, and the param.c noise removed.
It's in my pending queue for the moment.
I still am leaning towards just removing these wrappers and exposing
the (new) per-module mutex though. Happy for that patch to replace this
one, if you send it.
Thanks,
Rusty.
From: Dan Streetman <ddstreet@ieee.org>
Subject: module: make perm const, change BUG_ON to BUILD_BUG_ON
Change the struct kernel_param.perm field to a const, as it should never
be changed. Add a static const name for this (which gcc compiles out)
for kparam_[un]block_sysfs_r/w() macros to use; and change the BUG_ON()
in those macros to BUILD_BUG_ON().
This will enforce that the kernel_param permissions are never changed,
and it will fail at build instead of runtime for any misuse of
sysfs param blocking.
I initially wanted to just use the const param.perm in the block_sysfs
checks, but unfortunately that also requires the param itself to be
const, in order to do const folding into a constant compiletime
condition to BUILD_BUG_ON(), and on some archs the params aren't defined
as const:
/* On alpha, ia64 and ppc64 relocations to global data cannot go into
read-only sections (which is part of respective UNIX ABI on these
platforms). So 'const' makes no sense and even causes compile failures
with some compilers. */
Signed-off-by: Dan Streetman <ddstreet@ieee.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (slight tweak)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 7e0079936396..347e11f3bc32 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -68,7 +68,7 @@ enum {
struct kernel_param {
const char *name;
const struct kernel_param_ops *ops;
- u16 perm;
+ const u16 perm;
s8 level;
u8 flags;
union {
@@ -215,8 +215,10 @@ struct kparam_array
/* This is the fundamental function for registering boot/module
parameters. */
#define __module_param_call(prefix, name, ops, arg, perm, level, flags) \
+ /* These allow kparam_block_sysfs_*() to use BUILD_BUG_ON() */ \
+ static __always_unused const u16 __param_perm_##name = (perm); \
/* Default value instead of permissions? */ \
- 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 *)))) \
@@ -244,20 +246,20 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
*
* There's no point blocking write on a paramter that isn't writable via sysfs!
*/
-#define kparam_block_sysfs_write(name) \
- do { \
- BUG_ON(!(__param_##name.perm & 0222)); \
- __kernel_param_lock(); \
+#define kparam_block_sysfs_write(name) \
+ do { \
+ BUILD_BUG_ON(!(__param_perm_##name & 0222)); \
+ __kernel_param_lock(); \
} while (0)
/**
* kparam_unblock_sysfs_write - allows sysfs to write to a parameter again.
* @name: the name of the parameter
*/
-#define kparam_unblock_sysfs_write(name) \
- do { \
- BUG_ON(!(__param_##name.perm & 0222)); \
- __kernel_param_unlock(); \
+#define kparam_unblock_sysfs_write(name) \
+ do { \
+ BUILD_BUG_ON(!(__param_perm_##name & 0222)); \
+ __kernel_param_unlock(); \
} while (0)
/**
@@ -266,20 +268,20 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
*
* This also blocks sysfs writes.
*/
-#define kparam_block_sysfs_read(name) \
- do { \
- BUG_ON(!(__param_##name.perm & 0444)); \
- __kernel_param_lock(); \
+#define kparam_block_sysfs_read(name) \
+ do { \
+ BUILD_BUG_ON(!(__param_perm_##name & 0444)); \
+ __kernel_param_lock(); \
} while (0)
/**
* kparam_unblock_sysfs_read - allows sysfs to read a parameter again.
* @name: the name of the parameter
*/
-#define kparam_unblock_sysfs_read(name) \
- do { \
- BUG_ON(!(__param_##name.perm & 0444)); \
- __kernel_param_unlock(); \
+#define kparam_unblock_sysfs_read(name) \
+ do { \
+ BUILD_BUG_ON(!(__param_perm_##name & 0444)); \
+ __kernel_param_unlock(); \
} while (0)
#ifdef CONFIG_SYSFS
diff --git a/kernel/params.c b/kernel/params.c
index 0b9bbdf830cb..15715a3efb50 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -395,12 +395,11 @@ EXPORT_SYMBOL(param_ops_invbool);
int param_set_bint(const char *val, const struct kernel_param *kp)
{
- struct kernel_param boolkp;
+ /* Match bool exactly, by re-using it. */
+ struct kernel_param boolkp = *kp;
bool v;
int ret;
- /* Match bool exactly, by re-using it. */
- boolkp = *kp;
boolkp.arg = &v;
ret = param_set_bool(val, &boolkp);
@@ -480,9 +479,8 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
{
int i, off, ret;
const struct kparam_array *arr = kp->arr;
- struct kernel_param p;
+ struct kernel_param p = *kp;
- p = *kp;
for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
if (i)
buffer[off++] = ',';
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Dan Streetman <ddstreet@ieee.org> |
|---|---|
| Date | 2015-06-12 05:30 +0200 |
| Message-ID | <pAnPj-1sJ-3@gated-at.bofh.it> |
| In reply to | #1163708 |
On Thu, Jun 11, 2015 at 9:43 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Dan Streetman <ddstreet@ieee.org> writes:
>> Change the struct kernel_param.perm field to a const, as it should never
>> be changed. Add inline functions that return a const value, which are
>> compiled out, for kparam_[un]block_sysfs_r/w() macros to use; and change
>> the BUG_ON() in those macros to BUILD_BUG_ON().
>
> Nice win!
>
> I think it's slightly clearer if we just declare:
>
> static __always_unused const u16 __param_perm_##name = (perm).
>
> And indeed, gcc (at least 4.9) eliminates it correctly.
>
> So, here's your patch with that mod, and the param.c noise removed.
> It's in my pending queue for the moment.
>
> I still am leaning towards just removing these wrappers and exposing
> the (new) per-module mutex though. Happy for that patch to replace this
> one, if you send it.
sure ok, i'll update it and send it tomorrow. Thanks!
>
> Thanks,
> Rusty.
>
> From: Dan Streetman <ddstreet@ieee.org>
> Subject: module: make perm const, change BUG_ON to BUILD_BUG_ON
>
> Change the struct kernel_param.perm field to a const, as it should never
> be changed. Add a static const name for this (which gcc compiles out)
> for kparam_[un]block_sysfs_r/w() macros to use; and change the BUG_ON()
> in those macros to BUILD_BUG_ON().
>
> This will enforce that the kernel_param permissions are never changed,
> and it will fail at build instead of runtime for any misuse of
> sysfs param blocking.
>
> I initially wanted to just use the const param.perm in the block_sysfs
> checks, but unfortunately that also requires the param itself to be
> const, in order to do const folding into a constant compiletime
> condition to BUILD_BUG_ON(), and on some archs the params aren't defined
> as const:
>
> /* On alpha, ia64 and ppc64 relocations to global data cannot go into
> read-only sections (which is part of respective UNIX ABI on these
> platforms). So 'const' makes no sense and even causes compile failures
> with some compilers. */
>
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (slight tweak)
>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 7e0079936396..347e11f3bc32 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -68,7 +68,7 @@ enum {
> struct kernel_param {
> const char *name;
> const struct kernel_param_ops *ops;
> - u16 perm;
> + const u16 perm;
> s8 level;
> u8 flags;
> union {
> @@ -215,8 +215,10 @@ struct kparam_array
> /* This is the fundamental function for registering boot/module
> parameters. */
> #define __module_param_call(prefix, name, ops, arg, perm, level, flags) \
> + /* These allow kparam_block_sysfs_*() to use BUILD_BUG_ON() */ \
> + static __always_unused const u16 __param_perm_##name = (perm); \
> /* Default value instead of permissions? */ \
> - 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 *)))) \
> @@ -244,20 +246,20 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
> *
> * There's no point blocking write on a paramter that isn't writable via sysfs!
> */
> -#define kparam_block_sysfs_write(name) \
> - do { \
> - BUG_ON(!(__param_##name.perm & 0222)); \
> - __kernel_param_lock(); \
> +#define kparam_block_sysfs_write(name) \
> + do { \
> + BUILD_BUG_ON(!(__param_perm_##name & 0222)); \
> + __kernel_param_lock(); \
> } while (0)
>
> /**
> * kparam_unblock_sysfs_write - allows sysfs to write to a parameter again.
> * @name: the name of the parameter
> */
> -#define kparam_unblock_sysfs_write(name) \
> - do { \
> - BUG_ON(!(__param_##name.perm & 0222)); \
> - __kernel_param_unlock(); \
> +#define kparam_unblock_sysfs_write(name) \
> + do { \
> + BUILD_BUG_ON(!(__param_perm_##name & 0222)); \
> + __kernel_param_unlock(); \
> } while (0)
>
> /**
> @@ -266,20 +268,20 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
> *
> * This also blocks sysfs writes.
> */
> -#define kparam_block_sysfs_read(name) \
> - do { \
> - BUG_ON(!(__param_##name.perm & 0444)); \
> - __kernel_param_lock(); \
> +#define kparam_block_sysfs_read(name) \
> + do { \
> + BUILD_BUG_ON(!(__param_perm_##name & 0444)); \
> + __kernel_param_lock(); \
> } while (0)
>
> /**
> * kparam_unblock_sysfs_read - allows sysfs to read a parameter again.
> * @name: the name of the parameter
> */
> -#define kparam_unblock_sysfs_read(name) \
> - do { \
> - BUG_ON(!(__param_##name.perm & 0444)); \
> - __kernel_param_unlock(); \
> +#define kparam_unblock_sysfs_read(name) \
> + do { \
> + BUILD_BUG_ON(!(__param_perm_##name & 0444)); \
> + __kernel_param_unlock(); \
> } while (0)
>
> #ifdef CONFIG_SYSFS
> diff --git a/kernel/params.c b/kernel/params.c
> index 0b9bbdf830cb..15715a3efb50 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -395,12 +395,11 @@ EXPORT_SYMBOL(param_ops_invbool);
>
> int param_set_bint(const char *val, const struct kernel_param *kp)
> {
> - struct kernel_param boolkp;
> + /* Match bool exactly, by re-using it. */
> + struct kernel_param boolkp = *kp;
> bool v;
> int ret;
>
> - /* Match bool exactly, by re-using it. */
> - boolkp = *kp;
> boolkp.arg = &v;
>
> ret = param_set_bool(val, &boolkp);
> @@ -480,9 +479,8 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
> {
> int i, off, ret;
> const struct kparam_array *arr = kp->arr;
> - struct kernel_param p;
> + struct kernel_param p = *kp;
>
> - p = *kp;
> for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> if (i)
> buffer[off++] = ',';
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web