From: Michal Hocko <mhocko@kernel.org>
To: Alastair D'Silva <alastair@d-silva.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@redhat.com>,
Oscar Salvador <osalvador@suse.com>,
Pavel Tatashin <pasha.tatashin@soleen.com>,
Dan Williams <dan.j.williams@intel.com>,
Wei Yang <richard.weiyang@gmail.com>, Qian Cai <cai@lca.pw>,
Jason Gunthorpe <jgg@ziepe.ca>,
Logan Gunthorpe <logang@deltatee.com>,
Ira Weiny <ira.weiny@intel.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] memory_hotplug: Add a bounds check to check_hotplug_memory_range()
Date: Tue, 24 Sep 2019 11:09:34 +0200 [thread overview]
Message-ID: <20190924090934.GF23050@dhcp22.suse.cz> (raw)
In-Reply-To: <25e0af4cb24a41466032d704b89d25559e7ad968.camel@d-silva.org>
On Tue 24-09-19 11:31:05, Alastair D'Silva wrote:
> On Mon, 2019-09-23 at 14:25 +0200, Michal Hocko wrote:
> > On Tue 17-09-19 11:07:47, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > On PowerPC, the address ranges allocated to OpenCAPI LPC memory
> > > are allocated from firmware. These address ranges may be higher
> > > than what older kernels permit, as we increased the maximum
> > > permissable address in commit 4ffe713b7587
> > > ("powerpc/mm: Increase the max addressable memory to 2PB"). It is
> > > possible that the addressable range may change again in the
> > > future.
> > >
> > > In this scenario, we end up with a bogus section returned from
> > > __section_nr (see the discussion on the thread "mm: Trigger bug on
> > > if a section is not found in __section_nr").
> > >
> > > Adding a check here means that we fail early and have an
> > > opportunity to handle the error gracefully, rather than rumbling
> > > on and potentially accessing an incorrect section.
> > >
> > > Further discussion is also on the thread ("powerpc: Perform a
> > > bounds
> > > check in arch_add_memory").
> >
> > It would be nicer to refer to this by a message-id based url.
> > E.g.
> > http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> >
>
> Ok.
>
> > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > ---
> > > include/linux/memory_hotplug.h | 1 +
> > > mm/memory_hotplug.c | 13 ++++++++++++-
> > > 2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/memory_hotplug.h
> > > b/include/linux/memory_hotplug.h
> > > index f46ea71b4ffd..bc477e98a310 100644
> > > --- a/include/linux/memory_hotplug.h
> > > +++ b/include/linux/memory_hotplug.h
> > > @@ -110,6 +110,7 @@ extern void
> > > __online_page_increment_counters(struct page *page);
> > > extern void __online_page_free(struct page *page);
> > >
> > > extern int try_online_node(int nid);
> > > +int check_hotplug_memory_addressable(u64 start, u64 size);
> > >
> > > extern int arch_add_memory(int nid, u64 start, u64 size,
> > > struct mhp_restrictions *restrictions);
> > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > index c73f09913165..02cb9a74f561 100644
> > > --- a/mm/memory_hotplug.c
> > > +++ b/mm/memory_hotplug.c
> > > @@ -1030,6 +1030,17 @@ int try_online_node(int nid)
> > > return ret;
> > > }
> > >
> > > +int check_hotplug_memory_addressable(u64 start, u64 size)
> > > +{
> > > +#ifdef MAX_PHYSMEM_BITS
> > > + if ((start + size - 1) >> MAX_PHYSMEM_BITS)
> > > + return -E2BIG;
> > > +#endif
> >
> > Is there any arch which doesn't define this? We seemed to be using
> > this
> > in sparsemem code without any ifdefs.
>
> A few, but none of them would be enabling hotplug (which depends on
> sparsemem), so you're right, the ifdef could be removed.
>
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> >
> > If you squashed the patch 2 then it would become clear why this needs
> > to
> > be exported because you would have a driver user. I find it a bit
> > unfortunate to expect that any driver which uses the hotplug code is
> > expected to know that this check should be called. This sounds too
> > error
> > prone. Why hasn't been this done at __add_pages layer?
> >
>
> It seemed that is should be a peer of check_hotplug_memory_range(), as
> it gives similar feedback (whether the provided range is suitable).
Well, that one seems to do a similar yet a different kind of check. It
imposes a constrain to the alignment of the memory that is hotplugable
via add_memory_resource - aka memory with user visible sysfs interface
and that really has some restrictions on the memory block sizes now.
> If we did the check in __add_pages, wouldn't we potentially lose bits
> from the LSBs of start & size, or is there some other requirement that
> already ensures start & size are always page aligned?
I do not really think we have to care about page unaligned addresses.
Callers down the road usually work with pfns.
> It appears this patch has been accepted - if we were to make this
> change, does it go as another spin on this series or a new series?
yes, the patch has been rushed to Linus unfortunatelly. Although I do
not really see any reason why. Sigh...
Anyway, now that it is in Linus' tree then we can only do a follow up on
top.
> > > +
> > > static int check_hotplug_memory_range(u64 start, u64 size)
> > > {
> > > /* memory range must be block size aligned */
> > > @@ -1040,7 +1051,7 @@ static int check_hotplug_memory_range(u64
> > > start, u64 size)
> > > return -EINVAL;
> > > }
> > >
> > > - return 0;
> > > + return check_hotplug_memory_addressable(start, size);
> >
> > This will result in a silent failure (unlike misaligned case). Is
> > this
> > what we want?
>
> Good point - I guess it comes down to, is there anything we expect an
> end user to do about it? I'm not sure there is, in which case the bad
> RC, which is reported up every call chain that I can see, should be
> sufficient.
It seems like a clear HW/platform bug to me. And that should better be
reported loudly to the log to make sure people do complain to their FW
friends and have it fixed.
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2019-09-24 9:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-17 1:07 [PATCH v3 0/2] Add bounds check for Hotplugged memory Alastair D'Silva
2019-09-17 1:07 ` [PATCH v3 1/2] memory_hotplug: Add a bounds check to check_hotplug_memory_range() Alastair D'Silva
2019-09-17 7:26 ` David Hildenbrand
2019-09-23 12:25 ` Michal Hocko
2019-09-24 1:31 ` Alastair D'Silva
2019-09-24 9:09 ` Michal Hocko [this message]
2019-09-24 9:13 ` David Hildenbrand
2019-09-24 11:45 ` Michal Hocko
2019-09-17 1:07 ` [PATCH v3 2/2] mm: Add a bounds check in devm_memremap_pages() Alastair D'Silva
2019-09-17 7:27 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190924090934.GF23050@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=alastair@d-silva.org \
--cc=cai@lca.pw \
--cc=dan.j.williams@intel.com \
--cc=david@redhat.com \
--cc=ira.weiny@intel.com \
--cc=jgg@ziepe.ca \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=logang@deltatee.com \
--cc=osalvador@suse.com \
--cc=pasha.tatashin@soleen.com \
--cc=richard.weiyang@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).