lkml.org 
[lkml]   [2012]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/4] memory-hotplug: add memory_block_release
Hi Chen,

2012/09/28 15:04, Ni zhan Chen wrote:
> On 09/28/2012 11:45 AM, Yasuaki Ishimatsu wrote:
>> Hi Kosaki-san,
>>
>> 2012/09/28 10:35, KOSAKI Motohiro wrote:
>>> On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
>>> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>>>> Hi Chen,
>>>>
>>>>
>>>> 2012/09/27 19:20, Ni zhan Chen wrote:
>>>>>
>>>>> Hi Congyang,
>>>>>
>>>>> 2012/9/27 <wency@cn.fujitsu.com>
>>>>>
>>>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>>>
>>>>>> When calling remove_memory_block(), the function shows following message
>>>>>> at
>>>>>> device_release().
>>>>>>
>>>>>> Device 'memory528' does not have a release() function, it is broken and
>>>>>> must
>>>>>> be fixed.
>>>>>>
>>>>>
>>>>> What's the difference between the patch and original implemetation?
>>>>
>>>>
>>>> The implementation is for removing a memory_block. So the purpose is
>>>> same as original one. But original code is bad manner. kobject_cleanup()
>>>> is called by remove_memory_block() at last. But release function for
>>>> releasing memory_block is not registered. As a result, the kernel message
>>>> is shown. IMHO, memory_block should be release by the releae function.
>>>
>>> but your patch introduced use after free bug, if i understand correctly.
>>> See unregister_memory() function. After your patch, kobject_put() call
>>> release_memory_block() and kfree(). and then device_unregister() will
>>> touch freed memory.
>>
>
> this patch is similiar to [RFC v9 PATCH 10/21] memory-hotplug: add memory_block_release, they handle the same issue, can these two patches be fold to one?

You're right. The patch is same as [RFC v9 PATCH 10/21].
The patch is a bug fix. So we separated it from memory-hotplug patch-set.

Thanks,
Yasuaki Ishimatsu

>> It is not correct. The kobject_put() is prepared against find_memory_block()
>> in remove_memory_block() since kobject->kref is incremented in it.
>> So release_memory_block() is called by device_unregister() correctly as follows:
>>
>> [ 1014.589008] Pid: 126, comm: kworker/0:2 Not tainted 3.6.0-rc3-enable-memory-hotremove-and-root-bridge #3
>> [ 1014.702437] Call Trace:
>> [ 1014.731684] [<ffffffff8144d096>] release_memory_block+0x16/0x30
>> [ 1014.803581] [<ffffffff81438587>] device_release+0x27/0xa0
>> [ 1014.869312] [<ffffffff8133e962>] kobject_cleanup+0x82/0x1b0
>> [ 1014.937062] [<ffffffff8133ea9d>] kobject_release+0xd/0x10
>> [ 1015.002718] [<ffffffff8133e7ec>] kobject_put+0x2c/0x60
>> [ 1015.065271] [<ffffffff81438107>] put_device+0x17/0x20
>> [ 1015.126794] [<ffffffff8143918a>] device_unregister+0x2a/0x60
>> [ 1015.195578] [<ffffffff8144d55b>] remove_memory_block+0xbb/0xf0
>> [ 1015.266434] [<ffffffff8144d5af>] unregister_memory_section+0x1f/0x30
>> [ 1015.343532] [<ffffffff811c0a58>] __remove_section+0x68/0x110
>> [ 1015.412318] [<ffffffff811c0be7>] __remove_pages+0xe7/0x120
>> [ 1015.479021] [<ffffffff81653d8c>] arch_remove_memory+0x2c/0x80
>> [ 1015.548845] [<ffffffff8165497b>] remove_memory+0x6b/0xd0
>> [ 1015.613474] [<ffffffff813d946c>] acpi_memory_device_remove_memory+0x48/0x73
>> [ 1015.697834] [<ffffffff813d94c2>] acpi_memory_device_remove+0x2b/0x44
>> [ 1015.774922] [<ffffffff813a61e4>] acpi_device_remove+0x90/0xb2
>> [ 1015.844796] [<ffffffff8143c2fc>] __device_release_driver+0x7c/0xf0
>> [ 1015.919814] [<ffffffff8143c47f>] device_release_driver+0x2f/0x50
>> [ 1015.992753] [<ffffffff813a70dc>] acpi_bus_remove+0x32/0x6d
>> [ 1016.059462] [<ffffffff813a71a8>] acpi_bus_trim+0x91/0x102
>> [ 1016.125128] [<ffffffff813a72a1>] acpi_bus_hot_remove_device+0x88/0x16b
>> [ 1016.204295] [<ffffffff813a2e57>] acpi_os_execute_deferred+0x27/0x34
>> [ 1016.280350] [<ffffffff81090599>] process_one_work+0x219/0x680
>> [ 1016.350173] [<ffffffff81090538>] ? process_one_work+0x1b8/0x680
>> [ 1016.422072] [<ffffffff813a2e30>] ? acpi_os_wait_events_complete+0x23/0x23
>> [ 1016.504357] [<ffffffff810923ce>] worker_thread+0x12e/0x320
>> [ 1016.571064] [<ffffffff810922a0>] ? manage_workers+0x110/0x110
>> [ 1016.640886] [<ffffffff810983a6>] kthread+0xc6/0xd0
>> [ 1016.699290] [<ffffffff8167b144>] kernel_thread_helper+0x4/0x10
>> [ 1016.770149] [<ffffffff81670bb0>] ? retint_restore_args+0x13/0x13
>> [ 1016.843165] [<ffffffff810982e0>] ? __init_kthread_worker+0x70/0x70
>> [ 1016.918200] [<ffffffff8167b140>] ? gs_change+0x13/0x13
>>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>>
>>> static void
>>> unregister_memory(struct memory_block *memory)
>>> {
>>> BUG_ON(memory->dev.bus != &memory_subsys);
>>>
>>> /* drop the ref. we got in remove_memory_block() */
>>> kobject_put(&memory->dev.kobj);
>>> device_unregister(&memory->dev);
>>> }
>>>
>>
>>
>>
>




\
 
 \ /
  Last update: 2012-09-28 09:01    [W:0.033 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site