linux-kernel - Re: [PATCH v2] x86/mm: fix cpu stuck issue in __change_page_attr_set

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]

Message-ID: <alpine.DEB.2.21.1807050815570.1579@nanos.tec.linutronix.de>
Date:   Thu, 5 Jul 2018 08:28:23 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Bin Yang <bin.yang@...el.com>
cc:     mingo@...hat.com, hpa@...or.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/mm: fix cpu stuck issue in
 __change_page_attr_set_clr

On Thu, 5 Jul 2018, Bin Yang wrote:
>  static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
> -				   unsigned long pfn)
> +				   unsigned long pfn, unsigned long *page_num)
>  {
>  	pgprot_t forbidden = __pgprot(0);
> +	unsigned long tmp;
> +	unsigned long num = PUD_PAGE_SIZE >> PAGE_SHIFT;

Everything is a PUD? Oh well, you clearly made a lot of effort to
understand the code you are changing.

>  	/*
>  	 * The BIOS area between 640k and 1Mb needs to be executable for
>  	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
>  	 */
>  #ifdef CONFIG_PCI_BIOS
> -	if (pcibios_enabled && within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
> -		pgprot_val(forbidden) |= _PAGE_NX;
> +	if (pcibios_enabled) {
> +		tmp = (BIOS_BEGIN >> PAGE_SHIFT) > pfn ?
> +			(BIOS_BEGIN >> PAGE_SHIFT) - pfn : ULONG_MAX;
> +		if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT,
> +		    BIOS_END >> PAGE_SHIFT)) {
> +			pgprot_val(forbidden) |= _PAGE_NX;
> +			tmp = (BIOS_END >> PAGE_SHIFT) - pfn;
> +		}
> +		num = num > tmp ? tmp : num;

What? I really gave you a hint which used a overlaps() helper in the pseudo
code. But sure open coding the same thing in 5 places is faster, right?

You managed to make this code completely unreadable and I'm not even trying
to review that mess.

> +	for (i = 0; i < (psize >> PAGE_SHIFT);
> +	    i += pnum, addr += PAGE_SIZE * pnum, pfn += pnum) {
> +		pgprot_t chk_prot =
> +			static_protections(req_prot, addr, pfn, &pnum);

When done right, then there is no need for a loop at all. And I told you so.
But that would need more effort than creating a trainwreck, right? 

Stop sending half baken and half thought out patches. I really spent a lot
of time explaining you things in detail and giving you hints how it should
be done. Feel free to ignore me, but don't be surprised if I'm ignoring you
as well.

Thanks,

	tglx