Re: [patch 066/134] mm/memory_hotplug.c: add a bounds check to check_hotplug_memory_range()

[Michal Hocko]

On Mon 23-09-19 15:36:12, Andrew Morton wrote:
> From: Alastair D'Silva <alastair@xxxxxxxxxxx>
> Subject: mm/memory_hotplug.c: add a bounds check to check_hotplug_memory_range()
> 
> Patch series "Add bounds check for Hotplugged memory", v3.
> 
> 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.
> 
> 
> This patch (of 2):
> 
> 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").
> 
> David said:
> 
> : 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).

Isn't this rushed a bit? I've had a review feedback just yesterday
http://lkml.kernel.org/r/20190923122513.GO6016@xxxxxxxxxxxxxx and had
some concerns.

> Link: http://lkml.kernel.org/r/20190917010752.28395-2-alastair@xxxxxxxxxxx
> Signed-off-by: Alastair D'Silva <alastair@xxxxxxxxxxx>
> Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Wei Yang <richard.weiyang@xxxxxxxxx>
> Cc: Qian Cai <cai@xxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
>  include/linux/memory_hotplug.h |    1 +
>  mm/memory_hotplug.c            |   13 ++++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/memory_hotplug.h~memory_hotplug-add-a-bounds-check-to-check_hotplug_memory_range
> +++ a/include/linux/memory_hotplug.h
> @@ -110,6 +110,7 @@ extern void __online_page_increment_coun
>  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);
> --- a/mm/memory_hotplug.c~memory_hotplug-add-a-bounds-check-to-check_hotplug_memory_range
> +++ a/mm/memory_hotplug.c
> @@ -1023,6 +1023,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 */
> @@ -1033,7 +1044,7 @@ static int check_hotplug_memory_range(u6
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return check_hotplug_memory_addressable(start, size);
>  }
>  
>  static int online_memory_block(struct memory_block *mem, void *arg)
> _

-- 
Michal Hocko
SUSE Labs