Re: [PATCH] module: make perm const, change BUG_ON to BUILD_BUG_ON

[Dan Streetman]

On Thu, Jun 11, 2015 at 9:43 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> Dan Streetman <ddstreet@xxxxxxxx> 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@xxxxxxxx>
> 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@xxxxxxxx>
> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx> (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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/