[patch 1/5] x86: headers cleanup - boot.h

[patch 1/5] x86: headers cleanup - boot.h

[Cyrill Gorcunov]

[-- Attachment #1: x86-header-boot-h --]
[-- Type: text/plain, Size: 1471 bytes --]

Impact: cleanup

'make headers_check' warn us about leaking of kernel private
(mostly compile time vars) data to userspace in headers. Fix it.

Neither BOOT_HEAP_SIZE, BOOT_STACK_SIZE refs
was found by searching thru net (ie in user-space area)
so fence this all by __KERNEL__ guard.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

---
 arch/x86/include/asm/boot.h |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/boot.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/boot.h
+++ linux-2.6.git/arch/x86/include/asm/boot.h
@@ -10,14 +10,16 @@
 #define EXTENDED_VGA	0xfffe		/* 80x50 mode */
 #define ASK_VGA		0xfffd		/* ask for it at bootup */
 
+#ifdef __KERNEL__
+
 /* Physical address where kernel should be loaded. */
 #define LOAD_PHYSICAL_ADDR ((CONFIG_PHYSICAL_START \
 				+ (CONFIG_PHYSICAL_ALIGN - 1)) \
 				& ~(CONFIG_PHYSICAL_ALIGN - 1))
 
-#if (defined CONFIG_KERNEL_BZIP2)
+#ifdef CONFIG_KERNEL_BZIP2
 #define BOOT_HEAP_SIZE             0x400000
-#else
+#else /* !CONFIG_KERNEL_BZIP2 */
 
 #ifdef CONFIG_X86_64
 #define BOOT_HEAP_SIZE	0x7000
@@ -25,7 +27,7 @@
 #define BOOT_HEAP_SIZE	0x4000
 #endif
 
-#endif
+#endif /* !CONFIG_KERNEL_BZIP2 */
 
 #ifdef CONFIG_X86_64
 #define BOOT_STACK_SIZE	0x4000
@@ -33,4 +35,6 @@
 #define BOOT_STACK_SIZE	0x1000
 #endif
 
+#endif /* __KERNEL__ */
+
 #endif /* _ASM_X86_BOOT_H */

-- 

Re: [patch 1/5] x86: headers cleanup - boot.h

[Harvey Harrison]

On Wed, 2009-01-14 at 23:37 +0300, Cyrill Gorcunov wrote:
> -#if (defined CONFIG_KERNEL_BZIP2)
> +#ifdef CONFIG_KERNEL_BZIP2
>  #define BOOT_HEAP_SIZE             0x400000
> -#else
> +#else /* !CONFIG_KERNEL_BZIP2 */

Curious, I haven't seen the #else comments with the extra ! before,
is that something that's coming into common use?

Harvey

Re: [patch 1/5] x86: headers cleanup - boot.h

[Cyrill Gorcunov]

[Harvey Harrison - Wed, Jan 14, 2009 at 01:04:40PM -0800]
| On Wed, 2009-01-14 at 23:37 +0300, Cyrill Gorcunov wrote:
| > -#if (defined CONFIG_KERNEL_BZIP2)
| > +#ifdef CONFIG_KERNEL_BZIP2
| >  #define BOOT_HEAP_SIZE             0x400000
| > -#else
| > +#else /* !CONFIG_KERNEL_BZIP2 */
| 
| Curious, I haven't seen the #else comments with the extra ! before,
| is that something that's coming into common use?
| 
| Harvey
| 

Actually I saw the both.

./arch/mips/include/asm/dec/prom.h:54:#else /* !CONFIG_64BIT */
./arch/mips/include/asm/dec/prom.h:58:#endif /* !CONFIG_64BIT */

		- Cyrill -

Re: [patch 1/5] x86: headers cleanup - boot.h

[Cyrill Gorcunov]

[Cyrill Gorcunov - Thu, Jan 15, 2009 at 12:11:42AM +0300]
| [Harvey Harrison - Wed, Jan 14, 2009 at 01:04:40PM -0800]
| | On Wed, 2009-01-14 at 23:37 +0300, Cyrill Gorcunov wrote:
| | > -#if (defined CONFIG_KERNEL_BZIP2)
| | > +#ifdef CONFIG_KERNEL_BZIP2
| | >  #define BOOT_HEAP_SIZE             0x400000
| | > -#else
| | > +#else /* !CONFIG_KERNEL_BZIP2 */
| | 
| | Curious, I haven't seen the #else comments with the extra ! before,
| | is that something that's coming into common use?
| | 
| | Harvey
| | 
| 
| Actually I saw the both.
| 
| ./arch/mips/include/asm/dec/prom.h:54:#else /* !CONFIG_64BIT */
| ./arch/mips/include/asm/dec/prom.h:58:#endif /* !CONFIG_64BIT */
| 
| 		- Cyrill -

You know Harvey, I think such a details could be
mentioned in CodingStyle. At least I found two
types of comments:

1) Forward comments:

	#if FOO
	#else	/* !FOO */
	#endif	/* !FOO */

2) Backward comments:

	#if FOO
	#else	/* FOO */
	#endif	/* !FOO */

I prefer the first variant. But with any variant
I'm always rereading twice what is defined and
what is not :)

		- Cyrill -

[patch 2/5] x86: headers cleanup - prctl.h

[Cyrill Gorcunov]

[-- Attachment #1: x86-header-prctl-h --]
[-- Type: text/plain, Size: 990 bytes --]

Impact: cleanup

'make headers_check' warn us about leaking of kernel private
(mostly compile time vars) data to userspace in headers. Fix it.

sys_arch_prctl is completely removed from
header since frankly I don't even understand why we
describe it here. It is described like
__SYSCALL(__NR_arch_prctl, sys_arch_prctl) in unistd_64.h
and implemented in process_64.c. User-mode linux involved?
So this one in fact is suspicious.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/include/asm/prctl.h |    4 ----
 1 file changed, 4 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/prctl.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/prctl.h
+++ linux-2.6.git/arch/x86/include/asm/prctl.h
@@ -6,8 +6,4 @@
 #define ARCH_GET_FS 0x1003
 #define ARCH_GET_GS 0x1004
 
-#ifdef CONFIG_X86_64
-extern long sys_arch_prctl(int, unsigned long);
-#endif /* CONFIG_X86_64 */
-
 #endif /* _ASM_X86_PRCTL_H */

-- 

[patch 3/5] x86: headers cleanup - ptrace-abi.h

[Cyrill Gorcunov]

[-- Attachment #1: x86-header-ptrace-abi-h --]
[-- Type: text/plain, Size: 1269 bytes --]

Impact: cleanup

'make headers_check' warn us about leaking of kernel private
(mostly compile time vars) data to userspace in headers. Fix it.

ptrace_bts_config struct is wrapped by __KERNEL__ --
not sure if it was ever proposed for userland. On the
other hand branch tracing is really anxious for userland
developers.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/include/asm/ptrace-abi.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/ptrace-abi.h
+++ linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
@@ -50,7 +50,7 @@
 #define RSP 152
 #define SS 160
 #define ARGOFFSET R11
-#endif /* __ASSEMBLY__ */
+#endif /* __ASSEMBLY__ || __FRAME_OFFSETS */
 
 /* top of stack page */
 #define FRAME_SIZE 168
@@ -80,6 +80,7 @@
 
 #define PTRACE_SINGLEBLOCK	33	/* resume execution until next branch */
 
+#ifdef __KERNEL__
 #ifdef CONFIG_X86_PTRACE_BTS
 
 #ifndef __ASSEMBLY__
@@ -141,5 +142,6 @@ struct ptrace_bts_config {
    Returns number of BTS records drained.
 */
 #endif /* CONFIG_X86_PTRACE_BTS */
+#endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_PTRACE_ABI_H */

-- 

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[H. Peter Anvin]

Okay, this one really looks wrong to me.

Markus, could you confirm if this was ever intended to be exported to 
userspace?  I suspect The Right Thing is simply to remove the #ifdef 
CONFIG_X86_TRACE_BTS here...

	-hpa


Cyrill Gorcunov wrote:
> Impact: cleanup
> 
> 'make headers_check' warn us about leaking of kernel private
> (mostly compile time vars) data to userspace in headers. Fix it.
> 
> ptrace_bts_config struct is wrapped by __KERNEL__ --
> not sure if it was ever proposed for userland. On the
> other hand branch tracing is really anxious for userland
> developers.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  arch/x86/include/asm/ptrace-abi.h |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/include/asm/ptrace-abi.h
> +++ linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
> @@ -50,7 +50,7 @@
>  #define RSP 152
>  #define SS 160
>  #define ARGOFFSET R11
> -#endif /* __ASSEMBLY__ */
> +#endif /* __ASSEMBLY__ || __FRAME_OFFSETS */
>  
>  /* top of stack page */
>  #define FRAME_SIZE 168
> @@ -80,6 +80,7 @@
>  
>  #define PTRACE_SINGLEBLOCK	33	/* resume execution until next branch */
>  
> +#ifdef __KERNEL__
>  #ifdef CONFIG_X86_PTRACE_BTS
>  
>  #ifndef __ASSEMBLY__
> @@ -141,5 +142,6 @@ struct ptrace_bts_config {
>     Returns number of BTS records drained.
>  */
>  #endif /* CONFIG_X86_PTRACE_BTS */
> +#endif /* __KERNEL__ */
>  
>  #endif /* _ASM_X86_PTRACE_ABI_H */
> 

RE: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[Metzger, Markus T]

>-----Original Message-----
>From: H. Peter Anvin [mailto:hpa@zytor.com]
>Sent: Wednesday, January 14, 2009 11:22 PM
>To: Cyrill Gorcunov; Metzger, Markus T
>Cc: mingo@elte.hu; linux-kernel@vger.kernel.org; harvey.harrison@gmail.com; sam@ravnborg.org;
>jaswinderrajput@gmail.com; Cyrill Gorcunov
>Subject: Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h
>
>Okay, this one really looks wrong to me.
>
>Markus, could you confirm if this was ever intended to be exported to
>userspace?  I suspect The Right Thing is simply to remove the #ifdef
>CONFIG_X86_TRACE_BTS here...

This is intended for userspace.
I added the #ifdef so the entire feature could be turned off.
This might not be necessary for declarations and macros, though.

Do you want me to remove the #ifdef CONFIG around the declarations in ptrace headers?

regards,
markus.

>
>	-hpa
>
>
>Cyrill Gorcunov wrote:
>> Impact: cleanup
>>
>> 'make headers_check' warn us about leaking of kernel private
>> (mostly compile time vars) data to userspace in headers. Fix it.
>>
>> ptrace_bts_config struct is wrapped by __KERNEL__ --
>> not sure if it was ever proposed for userland. On the
>> other hand branch tracing is really anxious for userland
>> developers.
>>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> ---
>>  arch/x86/include/asm/ptrace-abi.h |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
>> ===================================================================
>> --- linux-2.6.git.orig/arch/x86/include/asm/ptrace-abi.h
>> +++ linux-2.6.git/arch/x86/include/asm/ptrace-abi.h
>> @@ -50,7 +50,7 @@
>>  #define RSP 152
>>  #define SS 160
>>  #define ARGOFFSET R11
>> -#endif /* __ASSEMBLY__ */
>> +#endif /* __ASSEMBLY__ || __FRAME_OFFSETS */
>>
>>  /* top of stack page */
>>  #define FRAME_SIZE 168
>> @@ -80,6 +80,7 @@
>>
>>  #define PTRACE_SINGLEBLOCK	33	/* resume execution until next branch */
>>
>> +#ifdef __KERNEL__
>>  #ifdef CONFIG_X86_PTRACE_BTS
>>
>>  #ifndef __ASSEMBLY__
>> @@ -141,5 +142,6 @@ struct ptrace_bts_config {
>>     Returns number of BTS records drained.
>>  */
>>  #endif /* CONFIG_X86_PTRACE_BTS */
>> +#endif /* __KERNEL__ */
>>
>>  #endif /* _ASM_X86_PTRACE_ABI_H */
>>
---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[Cyrill Gorcunov]

[Metzger, Markus T - Thu, Jan 15, 2009 at 08:06:31AM +0000]
| >-----Original Message-----
| >From: H. Peter Anvin [mailto:hpa@zytor.com]
| >Sent: Wednesday, January 14, 2009 11:22 PM
| >To: Cyrill Gorcunov; Metzger, Markus T
| >Cc: mingo@elte.hu; linux-kernel@vger.kernel.org; harvey.harrison@gmail.com; sam@ravnborg.org;
| >jaswinderrajput@gmail.com; Cyrill Gorcunov
| >Subject: Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h
| >
| >Okay, this one really looks wrong to me.
| >
| >Markus, could you confirm if this was ever intended to be exported to
| >userspace?  I suspect The Right Thing is simply to remove the #ifdef
| >CONFIG_X86_TRACE_BTS here...
| 
| This is intended for userspace.
| I added the #ifdef so the entire feature could be turned off.
| This might not be necessary for declarations and macros, though.
| 
| Do you want me to remove the #ifdef CONFIG around the declarations in ptrace headers?
| 
| regards,
| markus.

Hi Markus,

since it's supposed to be visible by userspace then we
should just remove this CONFIG_X86_PTRACE_BTS so if userspace
app has a reference to ptrace_bts_config but kernel in turn has
this feature turned off -- the uerspace app shouldn't fail
while being compiling.

| >
| >	-hpa
| >
| >
...

		- Cyrill -

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> Hi Markus,
> 
> since it's supposed to be visible by userspace then we
> should just remove this CONFIG_X86_PTRACE_BTS so if userspace
> app has a reference to ptrace_bts_config but kernel in turn has
> this feature turned off -- the uerspace app shouldn't fail
> while being compiling.
> 

Well, for userspace, the kernel configuration doesn't matter: the
CONFIG_* macros *never* exist.

This would be yet another good reason why having them be always defined
and 0/1 instead would be such an improvement, but we're not there.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[Cyrill Gorcunov]

[H. Peter Anvin - Thu, Jan 15, 2009 at 08:52:24AM -0800]
| Cyrill Gorcunov wrote:
| > 
| > Hi Markus,
| > 
| > since it's supposed to be visible by userspace then we
| > should just remove this CONFIG_X86_PTRACE_BTS so if userspace
| > app has a reference to ptrace_bts_config but kernel in turn has
| > this feature turned off -- the uerspace app shouldn't fail
| > while being compiling.
| > 
| 
| Well, for userspace, the kernel configuration doesn't matter: the
| CONFIG_* macros *never* exist.

Let me explain more detailed what I've meant with my prev message.
Markus said that ptrace_bts_config is supposed to be visible in userspace,
right? So I've a program which is built under kernel with
CONFIG_X86_PTRACE_BTS turned on. Then I rebuild my kernel with
CONFIG_X86_PTRACE_BTS turned off. Then I decide to recompile
my program for some reason _and_ compilation shouldn't fail
because of lack of ptrace_bts_config struct. So for userspace
kernel configuration does matter if it touches data being referenced
from user-side. BUT all I said is valid (at least I hop so) if _only_
ptrace_bts_config is supposed to be visible to user-space programs.

| 
| This would be yet another good reason why having them be always defined
| and 0/1 instead would be such an improvement, but we're not there.
| 

oh, that reminds me autoconf horror :) I don't know if it possible
but we could have some common/base file with all CONFIG_ set to 0/1
which any header being exported to userspace should include, or
we could modify unifdef to process headers in fashion: scan the header,
insert CONFIG_'s refered in the header with value 0/1 at top of the
header. Not sure if it worth it (too many files are to be touched).

| 	-hpa
| 
| -- 
| H. Peter Anvin, Intel Open Source Technology Center
| I work for Intel.  I don't speak on their behalf.
| 

		- Cyrill -

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> Let me explain more detailed what I've meant with my prev message.
> Markus said that ptrace_bts_config is supposed to be visible in userspace,
> right? So I've a program which is built under kernel with
> CONFIG_X86_PTRACE_BTS turned on. Then I rebuild my kernel with
> CONFIG_X86_PTRACE_BTS turned off. Then I decide to recompile
> my program for some reason _and_ compilation shouldn't fail
> because of lack of ptrace_bts_config struct. So for userspace
> kernel configuration does matter if it touches data being referenced
> from user-side. BUT all I said is valid (at least I hop so) if _only_
> ptrace_bts_config is supposed to be visible to user-space programs.
> 

CONFIG_* is not visible to userspace.  Furthermore, there is (almost) no
point in putting a structure definition under #ifdef unless it uses data
types that somehow depend on the configuration (and those data
structures would be fundamentally ineligible to be exported to
userspace!!) -- if the feature isn't configured the structure definition
just doesn't get used.

> | 
> | This would be yet another good reason why having them be always defined
> | and 0/1 instead would be such an improvement, but we're not there.
> | 
> 
> oh, that reminds me autoconf horror :) I don't know if it possible
> but we could have some common/base file with all CONFIG_ set to 0/1
> which any header being exported to userspace should include, or
> we could modify unifdef to process headers in fashion: scan the header,
> insert CONFIG_'s refered in the header with value 0/1 at top of the
> header. Not sure if it worth it (too many files are to be touched).
> 

No, no, no, no, no.

We're not exporting CONFIG_* to userspace.

The point was that if we were using #if instead of #ifdef, then -Wundef
could be used to complain instead of silently ignoring sections.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch 3/5] x86: headers cleanup - ptrace-abi.h

[Cyrill Gorcunov]

[H. Peter Anvin - Thu, Jan 15, 2009 at 09:55:23AM -0800]
... 
| The point was that if we were using #if instead of #ifdef, then -Wundef
| could be used to complain instead of silently ignoring sections.

Ah, I see now what you mean.

| 
| 	-hpa
| 
| -- 
| H. Peter Anvin, Intel Open Source Technology Center
| I work for Intel.  I don't speak on their behalf.
| 

		- Cyrill -

[patch 4/5] x86: headers cleanup - sigcontext32.h

[Cyrill Gorcunov]

[-- Attachment #1: x86-header-sigcontext32-h --]
[-- Type: text/plain, Size: 668 bytes --]

Impact: cleanup

'make headers_check' warn us about lack of linux/types.h
here. Lets add it.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

---
 arch/x86/include/asm/sigcontext32.h |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6.git/arch/x86/include/asm/sigcontext32.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/sigcontext32.h
+++ linux-2.6.git/arch/x86/include/asm/sigcontext32.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_X86_SIGCONTEXT32_H
 #define _ASM_X86_SIGCONTEXT32_H
 
+#include <linux/types.h>
+
 /* signal context for 32bit programs. */
 
 #define X86_FXSR_MAGIC		0x0000

-- 

[patch 5/5] x86: headers cleanup - setup.h

[Cyrill Gorcunov]

[-- Attachment #1: x86-header-setup-h --]
[-- Type: text/plain, Size: 1457 bytes --]

Impact: cleanup

'make headers_check' warn us about leaking of kernel private
(mostly compile time vars) data to userspace in headers. Fix it.

Guard this one by __KERNEL__.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/include/asm/setup.h |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/setup.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/setup.h
+++ linux-2.6.git/arch/x86/include/asm/setup.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_X86_SETUP_H
 #define _ASM_X86_SETUP_H
 
+#ifdef __KERNEL__
+
 #define COMMAND_LINE_SIZE 2048
 
 #ifndef __ASSEMBLY__
@@ -8,10 +10,8 @@
 /* Interrupt control for vSMPowered x86_64 systems */
 void vsmp_init(void);
 
-
 void setup_bios_corruption_check(void);
 
-
 #ifdef CONFIG_X86_VISWS
 extern void visws_early_detect(void);
 extern int is_visws_box(void);
@@ -43,7 +43,7 @@ struct x86_quirks {
 	void (*mpc_oem_bus_info)(struct mpc_bus *m, char *name);
 	void (*mpc_oem_pci_bus)(struct mpc_bus *m);
 	void (*smp_read_mpc_oem)(struct mpc_oemtable *oemtable,
-                                    unsigned short oemsize);
+				unsigned short oemsize);
 	int (*setup_ioapic_ids)(void);
 	int (*update_genapic)(void);
 };
@@ -56,8 +56,6 @@ extern unsigned long saved_video_mode;
 #endif
 #endif /* __ASSEMBLY__ */
 
-#ifdef __KERNEL__
-
 #ifdef __i386__
 
 #include <linux/pfn.h>

--