Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752498AbbFLDYG (ORCPT ); Thu, 11 Jun 2015 23:24:06 -0400 Received: from mail-ie0-f177.google.com ([209.85.223.177]:33319 "EHLO mail-ie0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751198AbbFLDYC (ORCPT ); Thu, 11 Jun 2015 23:24:02 -0400 MIME-Version: 1.0 In-Reply-To: <87y4jpg0q6.fsf@rustcorp.com.au> References: <1433958254-6776-1-git-send-email-ddstreet@ieee.org> <87y4jpg0q6.fsf@rustcorp.com.au> From: Dan Streetman Date: Thu, 11 Jun 2015 23:23:41 -0400 X-Google-Sender-Auth: fWrESugNXmK8g_1JRFTC4BuH3vE Message-ID: Subject: Re: [PATCH] module: make perm const, change BUG_ON to BUILD_BUG_ON To: Rusty Russell Cc: Jani Nikula , Greg Kroah-Hartman , "David S. Miller" , Christoph Hellwig , Andrew Morton , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7590 Lines: 172 On Thu, Jun 11, 2015 at 9:43 PM, Rusty Russell wrote: > Dan Streetman 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 > 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 > Signed-off-by: Rusty Russell (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/