Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On 04.09.19 07:25, Alastair D'Silva wrote: > On Mon, 2019-09-02 at 09:28 +0200, David Hildenbrand wrote: >> On 02.09.19 01:54, Alastair D'Silva wrote: >>> On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: On 27.08.19 08:39, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: >> On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: >>> From: Alastair D'Silva >>> >>> It is possible for firmware to allocate memory ranges >>> outside >>> the range of physical memory that we support >>> (MAX_PHYSMEM_BITS). >> >> Doesn't that count as a FW bug? Do you have any evidence of >> that >> in >> the >> field? Just wondering... >> > > Not outside our lab, but OpenCAPI attached LPC memory is > assigned > addresses based on the slot/NPU it is connected to. These > addresses > prior to: > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory > to > 2PB") > were inaccessible and resulted in bogus sections - see our > discussion > on 'mm: Trigger bug on if a section is not found in > __section_nr'. > Doing this check here was your suggestion :) > > It's entirely possible that a similar problem will occur in the > future, > and it's cheap to guard against, which is why I've added this. > If you keep it here, I guess this should be wrapped by a WARN_ON_ONCE(). If we move it to common code (e.g., __add_pages() or add_memory()), then probably not. I can see that s390x allows to configure MAX_PHYSMEM_BITS, so the check could actually make sense. >>> >>> I couldn't see a nice platform indepedent way to determine the >>> allowable address range, but if there is, then I'll move this to >>> the >>> generic code instead. >>> >> >> At least on the !ZONE_DEVICE path we have >> >> __add_memory() -> register_memory_resource() ... >> >> return ERR_PTR(-E2BIG); >> >> >> I was thinking about something like >> >> int add_pages() >> { >> if ((start + size - 1) >> MAX_PHYSMEM_BITS) >> return -E2BIG; >> >> return arch_add_memory(...) >> } >> >> And switching users of arch_add_memory() to add_pages(). However, x86 >> already has an add_pages() function, so that would need some more >> thought. >> >> Maybe simply renaming the existing add_pages() to arch_add_pages(). >> >> add_pages(): Create virtual mapping >> __add_pages(): Don't create virtual mapping >> >> arch_add_memory(): Arch backend for add_pages() >> arch_add_pages(): Arch backend for __add_pages() >> >> It would be even more consistent if we would have arch_add_pages() >> vs. >> __arch_add_pages(). > > Looking a bit further, I think a good course of action would be to add > the check to memory_hotplug.c:check_hotplug_memory_range(). > > This would be the least invasive, and could check both > MAX_POSSIBLE_PHYSMEM_BITS and MAX_PHYSMEM_BITS. You won't be able to catch the memremap path that way, just saying. But at least it would be an easy change. > > With that in mind, we can drop this patch. > -- Thanks, David / dhildenb
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On Mon, 2019-09-02 at 09:28 +0200, David Hildenbrand wrote: > On 02.09.19 01:54, Alastair D'Silva wrote: > > On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: > > > On 27.08.19 08:39, Alastair D'Silva wrote: > > > > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > > > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > > > > From: Alastair D'Silva > > > > > > > > > > > > It is possible for firmware to allocate memory ranges > > > > > > outside > > > > > > the range of physical memory that we support > > > > > > (MAX_PHYSMEM_BITS). > > > > > > > > > > Doesn't that count as a FW bug? Do you have any evidence of > > > > > that > > > > > in > > > > > the > > > > > field? Just wondering... > > > > > > > > > > > > > Not outside our lab, but OpenCAPI attached LPC memory is > > > > assigned > > > > addresses based on the slot/NPU it is connected to. These > > > > addresses > > > > prior to: > > > > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory > > > > to > > > > 2PB") > > > > were inaccessible and resulted in bogus sections - see our > > > > discussion > > > > on 'mm: Trigger bug on if a section is not found in > > > > __section_nr'. > > > > Doing this check here was your suggestion :) > > > > > > > > It's entirely possible that a similar problem will occur in the > > > > future, > > > > and it's cheap to guard against, which is why I've added this. > > > > > > > > > > If you keep it here, I guess this should be wrapped by a > > > WARN_ON_ONCE(). > > > > > > If we move it to common code (e.g., __add_pages() or > > > add_memory()), > > > then > > > probably not. I can see that s390x allows to configure > > > MAX_PHYSMEM_BITS, > > > so the check could actually make sense. > > > > > > > I couldn't see a nice platform indepedent way to determine the > > allowable address range, but if there is, then I'll move this to > > the > > generic code instead. > > > > At least on the !ZONE_DEVICE path we have > > __add_memory() -> register_memory_resource() ... > > return ERR_PTR(-E2BIG); > > > I was thinking about something like > > int add_pages() > { > if ((start + size - 1) >> MAX_PHYSMEM_BITS) > return -E2BIG; > > return arch_add_memory(...) > } > > And switching users of arch_add_memory() to add_pages(). However, x86 > already has an add_pages() function, so that would need some more > thought. > > Maybe simply renaming the existing add_pages() to arch_add_pages(). > > add_pages(): Create virtual mapping > __add_pages(): Don't create virtual mapping > > arch_add_memory(): Arch backend for add_pages() > arch_add_pages(): Arch backend for __add_pages() > > It would be even more consistent if we would have arch_add_pages() > vs. > __arch_add_pages(). Looking a bit further, I think a good course of action would be to add the check to memory_hotplug.c:check_hotplug_memory_range(). This would be the least invasive, and could check both MAX_POSSIBLE_PHYSMEM_BITS and MAX_PHYSMEM_BITS. With that in mind, we can drop this patch. -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On 02.09.19 01:54, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: >> On 27.08.19 08:39, Alastair D'Silva wrote: >>> On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > From: Alastair D'Silva > > It is possible for firmware to allocate memory ranges outside > the range of physical memory that we support > (MAX_PHYSMEM_BITS). Doesn't that count as a FW bug? Do you have any evidence of that in the field? Just wondering... >>> >>> Not outside our lab, but OpenCAPI attached LPC memory is assigned >>> addresses based on the slot/NPU it is connected to. These addresses >>> prior to: >>> 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to >>> 2PB") >>> were inaccessible and resulted in bogus sections - see our >>> discussion >>> on 'mm: Trigger bug on if a section is not found in __section_nr'. >>> Doing this check here was your suggestion :) >>> >>> It's entirely possible that a similar problem will occur in the >>> future, >>> and it's cheap to guard against, which is why I've added this. >>> >> >> If you keep it here, I guess this should be wrapped by a >> WARN_ON_ONCE(). >> >> If we move it to common code (e.g., __add_pages() or add_memory()), >> then >> probably not. I can see that s390x allows to configure >> MAX_PHYSMEM_BITS, >> so the check could actually make sense. >> > > I couldn't see a nice platform indepedent way to determine the > allowable address range, but if there is, then I'll move this to the > generic code instead. > At least on the !ZONE_DEVICE path we have __add_memory() -> register_memory_resource() ... return ERR_PTR(-E2BIG); I was thinking about something like int add_pages() { if ((start + size - 1) >> MAX_PHYSMEM_BITS) return -E2BIG; return arch_add_memory(...) } And switching users of arch_add_memory() to add_pages(). However, x86 already has an add_pages() function, so that would need some more thought. Maybe simply renaming the existing add_pages() to arch_add_pages(). add_pages(): Create virtual mapping __add_pages(): Don't create virtual mapping arch_add_memory(): Arch backend for add_pages() arch_add_pages(): Arch backend for __add_pages() It would be even more consistent if we would have arch_add_pages() vs. __arch_add_pages(). -- Thanks, David / dhildenb
RE: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On Tue, 2019-08-27 at 09:13 +0200, David Hildenbrand wrote: > On 27.08.19 08:39, Alastair D'Silva wrote: > > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > > From: Alastair D'Silva > > > > > > > > It is possible for firmware to allocate memory ranges outside > > > > the range of physical memory that we support > > > > (MAX_PHYSMEM_BITS). > > > > > > Doesn't that count as a FW bug? Do you have any evidence of that > > > in > > > the > > > field? Just wondering... > > > > > > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > > addresses based on the slot/NPU it is connected to. These addresses > > prior to: > > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to > > 2PB") > > were inaccessible and resulted in bogus sections - see our > > discussion > > on 'mm: Trigger bug on if a section is not found in __section_nr'. > > Doing this check here was your suggestion :) > > > > It's entirely possible that a similar problem will occur in the > > future, > > and it's cheap to guard against, which is why I've added this. > > > > If you keep it here, I guess this should be wrapped by a > WARN_ON_ONCE(). > > If we move it to common code (e.g., __add_pages() or add_memory()), > then > probably not. I can see that s390x allows to configure > MAX_PHYSMEM_BITS, > so the check could actually make sense. > I couldn't see a nice platform indepedent way to determine the allowable address range, but if there is, then I'll move this to the generic code instead. -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On Tue 27-08-19 16:39:56, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > > From: Alastair D'Silva > > > > > > It is possible for firmware to allocate memory ranges outside > > > the range of physical memory that we support (MAX_PHYSMEM_BITS). > > > > Doesn't that count as a FW bug? Do you have any evidence of that in > > the > > field? Just wondering... > > > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > addresses based on the slot/NPU it is connected to. These addresses > prior to: > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") > were inaccessible and resulted in bogus sections - see our discussion > on 'mm: Trigger bug on if a section is not found in __section_nr'. Please document this in the changelog -- Michal Hocko SUSE Labs
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On 27.08.19 08:39, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: >> On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: >>> From: Alastair D'Silva >>> >>> It is possible for firmware to allocate memory ranges outside >>> the range of physical memory that we support (MAX_PHYSMEM_BITS). >> >> Doesn't that count as a FW bug? Do you have any evidence of that in >> the >> field? Just wondering... >> > > Not outside our lab, but OpenCAPI attached LPC memory is assigned > addresses based on the slot/NPU it is connected to. These addresses > prior to: > 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") > were inaccessible and resulted in bogus sections - see our discussion > on 'mm: Trigger bug on if a section is not found in __section_nr'. > Doing this check here was your suggestion :) > > It's entirely possible that a similar problem will occur in the future, > and it's cheap to guard against, which is why I've added this. > If you keep it here, I guess this should be wrapped by a WARN_ON_ONCE(). If we move it to common code (e.g., __add_pages() or add_memory()), then probably not. I can see that s390x allows to configure MAX_PHYSMEM_BITS, so the check could actually make sense. -- Thanks, David / dhildenb
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On Tue, 2019-08-27 at 08:28 +0200, Michal Hocko wrote: > On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > > From: Alastair D'Silva > > > > It is possible for firmware to allocate memory ranges outside > > the range of physical memory that we support (MAX_PHYSMEM_BITS). > > Doesn't that count as a FW bug? Do you have any evidence of that in > the > field? Just wondering... > Not outside our lab, but OpenCAPI attached LPC memory is assigned addresses based on the slot/NPU it is connected to. These addresses prior to: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB") were inaccessible and resulted in bogus sections - see our discussion on 'mm: Trigger bug on if a section is not found in __section_nr'. Doing this check here was your suggestion :) It's entirely possible that a similar problem will occur in the future, and it's cheap to guard against, which is why I've added this. -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
Re: [PATCH] powerpc: Perform a bounds check in arch_add_memory
On Tue 27-08-19 15:20:46, Alastair D'Silva wrote: > From: Alastair D'Silva > > It is possible for firmware to allocate memory ranges outside > the range of physical memory that we support (MAX_PHYSMEM_BITS). Doesn't that count as a FW bug? Do you have any evidence of that in the field? Just wondering... > This patch adds a bounds check to ensure that any hotplugged > memory is addressable. > > Signed-off-by: Alastair D'Silva > --- > arch/powerpc/mm/mem.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c > index 9191a66b3bc5..de18fb73de30 100644 > --- a/arch/powerpc/mm/mem.c > +++ b/arch/powerpc/mm/mem.c > @@ -111,6 +111,9 @@ int __ref arch_add_memory(int nid, u64 start, u64 size, > unsigned long nr_pages = size >> PAGE_SHIFT; > int rc; > > + if ((start + size - 1) >> MAX_PHYSMEM_BITS) > + return -EINVAL; > + > resize_hpt_for_hotplug(memblock_phys_mem_size()); > > start = (unsigned long)__va(start); > -- > 2.21.0 -- Michal Hocko SUSE Labs
[PATCH] powerpc: Perform a bounds check in arch_add_memory
From: Alastair D'Silva It is possible for firmware to allocate memory ranges outside the range of physical memory that we support (MAX_PHYSMEM_BITS). This patch adds a bounds check to ensure that any hotplugged memory is addressable. Signed-off-by: Alastair D'Silva --- arch/powerpc/mm/mem.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 9191a66b3bc5..de18fb73de30 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -111,6 +111,9 @@ int __ref arch_add_memory(int nid, u64 start, u64 size, unsigned long nr_pages = size >> PAGE_SHIFT; int rc; + if ((start + size - 1) >> MAX_PHYSMEM_BITS) + return -EINVAL; + resize_hpt_for_hotplug(memblock_phys_mem_size()); start = (unsigned long)__va(start); -- 2.21.0