linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/1] Add bounds check for Hotplugged memory
@ 2019-09-26  1:34 Alastair D'Silva
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
  0 siblings, 1 reply; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-26  1:34 UTC (permalink / raw)
  To: alastair
  Cc: Andrew Morton, Oscar Salvador, Michal Hocko, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

From: Alastair D'Silva <alastair@d-silva.org>

This series adds bounds checks for hotplugged memory, ensuring that
it is within the physically addressable range (for platforms that
define MAX_(POSSIBLE_)PHYSMEM_BITS.

This allows for early failure, rather than attempting to access
bogus section numbers.

Changelog:
 V4:
   - Relocate call to __add_pages
   - Add a warning when the addressable check fails
 V3:
   - Perform the addressable check before we take the hotplug lock
 V2:
   - Don't use MAX_POSSIBLE_PHYSMEM_BITS as it's wider that what
     may be available

Alastair D'Silva (1):
  memory_hotplug: Add a bounds check to __add_pages

 mm/memory_hotplug.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 [PATCH v4 0/1] Add bounds check for Hotplugged memory Alastair D'Silva
@ 2019-09-26  1:34 ` Alastair D'Silva
  2019-09-26  7:12   ` David Hildenbrand
                     ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-26  1:34 UTC (permalink / raw)
  To: alastair
  Cc: Andrew Morton, Oscar Salvador, Michal Hocko, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

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")
http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 mm/memory_hotplug.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c73f09913165..212804c0f7f5 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
 	return 0;
 }
 
+static int check_hotplug_memory_addressable(unsigned long pfn,
+					    unsigned long nr_pages)
+{
+	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
+
+	if (max_addr >> MAX_PHYSMEM_BITS) {
+		WARN(1,
+		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
+		     pfn << PAGE_SHIFT, max_addr,
+		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
 /*
  * Reasonably generic function for adding memory.  It is
  * expected that archs that support memory hotplug will
@@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 	unsigned long nr, start_sec, end_sec;
 	struct vmem_altmap *altmap = restrictions->altmap;
 
+	err = check_hotplug_memory_addressable(pfn, nr_pages);
+	if (err)
+		return err;
+
 	if (altmap) {
 		/*
 		 * Validate altmap is within bounds of the total request
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
@ 2019-09-26  7:12   ` David Hildenbrand
  2019-09-26  7:37     ` David Hildenbrand
  2019-09-26  7:43     ` Michal Hocko
  2019-09-26  7:40   ` Oscar Salvador
                     ` (3 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26  7:12 UTC (permalink / raw)
  To: Alastair D'Silva, alastair
  Cc: Andrew Morton, Oscar Salvador, Michal Hocko, Pavel Tatashin,
	Dan Williams, linux-mm, linux-kernel

On 26.09.19 03:34, 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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>  	return 0;
>  }
>  
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> +					    unsigned long nr_pages)
> +{
> +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> +
> +	if (max_addr >> MAX_PHYSMEM_BITS) {
> +		WARN(1,
> +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> +		     pfn << PAGE_SHIFT, max_addr,
> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Reasonably generic function for adding memory.  It is
>   * expected that archs that support memory hotplug will
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  	unsigned long nr, start_sec, end_sec;
>  	struct vmem_altmap *altmap = restrictions->altmap;
>  
> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> +	if (err)
> +		return err;
> +
>  	if (altmap) {
>  		/*
>  		 * Validate altmap is within bounds of the total request
> 


I know Michal suggested this, but I still prefer checking early instead
of when we're knees-deep into adding of memory. But as I don't have any
power here, the code looks fine, although I consider the computations in
check_hotplug_memory_addressable() fairly ugly.

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:12   ` David Hildenbrand
@ 2019-09-26  7:37     ` David Hildenbrand
  2019-09-26  7:43     ` Michal Hocko
  1 sibling, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26  7:37 UTC (permalink / raw)
  To: Alastair D'Silva, alastair
  Cc: Andrew Morton, Oscar Salvador, Michal Hocko, Pavel Tatashin,
	Dan Williams, linux-mm, linux-kernel

On 26.09.19 09:12, David Hildenbrand wrote:
> On 26.09.19 03:34, 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")
>> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>>
>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>> ---
>>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index c73f09913165..212804c0f7f5 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>>  	return 0;
>>  }
>>  
>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>> +					    unsigned long nr_pages)
>> +{
>> +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
>> +
>> +	if (max_addr >> MAX_PHYSMEM_BITS) {
>> +		WARN(1,
>> +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>> +		     pfn << PAGE_SHIFT, max_addr,
>> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>> +		return -E2BIG;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Reasonably generic function for adding memory.  It is
>>   * expected that archs that support memory hotplug will
>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>  	unsigned long nr, start_sec, end_sec;
>>  	struct vmem_altmap *altmap = restrictions->altmap;
>>  
>> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
>> +	if (err)
>> +		return err;
>> +
>>  	if (altmap) {
>>  		/*
>>  		 * Validate altmap is within bounds of the total request
>>
> 
> 
> I know Michal suggested this, but I still prefer checking early instead
> of when we're knees-deep into adding of memory. But as I don't have any
> power here, the code looks fine, although I consider the computations in
> check_hotplug_memory_addressable() fairly ugly.
> 

Forgot to add

Acked-by: David Hildenbrand <david@redhat.com>

:)

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
  2019-09-26  7:12   ` David Hildenbrand
@ 2019-09-26  7:40   ` Oscar Salvador
  2019-09-26  7:42     ` David Hildenbrand
  2019-09-26  7:47     ` Michal Hocko
  2019-09-26  7:44   ` Michal Hocko
                     ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Oscar Salvador @ 2019-09-26  7:40 UTC (permalink / raw)
  To: Alastair D'Silva
  Cc: alastair, Andrew Morton, Michal Hocko, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  	unsigned long nr, start_sec, end_sec;
>  	struct vmem_altmap *altmap = restrictions->altmap;
>  
> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> +	if (err)
> +		return err;
> +

I am probably off here because 1) I am jumping blind in a middle of a discussion and
2) I got back from holydays yesterday, so bear with me.

Would not be better to just place the check in add_memory_resource instead?
Take into account that we create the memory mapping for this range in
arch_add_memory, so it looks weird to me to create the mapping if we are going to
fail right after because the range is simply off.

But as I said, I might be missing some previous discussion. 

-- 
Oscar Salvador
SUSE L3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:40   ` Oscar Salvador
@ 2019-09-26  7:42     ` David Hildenbrand
  2019-09-26  7:47     ` Michal Hocko
  1 sibling, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26  7:42 UTC (permalink / raw)
  To: Oscar Salvador, Alastair D'Silva
  Cc: alastair, Andrew Morton, Michal Hocko, Pavel Tatashin,
	Dan Williams, linux-mm, linux-kernel

On 26.09.19 09:40, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
>> From: Alastair D'Silva <alastair@d-silva.org>
>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>  	unsigned long nr, start_sec, end_sec;
>>  	struct vmem_altmap *altmap = restrictions->altmap;
>>  
>> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
>> +	if (err)
>> +		return err;
>> +
> 
> I am probably off here because 1) I am jumping blind in a middle of a discussion and
> 2) I got back from holydays yesterday, so bear with me.
> 
> Would not be better to just place the check in add_memory_resource instead?

At least devmem/memremap needs special handling.

> Take into account that we create the memory mapping for this range in
> arch_add_memory, so it looks weird to me to create the mapping if we are going to
> fail right after because the range is simply off.
> 
> But as I said, I might be missing some previous discussion. 
> 


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:12   ` David Hildenbrand
  2019-09-26  7:37     ` David Hildenbrand
@ 2019-09-26  7:43     ` Michal Hocko
  2019-09-26  7:46       ` David Hildenbrand
  1 sibling, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2019-09-26  7:43 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Alastair D'Silva, alastair, Andrew Morton, Oscar Salvador,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
> On 26.09.19 03:34, 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")
> > http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> > 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  mm/memory_hotplug.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index c73f09913165..212804c0f7f5 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> >  	return 0;
> >  }
> >  
> > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > +					    unsigned long nr_pages)
> > +{
> > +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> > +
> > +	if (max_addr >> MAX_PHYSMEM_BITS) {
> > +		WARN(1,
> > +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> > +		     pfn << PAGE_SHIFT, max_addr,
> > +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> > +		return -E2BIG;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Reasonably generic function for adding memory.  It is
> >   * expected that archs that support memory hotplug will
> > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> >  	unsigned long nr, start_sec, end_sec;
> >  	struct vmem_altmap *altmap = restrictions->altmap;
> >  
> > +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> > +	if (err)
> > +		return err;
> > +
> >  	if (altmap) {
> >  		/*
> >  		 * Validate altmap is within bounds of the total request
> > 
> 
> 
> I know Michal suggested this, but I still prefer checking early instead
> of when we're knees-deep into adding of memory.

What is your concern here? Unwinding the state should be pretty
straightfoward from this failure path.

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
  2019-09-26  7:12   ` David Hildenbrand
  2019-09-26  7:40   ` Oscar Salvador
@ 2019-09-26  7:44   ` Michal Hocko
  2019-09-26  7:53   ` Oscar Salvador
  2019-09-26 15:35   ` kbuild test robot
  4 siblings, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2019-09-26  7:44 UTC (permalink / raw)
  To: Alastair D'Silva
  Cc: alastair, Andrew Morton, Oscar Salvador, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On Thu 26-09-19 11:34:05, 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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>

Yes, this looks better to me. E2BIG is a new error code for this path
but no callers seem to be deeply concerned about a specific error codes
so this should be safe.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>  	return 0;
>  }
>  
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> +					    unsigned long nr_pages)
> +{
> +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> +
> +	if (max_addr >> MAX_PHYSMEM_BITS) {
> +		WARN(1,
> +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> +		     pfn << PAGE_SHIFT, max_addr,
> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Reasonably generic function for adding memory.  It is
>   * expected that archs that support memory hotplug will
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  	unsigned long nr, start_sec, end_sec;
>  	struct vmem_altmap *altmap = restrictions->altmap;
>  
> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> +	if (err)
> +		return err;
> +
>  	if (altmap) {
>  		/*
>  		 * Validate altmap is within bounds of the total request
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:43     ` Michal Hocko
@ 2019-09-26  7:46       ` David Hildenbrand
  2019-09-27  6:33         ` Alastair D'Silva
  0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26  7:46 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Alastair D'Silva, alastair, Andrew Morton, Oscar Salvador,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On 26.09.19 09:43, Michal Hocko wrote:
> On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
>> On 26.09.19 03:34, 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")
>>> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>>>
>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>>> ---
>>>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index c73f09913165..212804c0f7f5 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>>>  	return 0;
>>>  }
>>>  
>>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>>> +					    unsigned long nr_pages)
>>> +{
>>> +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
>>> +
>>> +	if (max_addr >> MAX_PHYSMEM_BITS) {
>>> +		WARN(1,
>>> +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>>> +		     pfn << PAGE_SHIFT, max_addr,
>>> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>>> +		return -E2BIG;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  /*
>>>   * Reasonably generic function for adding memory.  It is
>>>   * expected that archs that support memory hotplug will
>>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>>  	unsigned long nr, start_sec, end_sec;
>>>  	struct vmem_altmap *altmap = restrictions->altmap;
>>>  
>>> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
>>> +	if (err)
>>> +		return err;
>>> +
>>>  	if (altmap) {
>>>  		/*
>>>  		 * Validate altmap is within bounds of the total request
>>>
>>
>>
>> I know Michal suggested this, but I still prefer checking early instead
>> of when we're knees-deep into adding of memory.
> 
> What is your concern here? Unwinding the state should be pretty
> straightfoward from this failure path.

Just the general "check what you can check early without locks"
approach. But yeah, this series is probably not worth a v5, so I can
live with this change just fine :)


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:40   ` Oscar Salvador
  2019-09-26  7:42     ` David Hildenbrand
@ 2019-09-26  7:47     ` Michal Hocko
  1 sibling, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2019-09-26  7:47 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: Alastair D'Silva, alastair, Andrew Morton, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On Thu 26-09-19 09:40:05, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> >  	unsigned long nr, start_sec, end_sec;
> >  	struct vmem_altmap *altmap = restrictions->altmap;
> >  
> > +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> > +	if (err)
> > +		return err;
> > +
> 
> I am probably off here because 1) I am jumping blind in a middle of a discussion and
> 2) I got back from holydays yesterday, so bear with me.
> 
> Would not be better to just place the check in add_memory_resource instead?

This was the previous version of the patch. The argument is that we do
not want each add_pages user to think of this special handling.
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
                     ` (2 preceding siblings ...)
  2019-09-26  7:44   ` Michal Hocko
@ 2019-09-26  7:53   ` Oscar Salvador
  2019-09-27  5:14     ` Alastair D'Silva
  2019-09-26 15:35   ` kbuild test robot
  4 siblings, 1 reply; 15+ messages in thread
From: Oscar Salvador @ 2019-09-26  7:53 UTC (permalink / raw)
  To: Alastair D'Silva
  Cc: alastair, Andrew Morton, Michal Hocko, David Hildenbrand,
	Pavel Tatashin, Dan Williams, linux-mm, linux-kernel

On Thu, Sep 26, 2019 at 11:34:05AM +1000, 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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

Just a nit-picking below:

> ---
>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>  	return 0;
>  }
>  
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> +					    unsigned long nr_pages)
> +{
> +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;

I would use PFN_PHYS instead:

	unsigned long max_addr = PFN_PHYS(pfn + nr_pages) - 1;

> +
> +	if (max_addr >> MAX_PHYSMEM_BITS) {
> +		WARN(1,
> +		     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> +		     pfn << PAGE_SHIFT, max_addr,

Same here.

> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);

I would use a local variable to hold this computation.

> +		return -E2BIG;
> +	}
> +
> +	return 0;

-- 
Oscar Salvador
SUSE L3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
                     ` (3 preceding siblings ...)
  2019-09-26  7:53   ` Oscar Salvador
@ 2019-09-26 15:35   ` kbuild test robot
  4 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2019-09-26 15:35 UTC (permalink / raw)
  To: Alastair D'Silva
  Cc: kbuild-all, alastair, Andrew Morton, Oscar Salvador,
	Michal Hocko, David Hildenbrand, Pavel Tatashin, Dan Williams,
	linux-mm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2637 bytes --]

Hi Alastair,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3 next-20190925]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alastair-D-Silva/memory_hotplug-Add-a-bounds-check-to-__add_pages/20190926-094437
config: i386-randconfig-g004-201938 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   mm/memory_hotplug.c: In function 'check_hotplug_memory_addressable':
>> mm/memory_hotplug.c:286:15: warning: right shift count >= width of type [-Wshift-count-overflow]
     if (max_addr >> MAX_PHYSMEM_BITS) {
                  ^~
   In file included from arch/x86/include/asm/bug.h:83:0,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from mm/memory_hotplug.c:9:
>> mm/memory_hotplug.c:290:13: warning: left shift count >= width of type [-Wshift-count-overflow]
           (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
                ^
   include/asm-generic/bug.h:112:21: note: in definition of macro '__WARN_printf_taint'
     do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
                        ^~~
   include/asm-generic/bug.h:135:3: note: in expansion of macro '__WARN_printf'
      __WARN_printf(format);     \
      ^~~~~~~~~~~~~
>> mm/memory_hotplug.c:287:3: note: in expansion of macro 'WARN'
      WARN(1,
      ^~~~

vim +286 mm/memory_hotplug.c

   280	
   281	static int check_hotplug_memory_addressable(unsigned long pfn,
   282						    unsigned long nr_pages)
   283	{
   284		unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
   285	
 > 286		if (max_addr >> MAX_PHYSMEM_BITS) {
 > 287			WARN(1,
   288			     "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
   289			     pfn << PAGE_SHIFT, max_addr,
 > 290			     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
   291			return -E2BIG;
   292		}
   293	
   294		return 0;
   295	}
   296	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31679 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:53   ` Oscar Salvador
@ 2019-09-27  5:14     ` Alastair D'Silva
  0 siblings, 0 replies; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-27  5:14 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: Andrew Morton, Michal Hocko, David Hildenbrand, Pavel Tatashin,
	Dan Williams, linux-mm, linux-kernel

On Thu, 2019-09-26 at 09:53 +0200, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +1000, 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")
> > http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> > 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> 
> Just a nit-picking below:
> 
> > ---
> >  mm/memory_hotplug.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index c73f09913165..212804c0f7f5 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn,
> > unsigned long nr_pages,
> >  	return 0;
> >  }
> >  
> > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > +					    unsigned long nr_pages)
> > +{
> > +	unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> 
> I would use PFN_PHYS instead:
> 
> 	unsigned long max_addr = PFN_PHYS(pfn + nr_pages) - 1;
> 
> > +
> > +	if (max_addr >> MAX_PHYSMEM_BITS) {
> > +		WARN(1,
> > +		     "Hotplugged memory exceeds maximum addressable
> > address, range=%#lx-%#lx, maximum=%#lx\n",
> > +		     pfn << PAGE_SHIFT, max_addr,
> 
> Same here.
> 
> > +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> 
> I would use a local variable to hold this computation.
> 
> > +		return -E2BIG;
> > +	}
> > +
> > +	return 0;


Looks like I'll have to do another spin to change that to a ull anyway,
so I'll implement those suggestions.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org




^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-26  7:46       ` David Hildenbrand
@ 2019-09-27  6:33         ` Alastair D'Silva
  2019-09-27  7:24           ` David Hildenbrand
  0 siblings, 1 reply; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-27  6:33 UTC (permalink / raw)
  To: David Hildenbrand, Michal Hocko
  Cc: Andrew Morton, Oscar Salvador, Pavel Tatashin, Dan Williams,
	linux-mm, linux-kernel

On Thu, 2019-09-26 at 09:46 +0200, David Hildenbrand wrote:
> On 26.09.19 09:43, Michal Hocko wrote:
> > On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
> > > On 26.09.19 03:34, 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")
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20190827052047.31547-2D1-2Dalastair-40au1.ibm.com&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=cT4tgeEQ0Ll3SIlZDHE5AEXyKy6uKADMtf9_Eb7-vec&m=p9ZS4kSnvF0zq81jcCFd2nYj1zfTMvfbApCtmKI2KNA&s=yif-duzz_RESW3LUyU_0kkmefRAnKWjjn_p5Et-9B2g&e= 
> > > > 
> > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > > ---
> > > >  mm/memory_hotplug.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > > 
> > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > index c73f09913165..212804c0f7f5 100644
> > > > --- a/mm/memory_hotplug.c
> > > > +++ b/mm/memory_hotplug.c
> > > > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long
> > > > pfn, unsigned long nr_pages,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > > > +					    unsigned long
> > > > nr_pages)
> > > > +{
> > > > +	unsigned long max_addr = ((pfn + nr_pages) <<
> > > > PAGE_SHIFT) - 1;
> > > > +
> > > > +	if (max_addr >> MAX_PHYSMEM_BITS) {
> > > > +		WARN(1,
> > > > +		     "Hotplugged memory exceeds maximum
> > > > addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> > > > +		     pfn << PAGE_SHIFT, max_addr,
> > > > +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> > > > +		return -E2BIG;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Reasonably generic function for adding memory.  It is
> > > >   * expected that archs that support memory hotplug will
> > > > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned
> > > > long pfn, unsigned long nr_pages,
> > > >  	unsigned long nr, start_sec, end_sec;
> > > >  	struct vmem_altmap *altmap = restrictions->altmap;
> > > >  
> > > > +	err = check_hotplug_memory_addressable(pfn, nr_pages);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > >  	if (altmap) {
> > > >  		/*
> > > >  		 * Validate altmap is within bounds of the
> > > > total request
> > > > 
> > > 
> > > I know Michal suggested this, but I still prefer checking early
> > > instead
> > > of when we're knees-deep into adding of memory.
> > 
> > What is your concern here? Unwinding the state should be pretty
> > straightfoward from this failure path.
> 
> Just the general "check what you can check early without locks"
> approach. But yeah, this series is probably not worth a v5, so I can
> live with this change just fine :)
> 
> 

I'm going to spin a V5 anyway - where were you suggesting?

> -- 
> 
> Thanks,
> 
> David / dhildenb

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
  2019-09-27  6:33         ` Alastair D'Silva
@ 2019-09-27  7:24           ` David Hildenbrand
  0 siblings, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-27  7:24 UTC (permalink / raw)
  To: Alastair D'Silva, Michal Hocko
  Cc: Andrew Morton, Oscar Salvador, Pavel Tatashin, Dan Williams,
	linux-mm, linux-kernel

On 27.09.19 08:33, Alastair D'Silva wrote:
> On Thu, 2019-09-26 at 09:46 +0200, David Hildenbrand wrote:
>> On 26.09.19 09:43, Michal Hocko wrote:
>>> On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
>>>> On 26.09.19 03:34, 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")
>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20190827052047.31547-2D1-2Dalastair-40au1.ibm.com&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=cT4tgeEQ0Ll3SIlZDHE5AEXyKy6uKADMtf9_Eb7-vec&m=p9ZS4kSnvF0zq81jcCFd2nYj1zfTMvfbApCtmKI2KNA&s=yif-duzz_RESW3LUyU_0kkmefRAnKWjjn_p5Et-9B2g&e= 
>>>>>
>>>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>>>>> ---
>>>>>  mm/memory_hotplug.c | 20 ++++++++++++++++++++
>>>>>  1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>>> index c73f09913165..212804c0f7f5 100644
>>>>> --- a/mm/memory_hotplug.c
>>>>> +++ b/mm/memory_hotplug.c
>>>>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long
>>>>> pfn, unsigned long nr_pages,
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>>>>> +					    unsigned long
>>>>> nr_pages)
>>>>> +{
>>>>> +	unsigned long max_addr = ((pfn + nr_pages) <<
>>>>> PAGE_SHIFT) - 1;
>>>>> +
>>>>> +	if (max_addr >> MAX_PHYSMEM_BITS) {
>>>>> +		WARN(1,
>>>>> +		     "Hotplugged memory exceeds maximum
>>>>> addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>>>>> +		     pfn << PAGE_SHIFT, max_addr,
>>>>> +		     (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>>>>> +		return -E2BIG;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Reasonably generic function for adding memory.  It is
>>>>>   * expected that archs that support memory hotplug will
>>>>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned
>>>>> long pfn, unsigned long nr_pages,
>>>>>  	unsigned long nr, start_sec, end_sec;
>>>>>  	struct vmem_altmap *altmap = restrictions->altmap;
>>>>>  
>>>>> +	err = check_hotplug_memory_addressable(pfn, nr_pages);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>>  	if (altmap) {
>>>>>  		/*
>>>>>  		 * Validate altmap is within bounds of the
>>>>> total request
>>>>>
>>>>
>>>> I know Michal suggested this, but I still prefer checking early
>>>> instead
>>>> of when we're knees-deep into adding of memory.
>>>
>>> What is your concern here? Unwinding the state should be pretty
>>> straightfoward from this failure path.
>>
>> Just the general "check what you can check early without locks"
>> approach. But yeah, this series is probably not worth a v5, so I can
>> live with this change just fine :)
>>
>>
> 
> I'm going to spin a V5 anyway - where were you suggesting?

I preferred the previous places where we checked, but I think we settled
on __add_pages(). So I am fine with the changes Oscar proposed. You
might want to turn "max_addr" into a const if you feel fancy. :)

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-09-27  7:24 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26  1:34 [PATCH v4 0/1] Add bounds check for Hotplugged memory Alastair D'Silva
2019-09-26  1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
2019-09-26  7:12   ` David Hildenbrand
2019-09-26  7:37     ` David Hildenbrand
2019-09-26  7:43     ` Michal Hocko
2019-09-26  7:46       ` David Hildenbrand
2019-09-27  6:33         ` Alastair D'Silva
2019-09-27  7:24           ` David Hildenbrand
2019-09-26  7:40   ` Oscar Salvador
2019-09-26  7:42     ` David Hildenbrand
2019-09-26  7:47     ` Michal Hocko
2019-09-26  7:44   ` Michal Hocko
2019-09-26  7:53   ` Oscar Salvador
2019-09-27  5:14     ` Alastair D'Silva
2019-09-26 15:35   ` kbuild test robot

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).