linux-kernel - Re: [PATCH v5 1/4] mm: hugetlb_vmemmap: introduce STRUCT_PAGE_SIZE_IS_POWER_OF

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>] [day] [month] [year] [list]

Message-ID: <CAMZfGtU1sbsrnCbCKi_By8VTGN1CA1s4A=s1NcHzRunDUpGXgQ@mail.gmail.com>
Date:   Tue, 29 Mar 2022 23:37:10 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Jonathan Corbet <corbet@....net>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Oscar Salvador <osalvador@...e.de>,
        David Hildenbrand <david@...hat.com>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Xiongchun duan <duanxiongchun@...edance.com>,
        Muchun Song <smuchun@...il.com>
Subject: Re: [PATCH v5 1/4] mm: hugetlb_vmemmap: introduce STRUCT_PAGE_SIZE_IS_POWER_OF_2

On Fri, Mar 25, 2022 at 1:11 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Wed, Mar 23, 2022 at 9:57 PM Muchun Song <songmuchun@...edance.com> wrote:
> >
> > If the size of "struct page" is not the power of two and this
> > feature is enabled, then the vmemmap pages of HugeTLB will be
> > corrupted after remapping (panic is about to happen in theory).
> > But this only exists when !CONFIG_MEMCG && !CONFIG_SLUB on
> > x86_64.  However, it is not a conventional configuration nowadays.
> > So it is not a real word issue, just the result of a code review.
> > But we have to prevent anyone from configuring that combined
> > configuration.  In order to avoid many checks like "is_power_of_2
> > (sizeof(struct page))" through mm/hugetlb_vmemmap.c.  Introduce
> > STRUCT_PAGE_SIZE_IS_POWER_OF_2 to detect if the size of struct
> > page is power of 2 and make this feature depends on this new
> > config.  Then we could prevent anyone do any unexpected
> > configuration.
> >
> > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > Suggested-by: Luis Chamberlain <mcgrof@...nel.org>
> > ---
> >  Kbuild                           | 14 ++++++++++++++
> >  fs/Kconfig                       |  1 +
> >  include/linux/mm_types.h         |  2 ++
> >  mm/Kconfig                       |  3 +++
> >  mm/hugetlb_vmemmap.c             |  6 ------
> >  mm/struct_page_size.c            | 19 +++++++++++++++++++
> >  scripts/check_struct_page_po2.sh |  9 +++++++++
> >  7 files changed, 48 insertions(+), 6 deletions(-)
> >  create mode 100644 mm/struct_page_size.c
> >  create mode 100755 scripts/check_struct_page_po2.sh
> >
> > diff --git a/Kbuild b/Kbuild
> > index fa441b98c9f6..21415c3b2728 100644
> > --- a/Kbuild
> > +++ b/Kbuild
> > @@ -37,6 +37,20 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
> >         $(call filechk,offsets,__ASM_OFFSETS_H__)
> >
> >  #####
> > +# Generate struct_page_size.h.
> > +
> > +struct_page_size-file := include/generated/struct_page_size.h
> > +
> > +always-y := $(struct_page_size-file)
> > +targets := mm/struct_page_size.s
> > +
> > +mm/struct_page_size.s: $(timeconst-file) $(bounds-file)
> > +
> > +$(struct_page_size-file): mm/struct_page_size.s FORCE
> > +       $(call filechk,offsets,__LINUX_STRUCT_PAGE_SIZE_H__)
> > +       $(Q)$(MAKE) -f $(srctree)/Makefile syncconfig
>
>
> No, please do not do this.
> It is terrible to feed back this to Kconfig again.

OK. I'll remove syncconfig.

>
> If you know this happens on !CONFIG_MEMCG && !CONFIG_SLUB on x86_64,
> why don't you add this dependency directly?

It is not enough since the size of the struct page also depends on
LAST_CPUPID_NOT_IN_PAGE_FLAGS && CONFIG_SLAB.
We cannot know the result of LAST_CPUPID_NOT_IN_PAGE_FLAGS in
Kconfig.

>
>
> If you want to avoid the run-time check,
> why don't you use  BUILD_BUG_ON() ?
>

Now I have another solution to avoid the run-time check.
We could use macro STRUCT_PAGE_SIZE_IS_POWER_OF_2
to do that like the following.

#ifdef STRUCT_PAGE_SIZE_IS_POWER_OF_2
/* code */
#endif

Thanks.