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: <20210427083019.110184-1-wangwensheng4@huawei.com>
 <e838d8b5-84f1-5532-6f22-e4b729124e1c@redhat.com> <20210623230939.GA2963480@hori.linux.bs1.fc.nec.co.jp>
In-Reply-To: <20210623230939.GA2963480@hori.linux.bs1.fc.nec.co.jp>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Fri, 25 Jun 2021 14:23:05 -0700
Message-ID: <CAPcyv4h5a5AYscsyC40_5bc6j1kmjMFWJ_0MFAGEx1EPS9Tmrw@mail.gmail.com>
Subject: Re: [PATCH] mm/sparse: Fix flags overlap in section_mem_map
To:     =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= 
        <naoya.horiguchi@nec.com>
Cc:     Wang Wensheng <wangwensheng4@huawei.com>,
        David Hildenbrand <david@redhat.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "osalvador@suse.de" <osalvador@suse.de>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "rui.xiang@huawei.com" <rui.xiang@huawei.com>,
        =?UTF-8?B?SEFHSU8gS0FaVUhJVE8o6JCp5bC+IOS4gOS7gSk=?= 
        <k-hagio-ab@nec.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 23, 2021 at 4:10 PM HORIGUCHI NAOYA(=E5=A0=80=E5=8F=A3=E3=80=80=
=E7=9B=B4=E4=B9=9F)
<naoya.horiguchi@nec.com> wrote:
>
> On Tue, Apr 27, 2021 at 11:05:17AM +0200, David Hildenbrand wrote:
> > On 27.04.21 10:30, Wang Wensheng wrote:
> > > The section_mem_map member of struct mem_section stores some flags an=
d
> > > the address of struct page array of the mem_section.
> > >
> > > Additionally the node id of the mem_section is stored during early bo=
ot,
> > > where the struct page array has not been allocated. In other words, t=
he
> > > higher bits of section_mem_map are used for two purpose, and the node=
 id
> > > should be clear properly after the early boot.
> > >
> > > Currently the node id field is overlapped with the flag field and can=
not
> > > be clear properly. That overlapped bits would then be treated as
> > > mem_section flags and may lead to unexpected side effects.
> > >
> > > Define SECTION_NID_SHIFT using order_base_2 to ensure that the node i=
d
> > > field always locates after flags field. That's why the overlap occurs=
 -
> > > forgetting to increase SECTION_NID_SHIFT when adding new mem_section
> > > flag.
> > >
> > > Fixes: 326e1b8f83a4 ("mm/sparsemem: introduce a SECTION_IS_EARLY flag=
")
> > > Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com>
> > > ---
> > >   include/linux/mmzone.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index 47946ce..b01694d 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -1325,7 +1325,7 @@ extern size_t mem_section_usage_size(void);
> > >   #define SECTION_TAINT_ZONE_DEVICE (1UL<<4)
> > >   #define SECTION_MAP_LAST_BIT              (1UL<<5)
> > >   #define SECTION_MAP_MASK          (~(SECTION_MAP_LAST_BIT-1))
> > > -#define SECTION_NID_SHIFT          3
> > > +#define SECTION_NID_SHIFT          order_base_2(SECTION_MAP_LAST_BIT=
)
> > >   static inline struct page *__section_mem_map_addr(struct mem_sectio=
n *section)
> > >   {
> > >
> >
> > Well, all sections around during boot that have an early NID are early =
...
> > so it's not an issue with SECTION_IS_EARLY, no? I mean, it's ugly, but =
not
> > broken.
> >
> > But it's an issue with SECTION_TAINT_ZONE_DEVICE, AFAIKT.
> > sparse_init_one_section() would leave the bit set if the nid happens to=
 have
> > that bit set (e.g., node 2,3). It's semi-broken then, because we force =
all
> > pfn_to_online_page() through the slow path.
> >
> >
> > That whole section flag setting code is fragile.
>
> Hi Wensheng, David,
>
> This patch seems not accepted or updated yet, so what's going on?
>
> We recently saw the exact issue on testing crash utilities with latest
> kernels on 4 NUMA node system.  SECTION_TAINT_ZONE_DEVICE seems to be
> set wrongly, and makedumpfile could fail due to this. So we need a fix.
>
> I thought of another approach like below before finding this thread,
> so if you are fine, I'd like to submit a patch with this. And if you
> like going with order_base_2() approach, I'm glad to ack this patch.
>
>   --- a/include/linux/mmzone.h
>   +++ b/include/linux/mmzone.h
>   @@ -1358,14 +1358,15 @@ extern size_t mem_section_usage_size(void);
>     *      which results in PFN_SECTION_SHIFT equal 6.
>     * To sum it up, at least 6 bits are available.
>     */
>   +#define SECTION_MAP_LAST_SHIFT         5
>    #define SECTION_MARKED_PRESENT         (1UL<<0)
>    #define SECTION_HAS_MEM_MAP            (1UL<<1)
>    #define SECTION_IS_ONLINE              (1UL<<2)
>    #define SECTION_IS_EARLY               (1UL<<3)
>    #define SECTION_TAINT_ZONE_DEVICE      (1UL<<4)
>   -#define SECTION_MAP_LAST_BIT           (1UL<<5)
>   +#define SECTION_MAP_LAST_BIT           (1UL<<SECTION_MAP_LAST_SHIFT)
>    #define SECTION_MAP_MASK               (~(SECTION_MAP_LAST_BIT-1))
>   -#define SECTION_NID_SHIFT              3
>   +#define SECTION_NID_SHIFT              SECTION_MAP_LAST_SHIFT

Rather than make it dynamic, why not just make it 6 directly since
that matches the comment about the maximum number of flags available.