* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
@ 2019-09-26 7:12 ` David Hildenbrand
2019-09-26 7:37 ` David Hildenbrand
2019-09-26 7:43 ` Michal Hocko
2019-09-26 7:40 ` Oscar Salvador
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26 7:12 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Andrew Morton, Oscar Salvador, Michal Hocko, Pavel Tatashin,
Dan Williams, linux-mm, linux-kernel
On 26.09.19 03:34, 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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> mm/memory_hotplug.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> return 0;
> }
>
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> + unsigned long nr_pages)
> +{
> + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> +
> + if (max_addr >> MAX_PHYSMEM_BITS) {
> + WARN(1,
> + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> + pfn << PAGE_SHIFT, max_addr,
> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Reasonably generic function for adding memory. It is
> * expected that archs that support memory hotplug will
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> unsigned long nr, start_sec, end_sec;
> struct vmem_altmap *altmap = restrictions->altmap;
>
> + err = check_hotplug_memory_addressable(pfn, nr_pages);
> + if (err)
> + return err;
> +
> if (altmap) {
> /*
> * Validate altmap is within bounds of the total request
>
I know Michal suggested this, but I still prefer checking early instead
of when we're knees-deep into adding of memory. But as I don't have any
power here, the code looks fine, although I consider the computations in
check_hotplug_memory_addressable() fairly ugly.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:12 ` David Hildenbrand
@ 2019-09-26 7:37 ` David Hildenbrand
2019-09-26 7:43 ` Michal Hocko
1 sibling, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26 7:37 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Andrew Morton, Oscar Salvador, Michal Hocko, Pavel Tatashin,
Dan Williams, linux-mm, linux-kernel
On 26.09.19 09:12, David Hildenbrand wrote:
> On 26.09.19 03:34, 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")
>> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>>
>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>> ---
>> mm/memory_hotplug.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index c73f09913165..212804c0f7f5 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>> return 0;
>> }
>>
>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>> + unsigned long nr_pages)
>> +{
>> + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
>> +
>> + if (max_addr >> MAX_PHYSMEM_BITS) {
>> + WARN(1,
>> + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>> + pfn << PAGE_SHIFT, max_addr,
>> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>> + return -E2BIG;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * Reasonably generic function for adding memory. It is
>> * expected that archs that support memory hotplug will
>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> unsigned long nr, start_sec, end_sec;
>> struct vmem_altmap *altmap = restrictions->altmap;
>>
>> + err = check_hotplug_memory_addressable(pfn, nr_pages);
>> + if (err)
>> + return err;
>> +
>> if (altmap) {
>> /*
>> * Validate altmap is within bounds of the total request
>>
>
>
> I know Michal suggested this, but I still prefer checking early instead
> of when we're knees-deep into adding of memory. But as I don't have any
> power here, the code looks fine, although I consider the computations in
> check_hotplug_memory_addressable() fairly ugly.
>
Forgot to add
Acked-by: David Hildenbrand <david@redhat.com>
:)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:12 ` David Hildenbrand
2019-09-26 7:37 ` David Hildenbrand
@ 2019-09-26 7:43 ` Michal Hocko
2019-09-26 7:46 ` David Hildenbrand
1 sibling, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2019-09-26 7:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: Alastair D'Silva, alastair, Andrew Morton, Oscar Salvador,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
> On 26.09.19 03:34, 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")
> > http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > mm/memory_hotplug.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index c73f09913165..212804c0f7f5 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> > return 0;
> > }
> >
> > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > + unsigned long nr_pages)
> > +{
> > + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> > +
> > + if (max_addr >> MAX_PHYSMEM_BITS) {
> > + WARN(1,
> > + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> > + pfn << PAGE_SHIFT, max_addr,
> > + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> > + return -E2BIG;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Reasonably generic function for adding memory. It is
> > * expected that archs that support memory hotplug will
> > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> > unsigned long nr, start_sec, end_sec;
> > struct vmem_altmap *altmap = restrictions->altmap;
> >
> > + err = check_hotplug_memory_addressable(pfn, nr_pages);
> > + if (err)
> > + return err;
> > +
> > if (altmap) {
> > /*
> > * Validate altmap is within bounds of the total request
> >
>
>
> I know Michal suggested this, but I still prefer checking early instead
> of when we're knees-deep into adding of memory.
What is your concern here? Unwinding the state should be pretty
straightfoward from this failure path.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:43 ` Michal Hocko
@ 2019-09-26 7:46 ` David Hildenbrand
2019-09-27 6:33 ` Alastair D'Silva
0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26 7:46 UTC (permalink / raw)
To: Michal Hocko
Cc: Alastair D'Silva, alastair, Andrew Morton, Oscar Salvador,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On 26.09.19 09:43, Michal Hocko wrote:
> On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
>> On 26.09.19 03:34, 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")
>>> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>>>
>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>>> ---
>>> mm/memory_hotplug.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index c73f09913165..212804c0f7f5 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>>> return 0;
>>> }
>>>
>>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>>> + unsigned long nr_pages)
>>> +{
>>> + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
>>> +
>>> + if (max_addr >> MAX_PHYSMEM_BITS) {
>>> + WARN(1,
>>> + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>>> + pfn << PAGE_SHIFT, max_addr,
>>> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>>> + return -E2BIG;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * Reasonably generic function for adding memory. It is
>>> * expected that archs that support memory hotplug will
>>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>> unsigned long nr, start_sec, end_sec;
>>> struct vmem_altmap *altmap = restrictions->altmap;
>>>
>>> + err = check_hotplug_memory_addressable(pfn, nr_pages);
>>> + if (err)
>>> + return err;
>>> +
>>> if (altmap) {
>>> /*
>>> * Validate altmap is within bounds of the total request
>>>
>>
>>
>> I know Michal suggested this, but I still prefer checking early instead
>> of when we're knees-deep into adding of memory.
>
> What is your concern here? Unwinding the state should be pretty
> straightfoward from this failure path.
Just the general "check what you can check early without locks"
approach. But yeah, this series is probably not worth a v5, so I can
live with this change just fine :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:46 ` David Hildenbrand
@ 2019-09-27 6:33 ` Alastair D'Silva
2019-09-27 7:24 ` David Hildenbrand
0 siblings, 1 reply; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-27 6:33 UTC (permalink / raw)
To: David Hildenbrand, Michal Hocko
Cc: Andrew Morton, Oscar Salvador, Pavel Tatashin, Dan Williams,
linux-mm, linux-kernel
On Thu, 2019-09-26 at 09:46 +0200, David Hildenbrand wrote:
> On 26.09.19 09:43, Michal Hocko wrote:
> > On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
> > > On 26.09.19 03:34, 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")
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20190827052047.31547-2D1-2Dalastair-40au1.ibm.com&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=cT4tgeEQ0Ll3SIlZDHE5AEXyKy6uKADMtf9_Eb7-vec&m=p9ZS4kSnvF0zq81jcCFd2nYj1zfTMvfbApCtmKI2KNA&s=yif-duzz_RESW3LUyU_0kkmefRAnKWjjn_p5Et-9B2g&e=
> > > >
> > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > > ---
> > > > mm/memory_hotplug.c | 20 ++++++++++++++++++++
> > > > 1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > index c73f09913165..212804c0f7f5 100644
> > > > --- a/mm/memory_hotplug.c
> > > > +++ b/mm/memory_hotplug.c
> > > > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long
> > > > pfn, unsigned long nr_pages,
> > > > return 0;
> > > > }
> > > >
> > > > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > > > + unsigned long
> > > > nr_pages)
> > > > +{
> > > > + unsigned long max_addr = ((pfn + nr_pages) <<
> > > > PAGE_SHIFT) - 1;
> > > > +
> > > > + if (max_addr >> MAX_PHYSMEM_BITS) {
> > > > + WARN(1,
> > > > + "Hotplugged memory exceeds maximum
> > > > addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> > > > + pfn << PAGE_SHIFT, max_addr,
> > > > + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> > > > + return -E2BIG;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > /*
> > > > * Reasonably generic function for adding memory. It is
> > > > * expected that archs that support memory hotplug will
> > > > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned
> > > > long pfn, unsigned long nr_pages,
> > > > unsigned long nr, start_sec, end_sec;
> > > > struct vmem_altmap *altmap = restrictions->altmap;
> > > >
> > > > + err = check_hotplug_memory_addressable(pfn, nr_pages);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > if (altmap) {
> > > > /*
> > > > * Validate altmap is within bounds of the
> > > > total request
> > > >
> > >
> > > I know Michal suggested this, but I still prefer checking early
> > > instead
> > > of when we're knees-deep into adding of memory.
> >
> > What is your concern here? Unwinding the state should be pretty
> > straightfoward from this failure path.
>
> Just the general "check what you can check early without locks"
> approach. But yeah, this series is probably not worth a v5, so I can
> live with this change just fine :)
>
>
I'm going to spin a V5 anyway - where were you suggesting?
> --
>
> Thanks,
>
> David / dhildenb
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-27 6:33 ` Alastair D'Silva
@ 2019-09-27 7:24 ` David Hildenbrand
0 siblings, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-27 7:24 UTC (permalink / raw)
To: Alastair D'Silva, Michal Hocko
Cc: Andrew Morton, Oscar Salvador, Pavel Tatashin, Dan Williams,
linux-mm, linux-kernel
On 27.09.19 08:33, Alastair D'Silva wrote:
> On Thu, 2019-09-26 at 09:46 +0200, David Hildenbrand wrote:
>> On 26.09.19 09:43, Michal Hocko wrote:
>>> On Thu 26-09-19 09:12:50, David Hildenbrand wrote:
>>>> On 26.09.19 03:34, 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")
>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20190827052047.31547-2D1-2Dalastair-40au1.ibm.com&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=cT4tgeEQ0Ll3SIlZDHE5AEXyKy6uKADMtf9_Eb7-vec&m=p9ZS4kSnvF0zq81jcCFd2nYj1zfTMvfbApCtmKI2KNA&s=yif-duzz_RESW3LUyU_0kkmefRAnKWjjn_p5Et-9B2g&e=
>>>>>
>>>>> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>>>>> ---
>>>>> mm/memory_hotplug.c | 20 ++++++++++++++++++++
>>>>> 1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>>> index c73f09913165..212804c0f7f5 100644
>>>>> --- a/mm/memory_hotplug.c
>>>>> +++ b/mm/memory_hotplug.c
>>>>> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long
>>>>> pfn, unsigned long nr_pages,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static int check_hotplug_memory_addressable(unsigned long pfn,
>>>>> + unsigned long
>>>>> nr_pages)
>>>>> +{
>>>>> + unsigned long max_addr = ((pfn + nr_pages) <<
>>>>> PAGE_SHIFT) - 1;
>>>>> +
>>>>> + if (max_addr >> MAX_PHYSMEM_BITS) {
>>>>> + WARN(1,
>>>>> + "Hotplugged memory exceeds maximum
>>>>> addressable address, range=%#lx-%#lx, maximum=%#lx\n",
>>>>> + pfn << PAGE_SHIFT, max_addr,
>>>>> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>>>>> + return -E2BIG;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * Reasonably generic function for adding memory. It is
>>>>> * expected that archs that support memory hotplug will
>>>>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned
>>>>> long pfn, unsigned long nr_pages,
>>>>> unsigned long nr, start_sec, end_sec;
>>>>> struct vmem_altmap *altmap = restrictions->altmap;
>>>>>
>>>>> + err = check_hotplug_memory_addressable(pfn, nr_pages);
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> if (altmap) {
>>>>> /*
>>>>> * Validate altmap is within bounds of the
>>>>> total request
>>>>>
>>>>
>>>> I know Michal suggested this, but I still prefer checking early
>>>> instead
>>>> of when we're knees-deep into adding of memory.
>>>
>>> What is your concern here? Unwinding the state should be pretty
>>> straightfoward from this failure path.
>>
>> Just the general "check what you can check early without locks"
>> approach. But yeah, this series is probably not worth a v5, so I can
>> live with this change just fine :)
>>
>>
>
> I'm going to spin a V5 anyway - where were you suggesting?
I preferred the previous places where we checked, but I think we settled
on __add_pages(). So I am fine with the changes Oscar proposed. You
might want to turn "max_addr" into a const if you feel fancy. :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
2019-09-26 7:12 ` David Hildenbrand
@ 2019-09-26 7:40 ` Oscar Salvador
2019-09-26 7:42 ` David Hildenbrand
2019-09-26 7:47 ` Michal Hocko
2019-09-26 7:44 ` Michal Hocko
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Oscar Salvador @ 2019-09-26 7:40 UTC (permalink / raw)
To: Alastair D'Silva
Cc: alastair, Andrew Morton, Michal Hocko, David Hildenbrand,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> unsigned long nr, start_sec, end_sec;
> struct vmem_altmap *altmap = restrictions->altmap;
>
> + err = check_hotplug_memory_addressable(pfn, nr_pages);
> + if (err)
> + return err;
> +
I am probably off here because 1) I am jumping blind in a middle of a discussion and
2) I got back from holydays yesterday, so bear with me.
Would not be better to just place the check in add_memory_resource instead?
Take into account that we create the memory mapping for this range in
arch_add_memory, so it looks weird to me to create the mapping if we are going to
fail right after because the range is simply off.
But as I said, I might be missing some previous discussion.
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:40 ` Oscar Salvador
@ 2019-09-26 7:42 ` David Hildenbrand
2019-09-26 7:47 ` Michal Hocko
1 sibling, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2019-09-26 7:42 UTC (permalink / raw)
To: Oscar Salvador, Alastair D'Silva
Cc: alastair, Andrew Morton, Michal Hocko, Pavel Tatashin,
Dan Williams, linux-mm, linux-kernel
On 26.09.19 09:40, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
>> From: Alastair D'Silva <alastair@d-silva.org>
>> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> unsigned long nr, start_sec, end_sec;
>> struct vmem_altmap *altmap = restrictions->altmap;
>>
>> + err = check_hotplug_memory_addressable(pfn, nr_pages);
>> + if (err)
>> + return err;
>> +
>
> I am probably off here because 1) I am jumping blind in a middle of a discussion and
> 2) I got back from holydays yesterday, so bear with me.
>
> Would not be better to just place the check in add_memory_resource instead?
At least devmem/memremap needs special handling.
> Take into account that we create the memory mapping for this range in
> arch_add_memory, so it looks weird to me to create the mapping if we are going to
> fail right after because the range is simply off.
>
> But as I said, I might be missing some previous discussion.
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:40 ` Oscar Salvador
2019-09-26 7:42 ` David Hildenbrand
@ 2019-09-26 7:47 ` Michal Hocko
1 sibling, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2019-09-26 7:47 UTC (permalink / raw)
To: Oscar Salvador
Cc: Alastair D'Silva, alastair, Andrew Morton, David Hildenbrand,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On Thu 26-09-19 09:40:05, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> > unsigned long nr, start_sec, end_sec;
> > struct vmem_altmap *altmap = restrictions->altmap;
> >
> > + err = check_hotplug_memory_addressable(pfn, nr_pages);
> > + if (err)
> > + return err;
> > +
>
> I am probably off here because 1) I am jumping blind in a middle of a discussion and
> 2) I got back from holydays yesterday, so bear with me.
>
> Would not be better to just place the check in add_memory_resource instead?
This was the previous version of the patch. The argument is that we do
not want each add_pages user to think of this special handling.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
2019-09-26 7:12 ` David Hildenbrand
2019-09-26 7:40 ` Oscar Salvador
@ 2019-09-26 7:44 ` Michal Hocko
2019-09-26 7:53 ` Oscar Salvador
2019-09-26 15:35 ` kbuild test robot
4 siblings, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2019-09-26 7:44 UTC (permalink / raw)
To: Alastair D'Silva
Cc: alastair, Andrew Morton, Oscar Salvador, David Hildenbrand,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On Thu 26-09-19 11:34:05, 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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Yes, this looks better to me. E2BIG is a new error code for this path
but no callers seem to be deeply concerned about a specific error codes
so this should be safe.
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memory_hotplug.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> return 0;
> }
>
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> + unsigned long nr_pages)
> +{
> + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
> +
> + if (max_addr >> MAX_PHYSMEM_BITS) {
> + WARN(1,
> + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> + pfn << PAGE_SHIFT, max_addr,
> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Reasonably generic function for adding memory. It is
> * expected that archs that support memory hotplug will
> @@ -291,6 +307,10 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> unsigned long nr, start_sec, end_sec;
> struct vmem_altmap *altmap = restrictions->altmap;
>
> + err = check_hotplug_memory_addressable(pfn, nr_pages);
> + if (err)
> + return err;
> +
> if (altmap) {
> /*
> * Validate altmap is within bounds of the total request
> --
> 2.21.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
` (2 preceding siblings ...)
2019-09-26 7:44 ` Michal Hocko
@ 2019-09-26 7:53 ` Oscar Salvador
2019-09-27 5:14 ` Alastair D'Silva
2019-09-26 15:35 ` kbuild test robot
4 siblings, 1 reply; 15+ messages in thread
From: Oscar Salvador @ 2019-09-26 7:53 UTC (permalink / raw)
To: Alastair D'Silva
Cc: alastair, Andrew Morton, Michal Hocko, David Hildenbrand,
Pavel Tatashin, Dan Williams, linux-mm, linux-kernel
On Thu, Sep 26, 2019 at 11:34:05AM +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")
> http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Just a nit-picking below:
> ---
> mm/memory_hotplug.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..212804c0f7f5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> return 0;
> }
>
> +static int check_hotplug_memory_addressable(unsigned long pfn,
> + unsigned long nr_pages)
> +{
> + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
I would use PFN_PHYS instead:
unsigned long max_addr = PFN_PHYS(pfn + nr_pages) - 1;
> +
> + if (max_addr >> MAX_PHYSMEM_BITS) {
> + WARN(1,
> + "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
> + pfn << PAGE_SHIFT, max_addr,
Same here.
> + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
I would use a local variable to hold this computation.
> + return -E2BIG;
> + }
> +
> + return 0;
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 7:53 ` Oscar Salvador
@ 2019-09-27 5:14 ` Alastair D'Silva
0 siblings, 0 replies; 15+ messages in thread
From: Alastair D'Silva @ 2019-09-27 5:14 UTC (permalink / raw)
To: Oscar Salvador
Cc: Andrew Morton, Michal Hocko, David Hildenbrand, Pavel Tatashin,
Dan Williams, linux-mm, linux-kernel
On Thu, 2019-09-26 at 09:53 +0200, Oscar Salvador wrote:
> On Thu, Sep 26, 2019 at 11:34:05AM +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")
> > http://lkml.kernel.org/r/20190827052047.31547-1-alastair@au1.ibm.com
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
>
> Just a nit-picking below:
>
> > ---
> > mm/memory_hotplug.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index c73f09913165..212804c0f7f5 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -278,6 +278,22 @@ static int check_pfn_span(unsigned long pfn,
> > unsigned long nr_pages,
> > return 0;
> > }
> >
> > +static int check_hotplug_memory_addressable(unsigned long pfn,
> > + unsigned long nr_pages)
> > +{
> > + unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
>
> I would use PFN_PHYS instead:
>
> unsigned long max_addr = PFN_PHYS(pfn + nr_pages) - 1;
>
> > +
> > + if (max_addr >> MAX_PHYSMEM_BITS) {
> > + WARN(1,
> > + "Hotplugged memory exceeds maximum addressable
> > address, range=%#lx-%#lx, maximum=%#lx\n",
> > + pfn << PAGE_SHIFT, max_addr,
>
> Same here.
>
> > + (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
>
> I would use a local variable to hold this computation.
>
> > + return -E2BIG;
> > + }
> > +
> > + return 0;
Looks like I'll have to do another spin to change that to a ull anyway,
so I'll implement those suggestions.
--
Alastair D'Silva mob: 0423 762 819
skype: alastair_dsilva
Twitter: @EvilDeece
blog: http://alastair.d-silva.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] memory_hotplug: Add a bounds check to __add_pages
2019-09-26 1:34 ` [PATCH v4] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva
` (3 preceding siblings ...)
2019-09-26 7:53 ` Oscar Salvador
@ 2019-09-26 15:35 ` kbuild test robot
4 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2019-09-26 15:35 UTC (permalink / raw)
To: Alastair D'Silva
Cc: kbuild-all, alastair, Andrew Morton, Oscar Salvador,
Michal Hocko, David Hildenbrand, Pavel Tatashin, Dan Williams,
linux-mm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2637 bytes --]
Hi Alastair,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3 next-20190925]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Alastair-D-Silva/memory_hotplug-Add-a-bounds-check-to-__add_pages/20190926-094437
config: i386-randconfig-g004-201938 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
mm/memory_hotplug.c: In function 'check_hotplug_memory_addressable':
>> mm/memory_hotplug.c:286:15: warning: right shift count >= width of type [-Wshift-count-overflow]
if (max_addr >> MAX_PHYSMEM_BITS) {
^~
In file included from arch/x86/include/asm/bug.h:83:0,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from mm/memory_hotplug.c:9:
>> mm/memory_hotplug.c:290:13: warning: left shift count >= width of type [-Wshift-count-overflow]
(1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
^
include/asm-generic/bug.h:112:21: note: in definition of macro '__WARN_printf_taint'
do { __warn_printk(arg); __WARN_TAINT(taint); } while (0)
^~~
include/asm-generic/bug.h:135:3: note: in expansion of macro '__WARN_printf'
__WARN_printf(format); \
^~~~~~~~~~~~~
>> mm/memory_hotplug.c:287:3: note: in expansion of macro 'WARN'
WARN(1,
^~~~
vim +286 mm/memory_hotplug.c
280
281 static int check_hotplug_memory_addressable(unsigned long pfn,
282 unsigned long nr_pages)
283 {
284 unsigned long max_addr = ((pfn + nr_pages) << PAGE_SHIFT) - 1;
285
> 286 if (max_addr >> MAX_PHYSMEM_BITS) {
> 287 WARN(1,
288 "Hotplugged memory exceeds maximum addressable address, range=%#lx-%#lx, maximum=%#lx\n",
289 pfn << PAGE_SHIFT, max_addr,
> 290 (1ul << (MAX_PHYSMEM_BITS + 1)) - 1);
291 return -E2BIG;
292 }
293
294 return 0;
295 }
296
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31679 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread