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

[PATCH v3 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.

Changelog:
 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 (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            | 13 ++++++++++++-
 mm/memremap.c                  |  5 +++++
 3 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.21.0

[PATCH v3 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            | 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
+
+	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 +1051,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 v3 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/memremap.c b/mm/memremap.c
index 86432650f829..de2b67586401 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -175,6 +175,11 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 	int error, nid, is_ram;
 	bool need_devmap_managed = true;
 
+	error = check_hotplug_memory_addressable(res->start,
+						 resource_size(res));
+	if (error)
+		return ERR_PTR(error);
+
 	switch (pgmap->type) {
 	case MEMORY_DEVICE_PRIVATE:
 		if (!IS_ENABLED(CONFIG_DEVICE_PRIVATE)) {
-- 
2.21.0

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

[David Hildenbrand]

On 17.09.19 03:07, 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            | 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
> +
> +	return 0;
> +}

I guess checking for address space wrapping would be overkill. This
change makes sense for architecture-independent device drivers that make
use of the add/remove memory infrastructure (e.g., virtio-mem I am
working on).

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

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 17.09.19 03:07, 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 86432650f829..de2b67586401 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -175,6 +175,11 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  	int error, nid, is_ram;
>  	bool need_devmap_managed = true;
>  
> +	error = check_hotplug_memory_addressable(res->start,
> +						 resource_size(res));
> +	if (error)
> +		return ERR_PTR(error);
> +
>  	switch (pgmap->type) {
>  	case MEMORY_DEVICE_PRIVATE:
>  		if (!IS_ENABLED(CONFIG_DEVICE_PRIVATE)) {
> 

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

-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

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

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

> +
> +	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?

> +
>  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?

>  }
>  
>  static int online_memory_block(struct memory_block *mem, void *arg)
> -- 
> 2.21.0
> 

-- 
Michal Hocko
SUSE Labs

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

[Alastair D'Silva]

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

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?

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?

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

> >  }
> >  
> >  static int online_memory_block(struct memory_block *mem, void
> > *arg)
> > -- 
> > 2.21.0
> > 

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

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@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

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

[David Hildenbrand]

On 24.09.19 11:09, Michal Hocko wrote:
> 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.
> 

I don't agree in virtual environment. On s390x, MAX_PHYSMEM_BITS is
configurable. For example, if you have paravirtualized memory hotplug
(e.g., virtio-mem), you could add memory to the system that violates
this constraint.

virtio-mem, however, does properly check for MAX_PHYSMEM_BITS itself -
at least in the current RFC v3.

-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

On Tue 24-09-19 11:13:31, David Hildenbrand wrote:
> On 24.09.19 11:09, Michal Hocko wrote:
> > On Tue 24-09-19 11:31:05, Alastair D'Silva wrote:
> >> On Mon, 2019-09-23 at 14:25 +0200, Michal Hocko wrote:
[...]
> >>> 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.
> > 
> 
> I don't agree in virtual environment. On s390x, MAX_PHYSMEM_BITS is
> configurable. For example, if you have paravirtualized memory hotplug
> (e.g., virtio-mem), you could add memory to the system that violates
> this constraint.

What happens if that is the case. Does the machine change the
configuration in runtime or it needs a reboot?

Anyway, seeing this to be the case in the log would help in whatever
action is necessary to deal with the issue, right?
-- 
Michal Hocko
SUSE Labs