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
