[PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Wang, Xiaoming]

(edev->extcon_dev_type.groups) has been freed before device_unregister.
extcon_dev_unregister -> kfree(edev->extcon_dev_type.groups)
then device_unregister -> device_del -> device_remove_attrs
-> device_remove_groups(dev, type->groups);
panic because type->groups has been freed.

This patch is move device_unregister ahead of groups free
to avoid panic in extcon_dev_unregister.

stack
[ 22.847226] BUG: unable to handle kernel paging request at 5f39746e
[ 22.847234] IP: [<c1387fcd>] sysfs_remove_group+0x2d/0xd0
[ 22.847238] *pdpt = 0000000000000000 *pde = 0000000000000000
[ 22.847241] Oops: 0000 [#1] PREEMPT SMP
[ 22.847244] Modules linked in:
[ 22.847249] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 3.10.16-261140-g6533774 #1
[ 22.847251] task: f3078000 ti: f3072000 task.ti: f3072000
[ 22.847254] EIP: 0060:[<c1387fcd>] EFLAGS: 00010206 CPU: 0
[ 22.847257] EIP is at sysfs_remove_group+0x2d/0xd0
[ 22.847259] EAX: 00000004 EBX: 5f39746e ECX: 00000000 EDX: f2773560
[ 22.847261] ESI: f2653b80 EDI: f2773560 EBP: f3073c90 ESP: f3073c70
[ 22.847263] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 22.847264] CR0: 8005003b CR2: 5f39746e CR3: 020e5000 CR4: 001007f0
[ 22.847266] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 22.847268] DR6: ffff0ff0 DR7: 00000400
[ 22.847269] Stack:
[ 22.847276] c13848c9 c1ae3805 f3073c80 f24ddc4c 00000246 f2773e88 f1c44408 f2531340
[ 22.847283] f3073ca0 c16935ca f1c44400 f27d3340 f3073cb4 c1693858 f24ddc44 f1c44400
[ 22.847289] f1c4420c f3073cc8 c1693f76 f1c44400 00000001 00000000 f3073ce8 c1694011
[ 22.847290] Call Trace:
[ 22.847295] [<c13848c9>] ? sysfs_hash_and_remove+0x49/0xa0
[ 22.847300] [<c1ae3805>] ? sub_preempt_count+0x55/0xe0
[ 22.847306] [<c16935ca>] device_remove_groups+0x2a/0x40
[ 22.847309] [<c1693858>] device_remove_attrs+0x38/0x60
[ 22.847313] [<c1693f76>] device_del+0xd6/0x150
[ 22.847316] [<c1694011>] device_unregister+0x21/0x60
[ 22.847320] [<c13869e8>] ? sysfs_remove_link+0x18/0x30
[ 22.847323] [<c1697d04>] ? class_compat_remove_link+0x34/0x50
[ 22.847329] [<c18bf5d9>] extcon_dev_unregister+0xf9/0x130
[ 22.847333] [<c18bd21f>] pwrsrc_extcon_dev_reg_callback+0x7f/0xa0
[ 22.847337] [<c1ae36e3>] notifier_call_chain+0x43/0x60
[ 22.847343] [<c12634e1>] __blocking_notifier_call_chain+0x41/0x80
[ 22.847347] [<c126353f>] blocking_notifier_call_chain+0x1f/0x30
[ 22.847351] [<c18bef39>] extcon_dev_notify_add_device+0x19/0x20
[ 22.847354] [<c18bf074>] extcon_dev_register+0x134/0x580
[ 22.847358] [<c1ae3805>] ? sub_preempt_count+0x55/0xe0
[ 22.847363] [<c154b4da>] ? gpiod_request+0x6a/0x1d0
[ 22.847368] [<c132528a>] ? kmem_cache_alloc_trace+0xaa/0x170
[ 22.847372] [<c18c0179>] ? fsa9285_probe+0x99/0x3f0
[ 22.847375] [<c18c00e0>] ? fsa9285_irq_handler+0xf0/0xf0
[ 22.847379] [<c18c019f>] fsa9285_probe+0xbf/0x3f0
[ 22.847383] [<c18c00e0>] ? fsa9285_irq_handler+0xf0/0xf0
[ 22.847388] [<c1828d9e>] i2c_device_probe+0x7e/0xf0
[ 22.847392] [<c1386e52>] ? sysfs_create_link+0x22/0x40
[ 22.847395] [<c1696c62>] ? driver_sysfs_add+0x72/0xa0
[ 22.847399] [<c1697119>] driver_probe_device+0x79/0x360
[ 22.847403] [<c1697491>] __driver_attach+0x91/0xa0
[ 22.847407] [<c1697400>] ? driver_probe_device+0x360/0x360
[ 22.847410] [<c16955a2>] bus_for_each_dev+0x42/0x80
[ 22.847414] [<c1696bee>] driver_attach+0x1e/0x20
[ 22.847417] [<c1697400>] ? driver_probe_device+0x360/0x360
[ 22.847420] [<c169675f>] bus_add_driver+0xef/0x270
[ 22.847425] [<c1828e10>] ? i2c_device_probe+0xf0/0xf0
[ 22.847428] [<c1828e10>] ? i2c_device_probe+0xf0/0xf0
[ 22.847432] [<c1697a8a>] driver_register+0x6a/0x160
[ 22.847436] [<c1addb3d>] ? mutex_unlock+0xd/0x10
[ 22.847440] [<c1438ab2>] ? __create_file+0x122/0x2a0
[ 22.847446] [<c20557d3>] ? extcon_class_init+0x15/0x15
[ 22.847450] [<c1827cbb>] i2c_register_driver+0x2b/0xd0
[ 22.847454] [<c1438d05>] ? debugfs_create_file+0x35/0x40
[ 22.847458] [<c20557d3>] ? extcon_class_init+0x15/0x15
[ 22.847461] [<c20557e4>] fsa9285_extcon_init+0x11/0x29
[ 22.847465] [<c12001aa>] do_one_initcall+0xba/0x170
[ 22.847471] [<c2012b4a>] kernel_init_freeable+0x119/0x1b8
[ 22.847475] [<c20124d3>] ? do_early_param+0x7a/0x7a
[ 22.847480] [<c1ac4090>] kernel_init+0x10/0xd0
[ 22.847485] [<c1ae6cf7>] ret_from_kernel_thread+0x1b/0x28
[ 22.847488] [<c1ac4080>] ? rest_init+0x80/0x80

Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
---
 drivers/extcon/extcon-class.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 148382f..48f4669 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -794,6 +794,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
 		return;
 	}
 
+	device_unregister(edev->dev);
+
 	if (edev->mutually_exclusive && edev->max_supported) {
 		for (index = 0; edev->mutually_exclusive[index];
 				index++)
@@ -814,7 +816,6 @@ void extcon_dev_unregister(struct extcon_dev *edev)
 	if (switch_class)
 		class_compat_remove_link(switch_class, edev->dev, NULL);
 #endif
-	device_unregister(edev->dev);
 	put_device(edev->dev);
 }
 EXPORT_SYMBOL_GPL(extcon_dev_unregister);
-- 
1.7.1

RE: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 959 bytes --]



> -----Original Message-----
> From: Wang, Xiaoming
> Sent: Saturday, November 02, 2013 6:48 AM
> To: myungjoo.ham@samsung.com; cw00.choi@samsung.com;
> linux-kernel@vger.kernel.org
> Cc: Liu, Chuansheng; Zhang, Dongxing; Wang, Xiaoming
> Subject: [PATCH] [extcon]:remove freed groups caused the panic or warning in
> unregister flow
> 
> (edev->extcon_dev_type.groups) has been freed before device_unregister.
> extcon_dev_unregister -> kfree(edev->extcon_dev_type.groups)
> then device_unregister -> device_del -> device_remove_attrs
> -> device_remove_groups(dev, type->groups);
> panic because type->groups has been freed.
> 
This patch fix the panic that using the freed memory.

Tested-by: Liu, Chuansheng <chuansheng.liu@intel.com>
Reviewed-by: Liu, Chuansheng <chuansheng.liu@intel.com>


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Chanwoo Choi]

Hi Wang,

>  drivers/extcon/extcon-class.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 148382f..48f4669 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -794,6 +794,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  		return;
>  	}
>  
> +	device_unregister(edev->dev);
> +
>  	if (edev->mutually_exclusive && edev->max_supported) {
>  		for (index = 0; edev->mutually_exclusive[index];
>  				index++)
> @@ -814,7 +816,6 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  	if (switch_class)
>  		class_compat_remove_link(switch_class, edev->dev, NULL);
>  #endif
> -	device_unregister(edev->dev);
>  	put_device(edev->dev);
>  }
>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
> 

I think we could only apply following patch instead of moving the position of device_unregister().

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 148382f..ff27b19 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -805,10 +805,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
        for (index = 0; index < edev->max_supported; index++)
                kfree(edev->cables[index].attr_g.name);
 
-       if (edev->max_supported) {
-               kfree(edev->extcon_dev_type.groups);
+       if (edev->max_supported)
                kfree(edev->cables);
-       }
 
 #if defined(CONFIG_ANDROID)
        if (switch_class)

Thanks,
Chanwoo Choi

RE: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Wang, Xiaoming]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2472 bytes --]

Dear Choi

-----Original Message-----
From: Chanwoo Choi [mailto:cw00.choi@samsung.com] 
Sent: Monday, November 04, 2013 9:43 AM
To: Wang, Xiaoming
Cc: myungjoo.ham@samsung.com; linux-kernel@vger.kernel.org; Liu, Chuansheng; Zhang, Dongxing
Subject: Re: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

Hi Wang,

>  drivers/extcon/extcon-class.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-class.c 
> b/drivers/extcon/extcon-class.c index 148382f..48f4669 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -794,6 +794,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  		return;
>  	}
>  
> +	device_unregister(edev->dev);
> +
>  	if (edev->mutually_exclusive && edev->max_supported) {
>  		for (index = 0; edev->mutually_exclusive[index];
>  				index++)
> @@ -814,7 +816,6 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  	if (switch_class)
>  		class_compat_remove_link(switch_class, edev->dev, NULL);  #endif
> -	device_unregister(edev->dev);
>  	put_device(edev->dev);
>  }
>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
> 

I think we could only apply following patch instead of moving the position of device_unregister().

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index 148382f..ff27b19 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -805,10 +805,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
        for (index = 0; index < edev->max_supported; index++)
                kfree(edev->cables[index].attr_g.name);
 
-       if (edev->max_supported) {
-               kfree(edev->extcon_dev_type.groups);
+       if (edev->max_supported)
                kfree(edev->cables);
-       }
 
 #if defined(CONFIG_ANDROID)
        if (switch_class)

Thanks,
Chanwoo Choi

	I don't agree with you.
	Why do not you want moving the position of device_unregister()?
	It will cause the memory leak if has not kfree edev->extcon_dev_type.groups as your patch do firstly. And if you think kfree edev->extcon_dev_type.groups is meaningless well then kfree edev->extcon_dev_type.groups in function exton_dev_register (line 756)also should be removed I think. What do you think?

Thanks
Xiaoming.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Chanwoo Choi]

On 11/04/2013 11:07 AM, Wang, Xiaoming wrote:
> Dear Choi
> 
> -----Original Message-----
> From: Chanwoo Choi [mailto:cw00.choi@samsung.com] 
> Sent: Monday, November 04, 2013 9:43 AM
> To: Wang, Xiaoming
> Cc: myungjoo.ham@samsung.com; linux-kernel@vger.kernel.org; Liu, Chuansheng; Zhang, Dongxing
> Subject: Re: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow
> 
> Hi Wang,
> 
>>  drivers/extcon/extcon-class.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-class.c 
>> b/drivers/extcon/extcon-class.c index 148382f..48f4669 100644
>> --- a/drivers/extcon/extcon-class.c
>> +++ b/drivers/extcon/extcon-class.c
>> @@ -794,6 +794,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>  		return;
>>  	}
>>  
>> +	device_unregister(edev->dev);
>> +
>>  	if (edev->mutually_exclusive && edev->max_supported) {
>>  		for (index = 0; edev->mutually_exclusive[index];
>>  				index++)
>> @@ -814,7 +816,6 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>  	if (switch_class)
>>  		class_compat_remove_link(switch_class, edev->dev, NULL);  #endif
>> -	device_unregister(edev->dev);
>>  	put_device(edev->dev);
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>>
> 
> I think we could only apply following patch instead of moving the position of device_unregister().
> 
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index 148382f..ff27b19 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -805,10 +805,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>         for (index = 0; index < edev->max_supported; index++)
>                 kfree(edev->cables[index].attr_g.name);
>  
> -       if (edev->max_supported) {
> -               kfree(edev->extcon_dev_type.groups);
> +       if (edev->max_supported)
>                 kfree(edev->cables);
> -       }
>  
>  #if defined(CONFIG_ANDROID)
>         if (switch_class)
> 
> Thanks,
> Chanwoo Choi
> 
> 	I don't agree with you.
> 	Why do not you want moving the position of device_unregister()?
> 	It will cause the memory leak if has not kfree edev->extcon_dev_type.groups as your patch do firstly. And if you think kfree edev->extcon_dev_type.groups is meaningless well then kfree edev->extcon_dev_type.groups in function exton_dev_register (line 756)also should be removed I think. What do you think?
> 

As you comment, my opinion has memory leak problem. My mistake.
But, I prefer to call 'device_unregister' at the end of extcon_dev_unregister().
To resolve kernel panic, I think we could use 'devm_kzalloc' instead of kzalloc/kfree.
What is your opinion about my approach?

Thanks,
Chanwoo Choi

RE: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 988 bytes --]



> -----Original Message-----
> From: Chanwoo Choi [mailto:cw00.choi@samsung.com]
> Sent: Monday, November 04, 2013 10:37 AM
> To: Wang, Xiaoming
> Cc: myungjoo.ham@samsung.com; linux-kernel@vger.kernel.org; Liu,
> Chuansheng; Zhang, Dongxing
> Subject: Re: [PATCH] [extcon]:remove freed groups caused the panic or warning
> in unregister flow


> As you comment, my opinion has memory leak problem. My mistake.
> But, I prefer to call 'device_unregister' at the end of extcon_dev_unregister().
> To resolve kernel panic, I think we could use 'devm_kzalloc' instead of
> kzalloc/kfree.
> What is your opinion about my approach?
> 
That sounds good.
But in function extcon_dev_register(), the device attributes are allocated before device_register(),
and before device_register(), the function devm_kzalloc() can not be used.


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] [extcon]:remove freed groups caused the panic or warning in unregister flow

[Chanwoo Choi]

On 11/04/2013 12:47 PM, Liu, Chuansheng wrote:
> 
> 
>> -----Original Message-----
>> From: Chanwoo Choi [mailto:cw00.choi@samsung.com]
>> Sent: Monday, November 04, 2013 10:37 AM
>> To: Wang, Xiaoming
>> Cc: myungjoo.ham@samsung.com; linux-kernel@vger.kernel.org; Liu,
>> Chuansheng; Zhang, Dongxing
>> Subject: Re: [PATCH] [extcon]:remove freed groups caused the panic or warning
>> in unregister flow
> 
> 
>> As you comment, my opinion has memory leak problem. My mistake.
>> But, I prefer to call 'device_unregister' at the end of extcon_dev_unregister().
>> To resolve kernel panic, I think we could use 'devm_kzalloc' instead of
>> kzalloc/kfree.
>> What is your opinion about my approach?
>>
> That sounds good.
> But in function extcon_dev_register(), the device attributes are allocated before device_register(),
> and before device_register(), the function devm_kzalloc() can not be used.
> 

We could use 'edev->dev->parent' pointer for devm_kzalloc()
but it is ambiguous method.

OK, applied this patch.

Thanks,
Chanwoo Choi