On 11/12, Andrew Morton wrote:
>
> From: David Daney <[email protected]>
>
> 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.
On 11/13/2010 09:17 AM, Oleg Nesterov wrote:
> On 11/12, Andrew Morton wrote:
>>
>> From: David Daney<[email protected]>
>>
>> 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
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.