Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Nikolay Borisov <nik.borisov@suse.com>, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org, mhocko@suse.com, jslaby@suse.cz,
        Nikolay Borisov <nik.borisov@suse.com>
Subject: Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled
 is true
In-Reply-To: <20230609111311.4110901-4-nik.borisov@suse.com>
References: <20230609111311.4110901-1-nik.borisov@suse.com>
 <20230609111311.4110901-4-nik.borisov@suse.com>
Date:   Fri, 09 Jun 2023 17:22:22 +0200
Message-ID: <87edmkirtd.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> First stage of disabling ia32 compat layer is to disable 32bit syscall
> entry points. Legacy int 0x80 vector is disabled by zeroing out its gate
> descriptor in the idt and the sysenter vector is disabled by re-using
> the existing code in case IA32_EMULATION is disabled.

This describes WHAT the patch does without providing any context.

> +	if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
> +	    !IS_ENABLED(CONFIG_IA32_EMULATION)) {

I told you before that my brain based compiler complains about your
patches not building with CONFIG_IA32_EMULATION=n. The above still fails
to build.

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.

It's not too much asked to flip the config switch which affects the code
you are changing for a test.

> @@ -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.

> +		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 don't care about you wasting your time, but I very much care about you
wasting my time.

Stop this right now before it becomes a habit.

Thanks,

        tglx