[PATCH] headers_check.pl: disallow extern's

[PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

Since prototypes with "extern" refer to kernel functions, they make no
sense in userspace, so reject them automatically.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 scripts/headers_check.pl |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/scripts/headers_check.pl b/scripts/headers_check.pl
index 488a3b1..15b9bc6 100644
--- a/scripts/headers_check.pl
+++ b/scripts/headers_check.pl
@@ -33,6 +33,7 @@ foreach my $file (@files) {
 	while ($line = <FH>) {
 		$lineno++;
 		check_include();
+		check_prototypes();
 	}
 	close FH;
 }
@@ -54,3 +55,11 @@ sub check_include
 		}
 	}
 }
+
+sub check_prototypes
+{
+	if ($line =~ m/^\s*extern\b/) {
+		printf STDERR "$filename:$lineno: extern's make no sense in userspace\n";
+		$ret = 1;
+	}
+}
-- 
1.6.0.6

Re: [PATCH] headers_check.pl: disallow extern's

[Sam Ravnborg]

On Sat, Dec 27, 2008 at 02:43:36AM -0500, Mike Frysinger wrote:
> Since prototypes with "extern" refer to kernel functions, they make no
> sense in userspace, so reject them automatically.

Hi Mike.

I do agree with this - but adding this check resulted in:
usr/include/linux/coda_psdev.h:90: extern's make no sense in userspace
usr/include/linux/elf-fdpic.h:62: extern's make no sense in userspace
usr/include/linux/elf.h:379: extern's make no sense in userspace
usr/include/linux/elf.h:387: extern's make no sense in userspace
usr/include/linux/elf.h:401: extern's make no sense in userspace
usr/include/linux/elf.h:402: extern's make no sense in userspace
usr/include/linux/fs.h:24: extern's make no sense in userspace
usr/include/linux/fs.h:41: extern's make no sense in userspace
usr/include/linux/fs.h:42: extern's make no sense in userspace
usr/include/linux/fs.h:49: extern's make no sense in userspace
usr/include/linux/fs.h:51: extern's make no sense in userspace
usr/include/linux/fs.h:54: extern's make no sense in userspace
usr/include/linux/hid.h:74: extern's make no sense in userspace
usr/include/linux/in6.h:47: extern's make no sense in userspace
usr/include/linux/in6.h:49: extern's make no sense in userspace
usr/include/linux/nubus.h:296: extern's make no sense in userspace
usr/include/linux/nubus.h:298: extern's make no sense in userspace
usr/include/linux/nubus.h:302: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:687: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:995: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:997: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1467: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1760: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1764: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1766: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1769: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1771: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1805: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1948: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1949: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1950: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1951: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1962: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1963: extern's make no sense in userspace
usr/include/linux/reiserfs_fs.h:1964: extern's make no sense in userspace
usr/include/linux/socket.h:29: extern's make no sense in userspace
usr/include/linux/sound.h:33: extern's make no sense in userspace
usr/include/linux/sound.h:34: extern's make no sense in userspace
usr/include/linux/sound.h:35: extern's make no sense in userspace
usr/include/linux/sound.h:36: extern's make no sense in userspace
usr/include/linux/sound.h:37: extern's make no sense in userspace
usr/include/linux/sound.h:39: extern's make no sense in userspace
usr/include/linux/sound.h:40: extern's make no sense in userspace
usr/include/linux/sound.h:41: extern's make no sense in userspace
usr/include/linux/sound.h:42: extern's make no sense in userspace
usr/include/linux/soundcard.h:1047: extern's make no sense in userspace
usr/include/linux/soundcard.h:1048: extern's make no sense in userspace
usr/include/linux/soundcard.h:1049: extern's make no sense in userspace
usr/include/linux/soundcard.h:1050: extern's make no sense in userspace
usr/include/linux/soundcard.h:1051: extern's make no sense in userspace
usr/include/linux/soundcard.h:1053: extern's make no sense in userspace
usr/include/linux/soundcard.h:1055: extern's make no sense in userspace
usr/include/linux/soundcard.h:1056: extern's make no sense in userspace
usr/include/linux/soundcard.h:1061: extern's make no sense in userspace
usr/include/linux/soundcard.h:1062: extern's make no sense in userspace
usr/include/linux/soundcard.h:1078: extern's make no sense in userspace
usr/include/linux/soundcard.h:1079: extern's make no sense in userspace


And we need to get these fixed before we apply this patch.
Otherwise we will break every allyesconfig/allmodconfig builds

	Sam

Re: [PATCH] headers_check.pl: disallow extern's

[Sam Ravnborg]

> 
> 
> And we need to get these fixed before we apply this patch.
> Otherwise we will break every allyesconfig/allmodconfig builds

I turned them into warnings for now.
And when I was there I fixed the TODO:

>From 2bbc376f97c4a75fbbb603b34f73db235a8cd2c1 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 27 Dec 2008 19:52:20 +0100
Subject: [PATCH] kbuild: check for leaked CONFIG_ symbols to userspace

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/headers_check.pl |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/scripts/headers_check.pl b/scripts/headers_check.pl
index 5bdd975..72924a7 100644
--- a/scripts/headers_check.pl
+++ b/scripts/headers_check.pl
@@ -16,7 +16,7 @@
 #
 # 2) It is checked that prototypes does not use "extern"
 #
-# 3) TODO: check for leaked CONFIG_ symbols
+# 3) Check for leaked CONFIG_ symbols
 
 use strict;
 
@@ -36,6 +36,7 @@ foreach my $file (@files) {
 		$lineno++;
 		check_include();
 		check_prototypes();
+		check_config();
 	}
 	close FH;
 }
@@ -64,3 +65,11 @@ sub check_prototypes
 		printf STDERR "$filename:$lineno: extern's make no sense in userspace\n";
 	}
 }
+
+sub check_config
+{
+	if ($line =~ m/[^a-zA-Z0-9_]+CONFIG_([a-zA-Z0-9]+)[^a-zA-Z0-9]/) {
+		printf STDERR "$filename:$lineno: leaks CONFIG_$1 to userspace where it is not valid\n";
+	}
+}
+
-- 
1.6.0.2.GIT

Re: [PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

On Saturday 27 December 2008 13:54:15 Sam Ravnborg wrote:
> > And we need to get these fixed before we apply this patch.
> > Otherwise we will break every allyesconfig/allmodconfig builds
>
> I turned them into warnings for now.

i figured you'd choke when you saw the existing failure list :)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Saturday 27 December 2008 13:54:15 Sam Ravnborg wrote:
> > And we need to get these fixed before we apply this patch.
> > Otherwise we will break every allyesconfig/allmodconfig builds
>
> I turned them into warnings for now.
> And when I was there I fixed the TODO:
>
> From 2bbc376f97c4a75fbbb603b34f73db235a8cd2c1 Mon Sep 17 00:00:00 2001
> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Sat, 27 Dec 2008 19:52:20 +0100
> Subject: [PATCH] kbuild: check for leaked CONFIG_ symbols to userspace

btw, do you maintain these in a git tree somewhere, or you just posting them 
to lkml ?  the two kbuild trees under your name on kernel.org/git seems to be 
lacking this change.  i was looking at adding another check: barf if the 
header uses __[us]{8,16,32,64} types but does not include linux/types.h.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_check.pl: disallow extern's

[Sam Ravnborg]

On Mon, Dec 29, 2008 at 05:15:08AM -0500, Mike Frysinger wrote:
> On Saturday 27 December 2008 13:54:15 Sam Ravnborg wrote:
> > > And we need to get these fixed before we apply this patch.
> > > Otherwise we will break every allyesconfig/allmodconfig builds
> >
> > I turned them into warnings for now.
> > And when I was there I fixed the TODO:
> >
> > From 2bbc376f97c4a75fbbb603b34f73db235a8cd2c1 Mon Sep 17 00:00:00 2001
> > From: Sam Ravnborg <sam@ravnborg.org>
> > Date: Sat, 27 Dec 2008 19:52:20 +0100
> > Subject: [PATCH] kbuild: check for leaked CONFIG_ symbols to userspace
> 
> btw, do you maintain these in a git tree somewhere, or you just posting them 
> to lkml ?  the two kbuild trees under your name on kernel.org/git seems to be 
> lacking this change.  i was looking at adding another check: barf if the 
> header uses __[us]{8,16,32,64} types but does not include linux/types.h.

I push regurlary to kbuild-next.git but you hit a window where I had
asked Linus to pull and thus I avoided any updates.

I the meantime I managed to delete the patches I had queued up so
I need to do a bit of digging before I have a fully updated
kbuild-next.git again.

	Sam

Re: [PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Saturday 27 December 2008 13:43:55 Sam Ravnborg wrote:
> On Sat, Dec 27, 2008 at 02:43:36AM -0500, Mike Frysinger wrote:
> > Since prototypes with "extern" refer to kernel functions, they make no
> > sense in userspace, so reject them automatically.
>
> I do agree with this

so the next question is about using "extern" in general.  unless you have a 
nice way of detecting function prototypes (i certainly dont have a magic regex 
for this that is multiline, etc...), i think we should be using "extern" at 
least in every header that goes to userspace so that we can catch things.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_check.pl: disallow extern's

[Arnd Bergmann]

On Saturday 27 December 2008, Mike Frysinger wrote:
> +
> +sub check_prototypes
> +{
> +       if ($line =~ m/^\s*extern\b/) {
> +               printf STDERR "$filename:$lineno: extern's make no sense in userspace\n";
> +               $ret = 1;
> +       }
> +}

Unfortunately, this way you can only detect prototypes with an explicit
'extern' in them, not the equivalent form with an implicit extern.

Not sure if we can detect the other form reliably from perl.

	Arnd <><

Re: [PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On Wednesday 31 December 2008 11:58:03 Arnd Bergmann wrote:
> On Saturday 27 December 2008, Mike Frysinger wrote:
> > +sub check_prototypes
> > +{
> > +	if ($line =~ m/^\s*extern\b/) {
> > +		printf STDERR "extern's make no sense in userspace\n";
> > +		$ret = 1;
> > +	}
> > +}
>
> Unfortunately, this way you can only detect prototypes with an explicit
> 'extern' in them, not the equivalent form with an implicit extern.
>
> Not sure if we can detect the other form reliably from perl.

yes, i already mentioned both of these issues.  that's why i suggested we 
start adding "extern" to all the headers (or at least the userspace ones).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_check.pl: disallow extern's

[Arnd Bergmann]

On Wednesday 31 December 2008, Mike Frysinger wrote:
> On Wednesday 31 December 2008 11:58:03 Arnd Bergmann wrote:
>
> > Not sure if we can detect the other form reliably from perl.
> 
> yes, i already mentioned both of these issues.  that's why i suggested we 
> start adding "extern" to all the headers (or at least the userspace ones).

The general trend seems to be to remove the "extern" keyword from
kernel headers, because it is not interpreted, just like the 'auto'
keyword. In fact, only few of them currently carry the extern keyword:

$ ctags -f- -R --c-kinds=p obj/usr/include/  | grep extern | wc -l
33

$ ctags -f- -R --c-kinds=p obj/usr/include/  | grep -v extern | wc -l
191

Note that this is a completely separate issue from extern variable
declarations, which should to be disallowed in any case, and are easy
to detect as they always require the extern keyword.

	Arnd <><

Re: [PATCH] headers_check.pl: disallow extern's

[Arjan van de Ven]

On Sat, 3 Jan 2009 00:59:18 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 31 December 2008, Mike Frysinger wrote:
> > On Wednesday 31 December 2008 11:58:03 Arnd Bergmann wrote:
> >
> > > Not sure if we can detect the other form reliably from perl.
> > 
> > yes, i already mentioned both of these issues.  that's why i
> > suggested we start adding "extern" to all the headers (or at least
> > the userspace ones).
> 
> The general trend seems to be to remove the "extern" keyword from
> kernel headers, because it is not interpreted, just like the 'auto'
> keyword. In fact, only few of them currently carry the extern keyword:
> 
> $ ctags -f- -R --c-kinds=p obj/usr/include/  | grep extern | wc -l
> 33
> 
> $ ctags -f- -R --c-kinds=p obj/usr/include/  | grep -v extern | wc -l
> 191
> 
> Note that this is a completely separate issue from extern variable
> declarations, which should to be disallowed in any case, and are easy
> to detect as they always require the extern keyword.

in addition... what business does the kernel have to provide function
prototypes to userspace, other than the system call prototypes?


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] headers_check.pl: disallow extern's

[Arnd Bergmann]

On Saturday 03 January 2009, Arjan van de Ven wrote:
> in addition... what business does the kernel have to provide function
> prototypes to userspace, other than the system call prototypes?

This is the point I raised in my initial comment: we should warn about
all of them if possible. Note that the syscalls.h file is not even
exported. I can't think of a single case where a function protoype
should be exported from the kernel to user space. 

	Arnd <><

Re: [PATCH] headers_check.pl: disallow extern's

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Friday 02 January 2009 19:09:57 Arnd Bergmann wrote:
> On Saturday 03 January 2009, Arjan van de Ven wrote:
> > in addition... what business does the kernel have to provide function
> > prototypes to userspace, other than the system call prototypes?
>
> This is the point I raised in my initial comment: we should warn about
> all of them if possible. Note that the syscalls.h file is not even
> exported. I can't think of a single case where a function protoype
> should be exported from the kernel to user space.

right, and unless people someone can come up with some magic perl code to 
detect function prototypes, i suggest we start adding extern keywords ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_check.pl: disallow extern's

[Arnd Bergmann]

On Saturday 03 January 2009, Mike Frysinger wrote:
> On Friday 02 January 2009 19:09:57 Arnd Bergmann wrote:
> > On Saturday 03 January 2009, Arjan van de Ven wrote:
> > > in addition... what business does the kernel have to provide function
> > > prototypes to userspace, other than the system call prototypes?
> >
> > This is the point I raised in my initial comment: we should warn about
> > all of them if possible. Note that the syscalls.h file is not even
> > exported. I can't think of a single case where a function protoype
> > should be exported from the kernel to user space.
> 
> right, and unless people someone can come up with some magic perl code to 
> detect function prototypes, i suggest we start adding extern keywords ...

In the presence of /usr/bin/ctags-exuberant, we can use that to find all of
these. If it doesn't exist, just don't check or warn about the fact that
we might be missing checks.

	Arnd <><

Re: [PATCH] headers_check.pl: disallow extern's

[Nigel Cunningham]

Hi.

On Sat, 2008-12-27 at 02:43 -0500, Mike Frysinger wrote:
> Since prototypes with "extern" refer to kernel functions, they make no
> sense in userspace, so reject them automatically.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  scripts/headers_check.pl |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/headers_check.pl b/scripts/headers_check.pl
> index 488a3b1..15b9bc6 100644
> --- a/scripts/headers_check.pl
> +++ b/scripts/headers_check.pl
> @@ -33,6 +33,7 @@ foreach my $file (@files) {
>  	while ($line = <FH>) {
>  		$lineno++;
>  		check_include();
> +		check_prototypes();
>  	}
>  	close FH;
>  }
> @@ -54,3 +55,11 @@ sub check_include
>  		}
>  	}
>  }
> +
> +sub check_prototypes
> +{
> +	if ($line =~ m/^\s*extern\b/) {
> +		printf STDERR "$filename:$lineno: extern's make no sense in userspace\n";

s/extern's/externs/

> +		$ret = 1;
> +	}
> +}

Regards,

Nigel