linux-kernel - Re: [PATCH 4/4] checkpatch: only allow COMPILE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130704170958.GM22702@windriver.com>
Date:	Thu, 4 Jul 2013 13:09:58 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	Joe Perches <joe@...ches.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, <linux-kernel@...r.kernel.org>,
	Andy Whitcroft <apw@...onical.com>,
	Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig
 dependency lines

[Re: [PATCH 4/4] checkpatch: only allow COMPILE_TEST in Kconfig dependency lines] On 04/07/2013 (Thu 08:41) Joe Perches wrote:

> On Thu, 2013-07-04 at 01:39 -0400, Paul Gortmaker wrote:
> > The option CONFIG_COMPILE_TEST, added in commit 4bb1667255a
> > ("build some drivers only when compile-testing") is meant to
> > give a middle ground[1] between those who want the widest compile
> > coverage possible (e.g. building sparc drivers for mips) and
> > those who want dependencies to represent real world systems
> > (e.g. don't allow me to see OMAP options when building x86).
> > 
> > As such, this addition is meant to be used in dependency lines,
> > properly or'd in with the real world hardware dependency.  Those
> > who select it, get wide compile coverage.  Those who do not, get
> > real world dependencies that match where the hardware is available
> > and/or where the driver is useful.
> > 
> > With that in mind, it is clear that this is _not_ to be used in
> > any C code with "#ifdef CONFIG_COMPILE_TEST" etc.  However there
> > will (and already has been) instances of people thinking this is
> > an OK practice[2].  So teach checkpatch to spot them as an error.
> > 
> > [1] https://lkml.org/lkml/2013/3/7/456
> > [2] https://lkml.org/lkml/2013/7/1/641
> > 
> > Cc: Joe Perches <joe@...ches.com>
> > Cc: Andy Whitcroft <apw@...onical.com>
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Cc: Jiri Slaby <jslaby@...e.cz>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> > ---
> >  scripts/checkpatch.pl | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index b954de5..c0871a3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1922,6 +1922,12 @@ sub process {
> >  			     "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
> >  		}
> >  
> > +# disallow the addition of CONFIG_COMPILE_TEST in #if(def).
> > +		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_COMPILE_TEST\b/) {
> > +			ERROR("CONFIG_COMPILE_TEST",
> > +			     "Use of COMPILE_TEST is only allowed in Kconfig dependency lines.\n");
> 
> Why not just look for \bCONFIG_COMPILE_TEST\b?
> 
> I see it's the same style as the CONFIG_EXPERIMENTAL above it,
> but perhaps code could be written like

That is exactly why.  I valued consistency and the fact that this was
already a tested and working check, vs. me reinventing my own and then
having a "Oh crap, I never thought of that false positive" moment.

I'd suggest leaving it consistent with the existing solution as lowest
risk and then later (post 3.11) you could come along and recode all
similar instances to increase their scope if you really wanted to.

P.
--

> 
> #if defined CONFIG_FOO || \
>     defined CONFIG_BAR
> 
> and this wouldn't trigger.
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/