linux-kernel - Re: [PATCH v8 04/33] x86/traps: add install_system_interrupt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 05 Jun 2023 10:57:37 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Xin Li <xin3.li@...el.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org, kvm@...r.kernel.org
Cc:     mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        hpa@...or.com, peterz@...radead.org, andrew.cooper3@...rix.com,
        seanjc@...gle.com, pbonzini@...hat.com, ravi.v.shankar@...el.com,
        jiangshanlai@...il.com, shan.kang@...el.com
Subject: Re: [PATCH v8 04/33] x86/traps: add install_system_interrupt_handler()

On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> Some kernel components install system interrupt handlers into the IDT,
> and we need to do the same for system_interrupt_handlers.

We need to the same? This sentence does not make any sense at all.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

> +void install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr);

Why is this void *?

>  
> +void __init install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr)
> +{
> +	BUG_ON(n < FIRST_SYSTEM_VECTOR);
> +
> +#ifdef CONFIG_X86_64
> +	system_interrupt_handlers[n - FIRST_SYSTEM_VECTOR] = (system_interrupt_handler)addr;

Just that you can add a silly typecast here, right?

Oh well.

> -	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, asm_sysvec_xen_hvm_callback);
> +	install_system_interrupt_handler(HYPERVISOR_CALLBACK_VECTOR,
> +					 asm_sysvec_xen_hvm_callback,
> +					 sysvec_xen_hvm_callback);

Can we please make this less convoluted?

#ifdef CONFIG_X86_64
static inline void sysvec_setup_fred(unsigned int vector, void (*func)(struct pt_regs*))
{
        ...
}
#else
static inline void sysvec_setup_fred(unsigned int vector, void (*func)(struct pt_regs*))
{
}
#endif

#define sysvec_install(vector, func) {                                  \
        sysvec_setup_fred(vector, func);                                \
        alloc_intr_gate(vector, asm_##func);                            \
}

-	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, asm_sysvec_xen_hvm_callback);
+	sysvec_install(HYPERVISOR_CALLBACK_VECTOR, sysvec_xen_hvm_callback);

No?

Thanks,

        tglx