Date: Sun, 7 Jan 2018 12:36:55 +0100
From: Borislav Petkov <bp@alien8.de>
To: Jike Song <albcamus@gmail.com>
Cc: tglx@linutronix.de, linux-kernel@vger.kernel.org,
        David Woodhouse <dwmw@amazon.co.uk>,
        Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
        Jiri Koshina <jikos@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Andi Lutomirski <luto@amacapital.net>,
        Andi Kleen <ak@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Paul Turner <pjt@google.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Greg KH <gregkh@linux-foundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Kees Cook <keescook@google.com>, stable@vger.kernel.org
Subject: Re: [PATCH v2] x86/mm/pti: remove dead logic during user pagetable
 population
Message-ID: <20180107113655.zaovtpdfyjboccae@pd.tnic>
References: <20180107103317.22613-1-albcamus@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20180107103317.22613-1-albcamus@gmail.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org

On Sun, Jan 07, 2018 at 06:33:17PM +0800, Jike Song wrote:
> Look at one of the code snippets:
> 
>     162 if (pgd_none(*pgd)) {
>     163         unsigned long new_p4d_page = __get_free_page(gfp);
>     164         if (!new_p4d_page)
>     165                 return NULL;
>     166
>     167         if (pgd_none(*pgd)) {
>     168                 set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page)));
>     169                 new_p4d_page = 0;
>     170         }
>     171         if (new_p4d_page)
>     172                 free_page(new_p4d_page);
>     173 }
> 
> There can't be any difference between two pgd_none(*pgd) at L162 and L167,
> so it's always false at L171.

I think this is a remnant from the kaiser version which did this:

        if (pud_none(*pud)) {
                unsigned long new_pmd_page = __get_free_page(gfp);
                if (!new_pmd_page)
                        return NULL;
                spin_lock(&shadow_table_allocation_lock);
                if (pud_none(*pud))
                        set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page)));
                else
                        free_page(new_pmd_page);
                spin_unlock(&shadow_table_allocation_lock);
        }

I was wondering too, why the duplicated checks.

Which has this explanation about the need for the locking:

/*
 * At runtime, the only things we map are some things for CPU
 * hotplug, and stacks for new processes.  No two CPUs will ever
 * be populating the same addresses, so we only need to ensure
 * that we protect between two CPUs trying to allocate and
 * populate the same page table page.
 *
 * Only take this lock when doing a set_p[4um]d(), but it is not
 * needed for doing a set_pte().  We assume that only the *owner*
 * of a given allocation will be doing this for _their_
 * allocation.
 *
 * This ensures that once a system has been running for a while
 * and there have been stacks all over and these page tables
 * are fully populated, there will be no further acquisitions of
 * this lock.
 */
static DEFINE_SPINLOCK(shadow_table_allocation_lock);

Now I have my suspicions why that's not needed anymore upstream but I'd
let tglx explain better.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.