Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true | Date | Sat, 10 Jun 2023 13:26:34 +0200 |
| |
On Fri, Jun 09 2023 at 19:03, Nikolay Borisov wrote: > On 9.06.23 г. 18:22 ч., Thomas Gleixner wrote: >>> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) || >>> + !IS_ENABLED(CONFIG_IA32_EMULATION)) { >> >> Aside of that this condition is convoluted and can be simplified to >> exactly a simple and understandable >> >> if (foo) >> >> which is actually the obvious solution to make it compile in all >> configurations. > > I fail to see how this can be done the way you suggest given that > ia32_disabled is visible iff IA32_EMULATION is selected, this means an > #ifdef is mandatory so that ia32_disabled is checked when we know it > will exist as a symbol, the same applies for the entry_SYSCALL_compat > symbol which has to be used iff IA32_EMULATION is defined. I.e the > ignore code should also be duplicated in the #ifdef IA32_EMULATION && > ia32_disabled and in the #else condition.
That's wrong in every aspect. Neither the #ifdeffery nor the code duplication is mandatory.
arch/x86/include/asm/XXXX.h
#ifdef CONFIG_IA32_EMULATION extern bool __ia32_enabled;
static inline bool ia32_enabled(void) { return __ia32_enabled; } #else static inline bool ia32_enabled(void) { return IS_ENABLED(CONFIG_X86_32); } #endif
if (ia32_enabled()) { ... } else { ... }
Which avoids the #ifdeffery in the code _and_ the code duplication.
All it needs aside of the above is to make sure that the other two things which the compiler complains about being undeclared in the EMULATION=n case are treated differently in the relevant header file. It's not rocket science. See also below.
If you chose $XXXX.h carefully it simply works for everything including asm/elf.h.
I surely wouldn't have suggested it if it wouldn't be feasible and reasonably trivial. I wanted you to figure it out yourself instead of serving you the solution on a silver tablet. There are tons of examples in the code.
>>> @@ -226,6 +226,13 @@ void __init idt_setup_early_traps(void) >>> void __init idt_setup_traps(void) >>> { >>> idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true); >>> + >>> + if (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) { >> >> Ditto. > > This actually doesn't fail, because if IA32_EMULATION is n that > conditional expands to 'if (0 && ia32_disabled)' which is eliminated by > the compiler.
Making uninformed claims does not make it more correct.
This _cannot_ compile because the dead code elimination pass is not even reached due to ia32_disabled being undeclared.
declaration != definition and #ifdef CONSTANT != if (CONSTANT)
Compiler basics. You could have spared yourself the embarrassment and the lecture by compiling that file with IA32_EMULATION=n or by carefully analysing the compile fail of cpu/common.c. That code is not any different:
>>> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) || >>> + !IS_ENABLED(CONFIG_IA32_EMULATION)) {
and the compiler also complains rightfully about ia32_disabled being undeclared _before_ complaining about entry_*_compat.
So:
// Declaration extern bool foo;
if (0 && foo) .... compiles and links without ever defining 'foo'.
While:
#if 0 extern bool foo; #endif
if (0 && foo) .... already fails to compile due to -Werror
See?
>>> + gate_desc null_desc = {}; >> >> Lacks a newline between declaration and code. It's documented to be >> required, no? >> >>> + write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc); >>> + clear_bit(IA32_SYSCALL_VECTOR, system_vectors); >>> + } >> >> That aside, I asked you to split IA32_SYSCALL_VECTOR out of def_idts[] >> and handle it separately, no? If you disagree with me then reply to my >> review first instead of ignoring me silently. > > I tried doing this but it's no go since def_its is defined statically. > Since tha IA32_SYSCALL_VECTOR is the last one it can't simply be tacked > at the end of this array in a separate place. Hence the only viable > solution ( apart from making def_its a dynamically sized array) was to > simply overwrite IA32_SYSCALL_VECTOR in idt_table before it's being > loaded into the ldtr.
Obviously we have fundamentally different interpretations of the phrase 'split IA32_SYSCALL_VECTOR out of def_idts[] and handle it separately':
This splits it out:
def_idts[] = { ... -#if defined(CONFIG_IA32_EMULATION) - SYSG(IA32_SYSCALL_VECTOR, entry_INT80_compat), -#elif defined(CONFIG_X86_32) - SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32), -#endif }; +ia32_idt[] = { +#if defined(CONFIG_IA32_EMULATION) + SYSG(IA32_SYSCALL_VECTOR, entry_INT80_compat), +#elif defined(CONFIG_X86_32) + SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32), +#endif +}; This handles it separately:
idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
+ if (ia32_enabled()) + idt_setup_from_table(idt_table, ia32_idt,....); Which makes it bloody obvious what this is about.
Are you still convinced that the only viable solution is clearing it after the fact?
So please go and ensure that all config combinations work and that it builds and works for 32bit too. The latter fails to build because of including traps.h into an header for no reason.
There is absolutely no urgency to get this into the tree, so please take your time and do not rush out the next half baken version of this, unless you aim for the fast path to my ignore list.
Thanks,
tglx
| |