Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220120042513.1648831-1-pasha.tatashin@soleen.com>
 <20220120042513.1648831-3-pasha.tatashin@soleen.com> <CAAPL-u_kP5E5iOtyztsXLJG0YNFzganD3djr2nW0vTkUpv=A9Q@mail.gmail.com>
In-Reply-To: <CAAPL-u_kP5E5iOtyztsXLJG0YNFzganD3djr2nW0vTkUpv=A9Q@mail.gmail.com>
From:   Pasha Tatashin <pasha.tatashin@soleen.com>
Date:   Thu, 20 Jan 2022 13:25:45 -0500
Message-ID: <CA+CK2bC5WJxdb9tdZj8eiB1PeOfV-D4v1Tpi+TEZhMskbZ6k4A@mail.gmail.com>
Subject: Re: [PATCH 2/3] mm/page_table_check: check entries at pud and pmd levels
To:     Wei Xu <weixugc@google.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Rientjes <rientjes@google.com>,
        Paul Turner <pjt@google.com>, Greg Thelen <gthelen@google.com>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Fusion Future <qydwhotmail@gmail.com>,
        Hugh Dickins <hughd@google.com>, Zi Yan <ziy@nvidia.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Wei,

Thank you for looking at this patch.

> > +static void pmd_clear_level(struct mm_struct *mm, unsigned long addr,
> > +                           pmd_t *pmdp)
> > +{
> > +       unsigned long i;
> > +
> > +       for (i = 0; i < PTRS_PER_PMD; i++) {
> > +               pmd_t old_pmd = *pmdp;
> > +
> > +               if (pmd_user_accessible_page(old_pmd)) {
> > +                       page_table_check_clear(mm, addr, pmd_pfn(old_pmd),
> > +                                              PMD_PAGE_SIZE >> PAGE_SHIFT);
> > +               } else if (!pmd_bad(old_pmd) && !pmd_leaf(old_pmd)) {
> > +                       pte_t *ptep = pte_offset_map(&old_pmd, addr);
> > +
> > +                       pte_clear_level(mm, addr, ptep);
> > +                       pte_unmap(ptep);
> > +               }
>
> You can call __page_table_check_pmd_clear(mm, addr, old_pmd, addr)
> instead to share the new code.

Yeap.

>
> >
> > +               addr += PMD_PAGE_SIZE;
> > +               pmdp++;
> > +       }
> > +}
> > +
> >  /*
> >   * page is on free list, or is being allocated, verify that counters are zeroes
> >   * crash if they are not.
> > @@ -186,6 +220,11 @@ void __page_table_check_pmd_clear(struct mm_struct *mm, unsigned long addr,
> >         if (pmd_user_accessible_page(pmd)) {
> >                 page_table_check_clear(mm, addr, pmd_pfn(pmd),
> >                                        PMD_PAGE_SIZE >> PAGE_SHIFT);
> > +       } else if (!pmd_bad(pmd) && !pmd_leaf(pmd)) {
> > +               pte_t *ptep = pte_offset_map(&pmd, addr);
> > +
> > +               pte_clear_level(mm, addr, ptep);
> > +               pte_unmap(ptep);
> >         }
> >  }
> >  EXPORT_SYMBOL(__page_table_check_pmd_clear);
> > @@ -199,6 +238,10 @@ void __page_table_check_pud_clear(struct mm_struct *mm, unsigned long addr,
> >         if (pud_user_accessible_page(pud)) {
> >                 page_table_check_clear(mm, addr, pud_pfn(pud),
> >                                        PUD_PAGE_SIZE >> PAGE_SHIFT);
> > +       } else if (!pud_bad(pud) && !pud_leaf(pud)) {
> > +               pmd_t *pmdp = pmd_offset(&pud, addr);
> > +
> > +               pmd_clear_level(mm, addr, pmdp);
> >         }
> >  }
> >  EXPORT_SYMBOL(__page_table_check_pud_clear);
> > @@ -237,6 +280,11 @@ void __page_table_check_pmd_set(struct mm_struct *mm, unsigned long addr,
> >         if (pmd_user_accessible_page(old_pmd)) {
> >                 page_table_check_clear(mm, addr, pmd_pfn(old_pmd),
> >                                        PMD_PAGE_SIZE >> PAGE_SHIFT);
> > +       } else if (!pmd_bad(old_pmd) && !pmd_leaf(old_pmd)) {
> > +               pte_t *ptep = pte_offset_map(&old_pmd, addr);
> > +
> > +               pte_clear_level(mm, addr, ptep);
> > +               pte_unmap(ptep);
> >         }
> >
>
> How about replacing the above code with
> __page_table_check_pmd_clear(mm, addr, old_pmd)?

OK

>
> >         if (pmd_user_accessible_page(pmd)) {
> > @@ -259,6 +307,10 @@ void __page_table_check_pud_set(struct mm_struct *mm, unsigned long addr,
> >         if (pud_user_accessible_page(old_pud)) {
> >                 page_table_check_clear(mm, addr, pud_pfn(old_pud),
> >                                        PUD_PAGE_SIZE >> PAGE_SHIFT);
> > +       } else if (!pud_bad(old_pud) && !pud_leaf(old_pud)) {
> > +               pmd_t *pmdp = pmd_offset(&old_pud, addr);
> > +
> > +               pmd_clear_level(mm, addr, pmdp);
> >         }
>
> Replacing with __page_table_check_pud_clear(mm, addr, old_pud)?

OK

Good suggestions, I will simplify the code with your suggestions.

Pasha