[PATCH 0/2] Add bounds check for Hotplugged memory

[PATCH 0/2] Add bounds check for Hotplugged memory

[Alastair D'Silva]

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.

Alastair D'Silva (2):
  memory_hotplug: Add a bounds check to check_hotplug_memory_range()
  mm: Add a bounds check in devm_memremap_pages()

 include/linux/memory_hotplug.h |  1 +
 mm/memory_hotplug.c            | 19 ++++++++++++++++++-
 mm/memremap.c                  |  8 ++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.21.0

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

[Alastair D'Silva]

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

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 include/linux/memory_hotplug.h |  1 +
 mm/memory_hotplug.c            | 19 ++++++++++++++++++-
 2 files changed, 19 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..3c5428b014f9 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1030,6 +1030,23 @@ int try_online_node(int nid)
 	return ret;
 }
 
+#ifndef MAX_POSSIBLE_PHYSMEM_BITS
+#ifdef MAX_PHYSMEM_BITS
+#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS
+#endif
+#endif
+
+int check_hotplug_memory_addressable(u64 start, u64 size)
+{
+#ifdef MAX_POSSIBLE_PHYSMEM_BITS
+	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
+		return -E2BIG;
+#endif
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
+
 static int check_hotplug_memory_range(u64 start, u64 size)
 {
 	/* memory range must be block size aligned */
@@ -1040,7 +1057,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
 		return -EINVAL;
 	}
 
-	return 0;
+	return check_hotplug_memory_addressable(start, size);
 }
 
 static int online_memory_block(struct memory_block *mem, void *arg)
-- 
2.21.0

[PATCH 2/2] mm: Add a bounds check in devm_memremap_pages()

[Alastair D'Silva]

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

The call to check_hotplug_memory_addressable() validates that the memory
is fully addressable.

Without this call, it is possible that we may remap pages that is
not physically addressable, resulting in bogus section numbers
being returned from __section_nr().

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

diff --git a/mm/memremap.c b/mm/memremap.c
index 86432650f829..fd00993caa3e 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -269,6 +269,13 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 
 	mem_hotplug_begin();
 
+	error = check_hotplug_memory_addressable(res->start,
+						 resource_size(res));
+	if (error) {
+		mem_hotplug_done();
+		goto err_checkrange;
+	}
+
 	/*
 	 * For device private memory we call add_pages() as we only need to
 	 * allocate and initialize struct page for the device memory. More-
@@ -324,6 +331,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 
  err_add_memory:
 	kasan_remove_zero_shadow(__va(res->start), resource_size(res));
+ err_checkrange:
  err_kasan:
 	untrack_pfn(NULL, PHYS_PFN(res->start), resource_size(res));
  err_pfn_remap:
-- 
2.21.0

Re: [PATCH 2/2] mm: Add a bounds check in devm_memremap_pages()

[David Hildenbrand]

On 10.09.19 04:52, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> The call to check_hotplug_memory_addressable() validates that the memory
> is fully addressable.
> 
> Without this call, it is possible that we may remap pages that is
> not physically addressable, resulting in bogus section numbers
> being returned from __section_nr().
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/memremap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 86432650f829..fd00993caa3e 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -269,6 +269,13 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  
>  	mem_hotplug_begin();
>  
> +	error = check_hotplug_memory_addressable(res->start,
> +						 resource_size(res));
> +	if (error) {
> +		mem_hotplug_done();
> +		goto err_checkrange;
> +	}
> +

No need to check under the memory hotplug lock.

>  	/*
>  	 * For device private memory we call add_pages() as we only need to
>  	 * allocate and initialize struct page for the device memory. More-
> @@ -324,6 +331,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  
>   err_add_memory:
>  	kasan_remove_zero_shadow(__va(res->start), resource_size(res));
> + err_checkrange:
>   err_kasan:
>  	untrack_pfn(NULL, PHYS_PFN(res->start), resource_size(res));
>   err_pfn_remap:
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 10.09.19 04:52, 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").
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  include/linux/memory_hotplug.h |  1 +
>  mm/memory_hotplug.c            | 19 ++++++++++++++++++-
>  2 files changed, 19 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..3c5428b014f9 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1030,6 +1030,23 @@ int try_online_node(int nid)
>  	return ret;
>  }
>  
> +#ifndef MAX_POSSIBLE_PHYSMEM_BITS
> +#ifdef MAX_PHYSMEM_BITS
> +#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS
> +#endif
> +#endif
> +

I think using MAX_POSSIBLE_PHYSMEM_BITS bits is wrong. You should use
MAX_PHYSMEM_BITS.

E.g. on x86_64, MAX_POSSIBLE_PHYSMEM_BITS is 52, while MAX_PHYSMEM_BITS
is (pgtable_l5_enabled() ? 52 : 46) - so MAX_PHYSMEM_BITS depends on the
actual HW.

> +int check_hotplug_memory_addressable(u64 start, u64 size)
> +{
> +#ifdef MAX_POSSIBLE_PHYSMEM_BITS
> +	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
> +		return -E2BIG;
> +#endif
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> +
>  static int check_hotplug_memory_range(u64 start, u64 size)
>  {
>  	/* memory range must be block size aligned */
> @@ -1040,7 +1057,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return check_hotplug_memory_addressable(start, size);
>  }
>  
>  static int online_memory_block(struct memory_block *mem, void *arg)
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 10.09.19 04:52, 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").
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  include/linux/memory_hotplug.h |  1 +
>  mm/memory_hotplug.c            | 19 ++++++++++++++++++-
>  2 files changed, 19 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..3c5428b014f9 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1030,6 +1030,23 @@ int try_online_node(int nid)
>  	return ret;
>  }
>  
> +#ifndef MAX_POSSIBLE_PHYSMEM_BITS
> +#ifdef MAX_PHYSMEM_BITS
> +#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS
> +#endif
> +#endif
> +

I think using MAX_POSSIBLE_PHYSMEM_BITS is wrong. You should use
MAX_PHYSMEM_BITS.

E.g. on x86_64, MAX_POSSIBLE_PHYSMEM_BITS is 52, while MAX_PHYSMEM_BITS
is (pgtable_l5_enabled() ? 52 : 46) - so MAX_PHYSMEM_BITS depends on the
actual HW.

> +int check_hotplug_memory_addressable(u64 start, u64 size)
> +{
> +#ifdef MAX_POSSIBLE_PHYSMEM_BITS
> +	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
> +		return -E2BIG;
> +#endif
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> +
>  static int check_hotplug_memory_range(u64 start, u64 size)
>  {
>  	/* memory range must be block size aligned */
> @@ -1040,7 +1057,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return check_hotplug_memory_addressable(start, size);
>  }
>  
>  static int online_memory_block(struct memory_block *mem, void *arg)
> 


-- 

Thanks,

David / dhildenb

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

[Kirill A. Shutemov]

On Tue, Sep 10, 2019 at 12:52:20PM +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").
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  include/linux/memory_hotplug.h |  1 +
>  mm/memory_hotplug.c            | 19 ++++++++++++++++++-
>  2 files changed, 19 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..3c5428b014f9 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1030,6 +1030,23 @@ int try_online_node(int nid)
>  	return ret;
>  }
>  
> +#ifndef MAX_POSSIBLE_PHYSMEM_BITS
> +#ifdef MAX_PHYSMEM_BITS
> +#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS
> +#endif
> +#endif
> +
> +int check_hotplug_memory_addressable(u64 start, u64 size)
> +{
> +#ifdef MAX_POSSIBLE_PHYSMEM_BITS

How can it be not defined? You've defined it 6 lines above.

> +	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
> +		return -E2BIG;
> +#endif
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> +
>  static int check_hotplug_memory_range(u64 start, u64 size)
>  {
>  	/* memory range must be block size aligned */
> @@ -1040,7 +1057,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return check_hotplug_memory_addressable(start, size);
>  }
>  
>  static int online_memory_block(struct memory_block *mem, void *arg)
> -- 
> 2.21.0
> 

-- 
 Kirill A. Shutemov

RE: [PATCH 2/2] mm: Add a bounds check in devm_memremap_pages()

[Alastair D'Silva]

> -----Original Message-----
> From: David Hildenbrand <david@redhat.com>
> Sent: Tuesday, 10 September 2019 5:39 PM
> To: Alastair D'Silva <alastair@au1.ibm.com>; alastair@d-silva.org
> Cc: Andrew Morton <akpm@linux-foundation.org>; Oscar Salvador
> <osalvador@suse.com>; Michal Hocko <mhocko@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 2/2] mm: Add a bounds check in
> devm_memremap_pages()
> 
> On 10.09.19 04:52, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > The call to check_hotplug_memory_addressable() validates that the
> > memory is fully addressable.
> >
> > Without this call, it is possible that we may remap pages that is not
> > physically addressable, resulting in bogus section numbers being
> > returned from __section_nr().
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  mm/memremap.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/mm/memremap.c b/mm/memremap.c index
> > 86432650f829..fd00993caa3e 100644
> > --- a/mm/memremap.c
> > +++ b/mm/memremap.c
> > @@ -269,6 +269,13 @@ void *devm_memremap_pages(struct device
> *dev,
> > struct dev_pagemap *pgmap)
> >
> >  	mem_hotplug_begin();
> >
> > +	error = check_hotplug_memory_addressable(res->start,
> > +						 resource_size(res));
> > +	if (error) {
> > +		mem_hotplug_done();
> > +		goto err_checkrange;
> > +	}
> > +
> 
> No need to check under the memory hotplug lock.
> 

Thanks, I'll adjust it.

> >  	/*
> >  	 * For device private memory we call add_pages() as we only need to
> >  	 * allocate and initialize struct page for the device memory. More-
> > @@ -324,6 +331,7 @@ void *devm_memremap_pages(struct device *dev,
> > struct dev_pagemap *pgmap)
> >
> >   err_add_memory:
> >  	kasan_remove_zero_shadow(__va(res->start), resource_size(res));
> > + err_checkrange:
> >   err_kasan:
> >  	untrack_pfn(NULL, PHYS_PFN(res->start), resource_size(res));
> >   err_pfn_remap:
> >
> 
> 
> --
> 
> Thanks,
> 
> David / dhildenb
> 

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

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

[Alastair D'Silva]

> -----Original Message-----
> From: David Hildenbrand <david@redhat.com>
> Sent: Tuesday, 10 September 2019 5:46 PM
> To: Alastair D'Silva <alastair@au1.ibm.com>; alastair@d-silva.org
> Cc: Andrew Morton <akpm@linux-foundation.org>; Oscar Salvador
> <osalvador@suse.com>; Michal Hocko <mhocko@suse.com>; Pavel Tatashin
> <pasha.tatashin@soleen.com>; Wei Yang <richard.weiyang@gmail.com>;
> Dan Williams <dan.j.williams@intel.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 1/2] memory_hotplug: Add a bounds check to
> check_hotplug_memory_range()
> 
> On 10.09.19 04:52, 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").
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  include/linux/memory_hotplug.h |  1 +
> >  mm/memory_hotplug.c            | 19 ++++++++++++++++++-
> >  2 files changed, 19 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..3c5428b014f9 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1030,6 +1030,23 @@ int try_online_node(int nid)
> >  	return ret;
> >  }
> >
> > +#ifndef MAX_POSSIBLE_PHYSMEM_BITS
> > +#ifdef MAX_PHYSMEM_BITS
> > +#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS #endif
> #endif
> > +
> 
> I think using MAX_POSSIBLE_PHYSMEM_BITS bits is wrong. You should use
> MAX_PHYSMEM_BITS.
> 
> E.g. on x86_64, MAX_POSSIBLE_PHYSMEM_BITS is 52, while
> MAX_PHYSMEM_BITS is (pgtable_l5_enabled() ? 52 : 46) - so
> MAX_PHYSMEM_BITS depends on the actual HW.
> 

Thanks, I was following the pattern from zsmalloc.c, but what you say makes sense.

> > +int check_hotplug_memory_addressable(u64 start, u64 size) { #ifdef
> > +MAX_POSSIBLE_PHYSMEM_BITS
> > +	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
> > +		return -E2BIG;
> > +#endif
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> > +
> >  static int check_hotplug_memory_range(u64 start, u64 size)  {
> >  	/* memory range must be block size aligned */ @@ -1040,7 +1057,7
> @@
> > static int check_hotplug_memory_range(u64 start, u64 size)
> >  		return -EINVAL;
> >  	}
> >
> > -	return 0;
> > +	return check_hotplug_memory_addressable(start, size);
> >  }
> >
> >  static int online_memory_block(struct memory_block *mem, void *arg)
> >
> 
> 
> --
> 
> Thanks,
> 
> David / dhildenb
> 


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

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

[Alastair D'Silva]

> -----Original Message-----
> From: Kirill A. Shutemov <kirill@shutemov.name>
> Sent: Tuesday, 10 September 2019 8:15 PM
> To: Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Andrew Morton <akpm@linux-foundation.org>;
> David Hildenbrand <david@redhat.com>; Oscar Salvador
> <osalvador@suse.com>; Michal Hocko <mhocko@suse.com>; Pavel Tatashin
> <pasha.tatashin@soleen.com>; Wei Yang <richard.weiyang@gmail.com>;
> Dan Williams <dan.j.williams@intel.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 1/2] memory_hotplug: Add a bounds check to
> check_hotplug_memory_range()
> 
> On Tue, Sep 10, 2019 at 12:52:20PM +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").
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  include/linux/memory_hotplug.h |  1 +
> >  mm/memory_hotplug.c            | 19 ++++++++++++++++++-
> >  2 files changed, 19 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..3c5428b014f9 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1030,6 +1030,23 @@ int try_online_node(int nid)
> >  	return ret;
> >  }
> >
> > +#ifndef MAX_POSSIBLE_PHYSMEM_BITS
> > +#ifdef MAX_PHYSMEM_BITS
> > +#define MAX_POSSIBLE_PHYSMEM_BITS MAX_PHYSMEM_BITS #endif
> #endif
> > +
> > +int check_hotplug_memory_addressable(u64 start, u64 size) { #ifdef
> > +MAX_POSSIBLE_PHYSMEM_BITS
> 
> How can it be not defined? You've defined it 6 lines above.
> 

It's only conditionally defined.

I'll be following David H's advice and just using MAX_PHYSMEM_BITS in the
next spin anyway.

> > +	if ((start + size - 1) >> MAX_POSSIBLE_PHYSMEM_BITS)
> > +		return -E2BIG;
> > +#endif
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(check_hotplug_memory_addressable);
> > +
> >  static int check_hotplug_memory_range(u64 start, u64 size)  {
> >  	/* memory range must be block size aligned */ @@ -1040,7 +1057,7
> @@
> > static int check_hotplug_memory_range(u64 start, u64 size)
> >  		return -EINVAL;
> >  	}
> >
> > -	return 0;
> > +	return check_hotplug_memory_addressable(start, size);
> >  }
> >
> >  static int online_memory_block(struct memory_block *mem, void *arg)
> > --
> > 2.21.0
> >
> 
> --
>  Kirill A. Shutemov
> 


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