Re: [PATCH v3 1/2] memory_hotplug: Add a bounds check to check_hotplug_memory_range()

[Michal Hocko]

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@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx
> > 
> 
> Ok.
> 
> > > Signed-off-by: Alastair D'Silva <alastair@xxxxxxxxxxx>
> > > ---
> > >  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