+ exec_domain-establish-a-linux32-domain-on-config_compat-systems.patch added to -mm tree

+ exec_domain-establish-a-linux32-domain-on-config_compat-systems.patch added to -mm tree

[akpm]

The patch titled
     exec_domain: establish a Linux32 domain on CONFIG_COMPAT systems
has been added to the -mm tree.  Its filename is
     exec_domain-establish-a-linux32-domain-on-config_compat-systems.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: exec_domain: establish a Linux32 domain on CONFIG_COMPAT systems
From: David Daney <ddaney@caviumnetworks.com>

If PER_LINUX32 is set calling sys_personality, we will try to find the
corresponding exec_domain.  This causes us to try to load a module for
personality-8.  After running the userspace module loader and failing to
find the module, we fall back to the default.

We can avoid the failed module loading overhead by building-in the
linux32_exec_domain for systems that have CONFIG_COMPAT.

I noticed this on MIPS64 systems, but it is a general problem and applies
to x86 among others.  Admittedly sys_personality() is probably not in the
hot path of many programs, but the overhead of the extra exec_domain is
fairly small.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Roland McGrath <roland@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Kyle McMartin <kyle@mcmartin.ca>
Cc: Helge Deller <deller@gmx.de>
Cc: "James E.J. Bottomley" <jejb@parisc-linux.org>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: David Miller <davem@davemloft.net>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: "Luck, Tony" <tony.luck@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/exec_domain.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff -puN kernel/exec_domain.c~exec_domain-establish-a-linux32-domain-on-config_compat-systems kernel/exec_domain.c
--- a/kernel/exec_domain.c~exec_domain-establish-a-linux32-domain-on-config_compat-systems
+++ a/kernel/exec_domain.c
@@ -34,6 +34,17 @@ static unsigned long ident_map[32] = {
 	24,	25,	26,	27,	28,	29,	30,	31
 };
 
+#ifdef CONFIG_COMPAT
+static struct exec_domain linux32_exec_domain = {
+	.name		= "Linux32",		/* name */
+	.handler	= default_handler,	/* lcall7 causes a seg fault. */
+	.pers_low	= PER_LINUX32,
+	.pers_high	= PER_LINUX32,
+	.signal_map	= ident_map,		/* Identity map signals. */
+	.signal_invmap	= ident_map,		/*  - both ways. */
+};
+#endif
+
 struct exec_domain default_exec_domain = {
 	.name		= "Linux",		/* name */
 	.handler	= default_handler,	/* lcall7 causes a seg fault. */
@@ -41,6 +52,9 @@ struct exec_domain default_exec_domain =
 	.pers_high	= 0,			/* PER_LINUX personality. */
 	.signal_map	= ident_map,		/* Identity map signals. */
 	.signal_invmap	= ident_map,		/*  - both ways. */
+#ifdef CONFIG_COMPAT
+	.next		= &linux32_exec_domain,
+#endif
 };
 
 
_

Patches currently in -mm which might be from ddaney@caviumnetworks.com are

origin.patch
exec_domain-establish-a-linux32-domain-on-config_compat-systems.patch

Re: + exec_domain-establish-a-linux32-domain-on-config_compat-systems.patc h added to -mm tree

[Oleg Nesterov]

On 11/12, Andrew Morton wrote:
>
> From: David Daney <ddaney@caviumnetworks.com>
>
> If PER_LINUX32 is set calling sys_personality, we will try to find the
> corresponding exec_domain.  This causes us to try to load a module for
> personality-8.  After running the userspace module loader and failing to
> find the module, we fall back to the default.

Cough. It is not easy to me comment this patch ;)

Personally, I think this change is fine. But, despite the fact
the code in exec_domain.c is very trivial, I was never able to really
understand its rationality. And the usage of ->personality has some
oddities.

In particular, I can't parse default_exec_domain() at all. And,
what exec_domain->handler() actually does? I do not see anything
in arch/ which uses EXEC_DOMAIN offsets.

Perhaps someone from CC can explain this?


> We can avoid the failed module loading overhead by building-in the
> linux32_exec_domain for systems that have CONFIG_COMPAT.

Indeed. But at the same time this means it is not possible to use
personality-8.ko if the system has it.

Don't get me wrong, I have no idea why anyone could want this module,
just I am a bit worried.

> +#ifdef CONFIG_COMPAT
> +static struct exec_domain linux32_exec_domain = {
> +	.name		= "Linux32",		/* name */
> +	.handler	= default_handler,	/* lcall7 causes a seg fault. */
> +	.pers_low	= PER_LINUX32,
> +	.pers_high	= PER_LINUX32,
> +	.signal_map	= ident_map,		/* Identity map signals. */
> +	.signal_invmap	= ident_map,		/*  - both ways. */
> +};
> +#endif
> +
>  struct exec_domain default_exec_domain = {
>  	.name		= "Linux",		/* name */
>  	.handler	= default_handler,	/* lcall7 causes a seg fault. */
> @@ -41,6 +52,9 @@ struct exec_domain default_exec_domain =
>  	.pers_high	= 0,			/* PER_LINUX personality. */
>  	.signal_map	= ident_map,		/* Identity map signals. */
>  	.signal_invmap	= ident_map,		/*  - both ways. */
> +#ifdef CONFIG_COMPAT
> +	.next		= &linux32_exec_domain,
> +#endif
>  };

OK, but please look at arch/s390/kernel/compat_exec_domain.c and
arch/ia64/mm/init.c, they also register PER_LINUX32 domain, not
good. And note that register_exec_domain() doesn't check
pers_low/high, this means linux32_exec_domain can silently supress
s390_exec_domain/ia32_exec_domain.

Oleg.

Re: + exec_domain-establish-a-linux32-domain-on-config_compat-systems.patc h added to -mm tree

[David Daney]

On 11/13/2010 09:17 AM, Oleg Nesterov wrote:
> On 11/12, Andrew Morton wrote:
>>
>> From: David Daney<ddaney@caviumnetworks.com>
>>
>> If PER_LINUX32 is set calling sys_personality, we will try to find the
>> corresponding exec_domain.  This causes us to try to load a module for
>> personality-8.  After running the userspace module loader and failing to
>> find the module, we fall back to the default.
>
> Cough. It is not easy to me comment this patch ;)
>
> Personally, I think this change is fine. But, despite the fact
> the code in exec_domain.c is very trivial, I was never able to really
> understand its rationality. And the usage of ->personality has some
> oddities.
>
> In particular, I can't parse default_exec_domain() at all. And,
> what exec_domain->handler() actually does? I do not see anything
> in arch/ which uses EXEC_DOMAIN offsets.
>
> Perhaps someone from CC can explain this?
>
>
>> We can avoid the failed module loading overhead by building-in the
>> linux32_exec_domain for systems that have CONFIG_COMPAT.
>
> Indeed. But at the same time this means it is not possible to use
> personality-8.ko if the system has it.

Well in the same way it is not possible to use personality-0.ko 
(PER_LINUX) because it is just as built-in.

>
> Don't get me wrong, I have no idea why anyone could want this module,
> just I am a bit worried.

If the personality is built-in, then I don't see how it makes any sense 
to attempt to override it with an externally supplied version.  If you 
want set a domain for PER_LINUX32, don't configure you system to supply 
a default version.

>
>> +#ifdef CONFIG_COMPAT
>> +static struct exec_domain linux32_exec_domain = {
>> +	.name		= "Linux32",		/* name */
>> +	.handler	= default_handler,	/* lcall7 causes a seg fault. */
>> +	.pers_low	= PER_LINUX32,
>> +	.pers_high	= PER_LINUX32,
>> +	.signal_map	= ident_map,		/* Identity map signals. */
>> +	.signal_invmap	= ident_map,		/*  - both ways. */
>> +};
>> +#endif
>> +
>>   struct exec_domain default_exec_domain = {
>>   	.name		= "Linux",		/* name */
>>   	.handler	= default_handler,	/* lcall7 causes a seg fault. */
>> @@ -41,6 +52,9 @@ struct exec_domain default_exec_domain =
>>   	.pers_high	= 0,			/* PER_LINUX personality. */
>>   	.signal_map	= ident_map,		/* Identity map signals. */
>>   	.signal_invmap	= ident_map,		/*  - both ways. */
>> +#ifdef CONFIG_COMPAT
>> +	.next		=&linux32_exec_domain,
>> +#endif
>>   };
>
> OK, but please look at arch/s390/kernel/compat_exec_domain.c and
> arch/ia64/mm/init.c, they also register PER_LINUX32 domain, not
> good. And note that register_exec_domain() doesn't check
> pers_low/high, this means linux32_exec_domain can silently supress
> s390_exec_domain/ia32_exec_domain.
>

Ah, I had not known about this.  The comments in arch/ia64/mm/init.c 
mirror my reason for creating the patch.

I think the s390 and ia64 definitions will conflict with the #ifdef 
CONFIG_COMPAT in my patch.  I will attempt to correct this in a new 
version of the patch.

Thanks for looking at this,
David Daney

Re: + exec_domain-establish-a-linux32-domain-on-config_compat-systems.patc h added to -mm tree

[Oleg Nesterov]

On 11/15, David Daney wrote:
>
> On 11/13/2010 09:17 AM, Oleg Nesterov wrote:
>>
>>> We can avoid the failed module loading overhead by building-in the
>>> linux32_exec_domain for systems that have CONFIG_COMPAT.
>>
>> Indeed. But at the same time this means it is not possible to use
>> personality-8.ko if the system has it.
>
> Well in the same way it is not possible to use personality-0.ko
> (PER_LINUX) because it is just as built-in.

Sure, but this was never possible. But your patch adds the obvious
user-visible change.

>> Don't get me wrong, I have no idea why anyone could want this module,
>> just I am a bit worried.
>
> If the personality is built-in, then I don't see how it makes any sense
> to attempt to override it with an externally supplied version.  If you
> want set a domain for PER_LINUX32, don't configure you system to supply
> a default version.

Well, no need to convince me ;) To me, this request_module() doesn't
make any sense at all.

I won't argue against this change. Just I wanted to be sure this
issue is not overlooked.

Oleg.