diff mbox

[RFC,v9,13/21] memory-hotplug: check page type in get_page_bootmem

Message ID 1346837155-534-14-git-send-email-wency@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wen Congyang Sept. 5, 2012, 9:25 a.m. UTC 
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

The function get_page_bootmem() may be called more than one time to the same
page. There is no need to set page's type, private if the function is not
the first time called to the page.

Note: the patch is just optimization and does not fix any problem.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
---
 mm/memory_hotplug.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

Comments

Ni zhan Chen Sept. 29, 2012, 2:15 a.m. UTC | #1 
On 09/05/2012 05:25 PM, wency@cn.fujitsu.com wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> The function get_page_bootmem() may be called more than one time to the same
> page. There is no need to set page's type, private if the function is not
> the first time called to the page.
>
> Note: the patch is just optimization and does not fix any problem.

Hi Yasuaki,

this patch is reasonable to me. I have another question associated to 
get_page_bootmem(), the question is from another fujitsu guy's patch 
changelog [commit : 04753278769f3], the changelog said  that:

  1) When the memmap of removing section is allocated on other
      section by bootmem, it should/can be free.
  2) When the memmap of removing section is allocated on the
      same section, it shouldn't be freed. Because the section has to be
      logical memory offlined already and all pages must be isolated against
      page allocater. If it is freed, page allocator may use it which will
      be removed physically soon.

but I don't see his patch guarantee 2), it means that his patch doesn't 
guarantee the memmap of removing section which is allocated on other 
section by bootmem doesn't be freed. Hopefully get your explaination in 
details, thanks in advance. :-)

>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> ---
>   mm/memory_hotplug.c |   15 +++++++++++----
>   1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index d736df3..26a5012 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -95,10 +95,17 @@ static void release_memory_resource(struct resource *res)
>   static void get_page_bootmem(unsigned long info,  struct page *page,
>   			     unsigned long type)
>   {
> -	page->lru.next = (struct list_head *) type;
> -	SetPagePrivate(page);
> -	set_page_private(page, info);
> -	atomic_inc(&page->_count);
> +	unsigned long page_type;
> +
> +	page_type = (unsigned long)page->lru.next;
> +	if (page_type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
> +	    page_type > MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE){
> +		page->lru.next = (struct list_head *)type;
> +		SetPagePrivate(page);
> +		set_page_private(page, info);
> +		atomic_inc(&page->_count);
> +	} else
> +		atomic_inc(&page->_count);
>   }
>   
>   /* reference to __meminit __free_pages_bootmem is valid

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yasuaki Ishimatsu Oct. 1, 2012, 3:03 a.m. UTC | #2 
Hi Chen,

2012/09/29 11:15, Ni zhan Chen wrote:
> On 09/05/2012 05:25 PM, wency@cn.fujitsu.com wrote:
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> The function get_page_bootmem() may be called more than one time to the same
>> page. There is no need to set page's type, private if the function is not
>> the first time called to the page.
>>
>> Note: the patch is just optimization and does not fix any problem.
>
> Hi Yasuaki,
>
> this patch is reasonable to me. I have another question associated to get_page_bootmem(), the question is from another fujitsu guy's patch changelog [commit : 04753278769f3], the changelog said  that:
>
>   1) When the memmap of removing section is allocated on other
>       section by bootmem, it should/can be free.
>   2) When the memmap of removing section is allocated on the
>       same section, it shouldn't be freed. Because the section has to be
>       logical memory offlined already and all pages must be isolated against
>       page allocater. If it is freed, page allocator may use it which will
>       be removed physically soon.
>
> but I don't see his patch guarantee 2), it means that his patch doesn't guarantee the memmap of removing section which is allocated on other section by bootmem doesn't be freed. Hopefully get your explaination in details, thanks in advance. :-)

In my understanding, the patch does not guarantee it.
Please see [commit : 0c0a4a517a31e]. free_map_bootmem() in the commit
guarantees it.

Thanks,
Yasuaki Ishimatsu

>
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> ---
>>   mm/memory_hotplug.c |   15 +++++++++++----
>>   1 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index d736df3..26a5012 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -95,10 +95,17 @@ static void release_memory_resource(struct resource *res)
>>   static void get_page_bootmem(unsigned long info,  struct page *page,
>>                    unsigned long type)
>>   {
>> -    page->lru.next = (struct list_head *) type;
>> -    SetPagePrivate(page);
>> -    set_page_private(page, info);
>> -    atomic_inc(&page->_count);
>> +    unsigned long page_type;
>> +
>> +    page_type = (unsigned long)page->lru.next;
>> +    if (page_type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
>> +        page_type > MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE){
>> +        page->lru.next = (struct list_head *)type;
>> +        SetPagePrivate(page);
>> +        set_page_private(page, info);
>> +        atomic_inc(&page->_count);
>> +    } else
>> +        atomic_inc(&page->_count);
>>   }
>>   /* reference to __meminit __free_pages_bootmem is valid
>


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ni zhan Chen Oct. 2, 2012, 12:24 p.m. UTC | #3 
On 10/01/2012 11:03 AM, Yasuaki Ishimatsu wrote:
> Hi Chen,
>
> 2012/09/29 11:15, Ni zhan Chen wrote:
>> On 09/05/2012 05:25 PM, wency@cn.fujitsu.com wrote:
>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>
>>> The function get_page_bootmem() may be called more than one time to 
>>> the same
>>> page. There is no need to set page's type, private if the function 
>>> is not
>>> the first time called to the page.
>>>
>>> Note: the patch is just optimization and does not fix any problem.
>>
>> Hi Yasuaki,
>>
>> this patch is reasonable to me. I have another question associated to 
>> get_page_bootmem(), the question is from another fujitsu guy's patch 
>> changelog [commit : 04753278769f3], the changelog said  that:
>>
>>   1) When the memmap of removing section is allocated on other
>>       section by bootmem, it should/can be free.
>>   2) When the memmap of removing section is allocated on the
>>       same section, it shouldn't be freed. Because the section has to be
>>       logical memory offlined already and all pages must be isolated 
>> against
>>       page allocater. If it is freed, page allocator may use it which 
>> will
>>       be removed physically soon.
>>
>> but I don't see his patch guarantee 2), it means that his patch 
>> doesn't guarantee the memmap of removing section which is allocated 
>> on other section by bootmem doesn't be freed. Hopefully get your 
>> explaination in details, thanks in advance. :-)
>
> In my understanding, the patch does not guarantee it.
> Please see [commit : 0c0a4a517a31e]. free_map_bootmem() in the commit
> guarantees it.

Thanks Yasuaki, I have already seen the commit you mentioned. But the 
changelog of the commit I point out 2), why it said that "If it is 
freed, page allocator may use it which will be removed physically soon", 
does it mean that use-after-free ? AFAK, the isolated pages will be free 
if no users use it, so why not free the associated memmap?

>
> Thanks,
> Yasuaki Ishimatsu
>
>>
>>>
>>> CC: David Rientjes <rientjes@google.com>
>>> CC: Jiang Liu <liuj97@gmail.com>
>>> CC: Len Brown <len.brown@intel.com>
>>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> CC: Paul Mackerras <paulus@samba.org>
>>> CC: Christoph Lameter <cl@linux.com>
>>> Cc: Minchan Kim <minchan.kim@gmail.com>
>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>> CC: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>> ---
>>>   mm/memory_hotplug.c |   15 +++++++++++----
>>>   1 files changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index d736df3..26a5012 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -95,10 +95,17 @@ static void release_memory_resource(struct 
>>> resource *res)
>>>   static void get_page_bootmem(unsigned long info,  struct page *page,
>>>                    unsigned long type)
>>>   {
>>> -    page->lru.next = (struct list_head *) type;
>>> -    SetPagePrivate(page);
>>> -    set_page_private(page, info);
>>> -    atomic_inc(&page->_count);
>>> +    unsigned long page_type;
>>> +
>>> +    page_type = (unsigned long)page->lru.next;
>>> +    if (page_type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
>>> +        page_type > MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE){
>>> +        page->lru.next = (struct list_head *)type;
>>> +        SetPagePrivate(page);
>>> +        set_page_private(page, info);
>>> +        atomic_inc(&page->_count);
>>> +    } else
>>> +        atomic_inc(&page->_count);
>>>   }
>>>   /* reference to __meminit __free_pages_bootmem is valid
>>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index d736df3..26a5012 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -95,10 +95,17 @@  static void release_memory_resource(struct resource *res)
 static void get_page_bootmem(unsigned long info,  struct page *page,
 			     unsigned long type)
 {
-	page->lru.next = (struct list_head *) type;
-	SetPagePrivate(page);
-	set_page_private(page, info);
-	atomic_inc(&page->_count);
+	unsigned long page_type;
+
+	page_type = (unsigned long)page->lru.next;
+	if (page_type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
+	    page_type > MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE){
+		page->lru.next = (struct list_head *)type;
+		SetPagePrivate(page);
+		set_page_private(page, info);
+		atomic_inc(&page->_count);
+	} else
+		atomic_inc(&page->_count);
 }
 
 /* reference to __meminit __free_pages_bootmem is valid