Messages in this thread | | | Date | Mon, 15 Nov 2010 10:57:38 -0800 | From | David Daney <> | Subject | Re: + exec_domain-establish-a-linux32-domain-on-config_compat-systems.patc h added to -mm tree |
| |
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
| |