diff mbox series

ipmi: use dynamic memory for DMI driver override

Message ID 1516207017-158659-1-git-send-email-john.garry@huawei.com
State Accepted
Commit 5516e21a1e95e9b9f39985598431a25477d91643
Headers show
Series ipmi: use dynamic memory for DMI driver override | expand

Commit Message

John Garry Jan. 17, 2018, 4:36 p.m. UTC 
Currently a crash can be seen if we reach the "err"
label in dmi_add_platform_ipmi(), calling
platform_device_put(), like here:
[    7.270584]  (null): ipmi:dmi: Unable to add resources: -16
[    7.330229] ------------[ cut here ]------------
[    7.334889] kernel BUG at mm/slub.c:3894!
[    7.338936] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[    7.344475] Modules linked in:
[    7.347556] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc2-00004-gbe9cb7b-dirty #114
[    7.355907] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT17 Nemo 2.0 RC0 11/29/2017
[    7.365137] task: 00000000c211f6d3 task.stack: 00000000f276e9af
[    7.371116] pstate: 60000005 (nZCv daif -PAN -UAO)
[    7.375957] pc : kfree+0x194/0x1b4
[    7.379389] lr : platform_device_release+0xcc/0xd8
[    7.384225] sp : ffff0000092dba90
[    7.387567] x29: ffff0000092dba90 x28: ffff000008a83000
[    7.392933] x27: ffff0000092dbc10 x26: 00000000000000e6
[    7.398297] x25: 0000000000000003 x24: ffff0000085b51e8
[    7.403662] x23: 0000000000000100 x22: ffff7e0000234cc0
[    7.409027] x21: ffff000008af3660 x20: ffff8017d21acc10
[    7.414392] x19: ffff8017d21acc00 x18: 0000000000000002
[    7.419757] x17: 0000000000000001 x16: 0000000000000008
[    7.425121] x15: 0000000000000001 x14: 6666666678303d65
[    7.430486] x13: 6469727265766f5f x12: 7265766972642e76
[    7.435850] x11: 6564703e2d617020 x10: 6530326435373638
[    7.441215] x9 : 3030303030303030 x8 : 3d76656420657361
[    7.446580] x7 : ffff000008f59df8 x6 : ffff8017fbe0ea50
[    7.451945] x5 : 0000000000000000 x4 : 0000000000000000
[    7.457309] x3 : ffffffffffffffff x2 : 0000000000000000
[    7.462674] x1 : 0fffc00000000800 x0 : ffff7e0000234ce0
[    7.468039] Process swapper/0 (pid: 1, stack limit = 0x00000000f276e9af)
[    7.474809] Call trace:
[    7.477272]  kfree+0x194/0x1b4
[    7.480351]  platform_device_release+0xcc/0xd8
[    7.484837]  device_release+0x34/0x90
[    7.488531]  kobject_put+0x70/0xcc
[    7.491961]  put_device+0x14/0x1c
[    7.495304]  platform_device_put+0x14/0x1c
[    7.499439]  dmi_add_platform_ipmi+0x348/0x3ac
[    7.503923]  scan_for_dmi_ipmi+0xfc/0x10c
[    7.507970]  do_one_initcall+0x38/0x124
[    7.511840]  kernel_init_freeable+0x188/0x228
[    7.516238]  kernel_init+0x10/0x100
[    7.519756]  ret_from_fork+0x10/0x18
[    7.523362] Code: f94002c0 37780080 f94012c0 37000040 (d4210000)
[    7.529552] ---[ end trace 11750e4787deef9e ]---
[    7.534228] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    7.534228]

This is because when the device is released in
platform_device_release(), we try to free
pdev.driver_override. This is a const string, hence
the crash.
Fix by using dynamic memory for pdev->driver_override.

Signed-off-by: John Garry <john.garry@huawei.com>


-- 
1.9.1

Comments

Corey Minyard Jan. 17, 2018, 4:49 p.m. UTC | #1 
On 01/17/2018 10:36 AM, John Garry wrote:
> Currently a crash can be seen if we reach the "err"

> label in dmi_add_platform_ipmi(), calling

> platform_device_put(), like here:

> [    7.270584]  (null): ipmi:dmi: Unable to add resources: -16

> [    7.330229] ------------[ cut here ]------------

> [    7.334889] kernel BUG at mm/slub.c:3894!

> [    7.338936] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP

> [    7.344475] Modules linked in:

> [    7.347556] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc2-00004-gbe9cb7b-dirty #114

> [    7.355907] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT17 Nemo 2.0 RC0 11/29/2017

> [    7.365137] task: 00000000c211f6d3 task.stack: 00000000f276e9af

> [    7.371116] pstate: 60000005 (nZCv daif -PAN -UAO)

> [    7.375957] pc : kfree+0x194/0x1b4

> [    7.379389] lr : platform_device_release+0xcc/0xd8

> [    7.384225] sp : ffff0000092dba90

> [    7.387567] x29: ffff0000092dba90 x28: ffff000008a83000

> [    7.392933] x27: ffff0000092dbc10 x26: 00000000000000e6

> [    7.398297] x25: 0000000000000003 x24: ffff0000085b51e8

> [    7.403662] x23: 0000000000000100 x22: ffff7e0000234cc0

> [    7.409027] x21: ffff000008af3660 x20: ffff8017d21acc10

> [    7.414392] x19: ffff8017d21acc00 x18: 0000000000000002

> [    7.419757] x17: 0000000000000001 x16: 0000000000000008

> [    7.425121] x15: 0000000000000001 x14: 6666666678303d65

> [    7.430486] x13: 6469727265766f5f x12: 7265766972642e76

> [    7.435850] x11: 6564703e2d617020 x10: 6530326435373638

> [    7.441215] x9 : 3030303030303030 x8 : 3d76656420657361

> [    7.446580] x7 : ffff000008f59df8 x6 : ffff8017fbe0ea50

> [    7.451945] x5 : 0000000000000000 x4 : 0000000000000000

> [    7.457309] x3 : ffffffffffffffff x2 : 0000000000000000

> [    7.462674] x1 : 0fffc00000000800 x0 : ffff7e0000234ce0

> [    7.468039] Process swapper/0 (pid: 1, stack limit = 0x00000000f276e9af)

> [    7.474809] Call trace:

> [    7.477272]  kfree+0x194/0x1b4

> [    7.480351]  platform_device_release+0xcc/0xd8

> [    7.484837]  device_release+0x34/0x90

> [    7.488531]  kobject_put+0x70/0xcc

> [    7.491961]  put_device+0x14/0x1c

> [    7.495304]  platform_device_put+0x14/0x1c

> [    7.499439]  dmi_add_platform_ipmi+0x348/0x3ac

> [    7.503923]  scan_for_dmi_ipmi+0xfc/0x10c

> [    7.507970]  do_one_initcall+0x38/0x124

> [    7.511840]  kernel_init_freeable+0x188/0x228

> [    7.516238]  kernel_init+0x10/0x100

> [    7.519756]  ret_from_fork+0x10/0x18

> [    7.523362] Code: f94002c0 37780080 f94012c0 37000040 (d4210000)

> [    7.529552] ---[ end trace 11750e4787deef9e ]---

> [    7.534228] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

> [    7.534228]

>

> This is because when the device is released in

> platform_device_release(), we try to free

> pdev.driver_override. This is a const string, hence

> the crash.

> Fix by using dynamic memory for pdev->driver_override.

>

> Signed-off-by: John Garry <john.garry@huawei.com>


Oops, sorry about that.  Yes, queued for the next release and 4.14.x.

-corey

>

> diff --git a/drivers/char/ipmi/ipmi_dmi.c b/drivers/char/ipmi/ipmi_dmi.c

> index ab78b3b..c5112b1 100644

> --- a/drivers/char/ipmi/ipmi_dmi.c

> +++ b/drivers/char/ipmi/ipmi_dmi.c

> @@ -106,7 +106,10 @@ static void __init dmi_add_platform_ipmi(unsigned long base_addr,

>   		pr_err("ipmi:dmi: Error allocation IPMI platform device\n");

>   		return;

>   	}

> -	pdev->driver_override = override;

> +	pdev->driver_override = kasprintf(GFP_KERNEL, "%s",

> +					  override);

> +	if (!pdev->driver_override)

> +		goto err;

>   

>   	if (type == IPMI_DMI_TYPE_SSIF) {

>   		set_prop_entry(p[pidx++], "i2c-addr", u16, base_addr);

> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c

> index 779869e..2a19aa8 100644

> --- a/drivers/char/ipmi/ipmi_si_intf.c

> +++ b/drivers/char/ipmi/ipmi_si_intf.c

> @@ -2426,6 +2426,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>   {

>   	struct smi_info *e;

>   	int rv = -ENOENT;

> +	struct platform_device *pdev = to_platform_device(dev);

>   

>   	mutex_lock(&smi_infos_lock);

>   	list_for_each_entry(e, &smi_infos, link) {

> @@ -2436,6 +2437,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>   		}

>   	}

>   	mutex_unlock(&smi_infos_lock);

> +	kfree(pdev->driver_override);

>   

>   	return rv;

>   }
John Garry Jan. 17, 2018, 5:06 p.m. UTC | #2 
On 17/01/2018 16:49, Corey Minyard wrote:
> On 01/17/2018 10:36 AM, John Garry wrote:

>> Currently a crash can be seen if we reach the "err"

>> label in dmi_add_platform_ipmi(), calling

>> platform_device_put(), like here:

>> [    7.270584]  (null): ipmi:dmi: Unable to add resources: -16

>> [    7.330229] ------------[ cut here ]------------

>> [    7.334889] kernel BUG at mm/slub.c:3894!

>> [    7.338936] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP

>> [    7.344475] Modules linked in:

>> [    7.347556] CPU: 1 PID: 1 Comm: swapper/0 Not tainted

>> 4.15.0-rc2-00004-gbe9cb7b-dirty #114

>> [    7.355907] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon

>> D05 IT17 Nemo 2.0 RC0 11/29/2017

>> [    7.365137] task: 00000000c211f6d3 task.stack: 00000000f276e9af

>> [    7.371116] pstate: 60000005 (nZCv daif -PAN -UAO)

>> [    7.375957] pc : kfree+0x194/0x1b4

>> [    7.379389] lr : platform_device_release+0xcc/0xd8

>> [    7.384225] sp : ffff0000092dba90

>> [    7.387567] x29: ffff0000092dba90 x28: ffff000008a83000

>> [    7.392933] x27: ffff0000092dbc10 x26: 00000000000000e6

>> [    7.398297] x25: 0000000000000003 x24: ffff0000085b51e8

>> [    7.403662] x23: 0000000000000100 x22: ffff7e0000234cc0

>> [    7.409027] x21: ffff000008af3660 x20: ffff8017d21acc10

>> [    7.414392] x19: ffff8017d21acc00 x18: 0000000000000002

>> [    7.419757] x17: 0000000000000001 x16: 0000000000000008

>> [    7.425121] x15: 0000000000000001 x14: 6666666678303d65

>> [    7.430486] x13: 6469727265766f5f x12: 7265766972642e76

>> [    7.435850] x11: 6564703e2d617020 x10: 6530326435373638

>> [    7.441215] x9 : 3030303030303030 x8 : 3d76656420657361

>> [    7.446580] x7 : ffff000008f59df8 x6 : ffff8017fbe0ea50

>> [    7.451945] x5 : 0000000000000000 x4 : 0000000000000000

>> [    7.457309] x3 : ffffffffffffffff x2 : 0000000000000000

>> [    7.462674] x1 : 0fffc00000000800 x0 : ffff7e0000234ce0

>> [    7.468039] Process swapper/0 (pid: 1, stack limit =

>> 0x00000000f276e9af)

>> [    7.474809] Call trace:

>> [    7.477272]  kfree+0x194/0x1b4

>> [    7.480351]  platform_device_release+0xcc/0xd8

>> [    7.484837]  device_release+0x34/0x90

>> [    7.488531]  kobject_put+0x70/0xcc

>> [    7.491961]  put_device+0x14/0x1c

>> [    7.495304]  platform_device_put+0x14/0x1c

>> [    7.499439]  dmi_add_platform_ipmi+0x348/0x3ac

>> [    7.503923]  scan_for_dmi_ipmi+0xfc/0x10c

>> [    7.507970]  do_one_initcall+0x38/0x124

>> [    7.511840]  kernel_init_freeable+0x188/0x228

>> [    7.516238]  kernel_init+0x10/0x100

>> [    7.519756]  ret_from_fork+0x10/0x18

>> [    7.523362] Code: f94002c0 37780080 f94012c0 37000040 (d4210000)

>> [    7.529552] ---[ end trace 11750e4787deef9e ]---

>> [    7.534228] Kernel panic - not syncing: Attempted to kill init!

>> exitcode=0x0000000b

>> [    7.534228]

>>

>> This is because when the device is released in

>> platform_device_release(), we try to free

>> pdev.driver_override. This is a const string, hence

>> the crash.

>> Fix by using dynamic memory for pdev->driver_override.

>>

>> Signed-off-by: John Garry <john.garry@huawei.com>

>

> Oops, sorry about that.  Yes, queued for the next release and 4.14.x.

>

> -corey

>

>>

>> diff --git a/drivers/char/ipmi/ipmi_dmi.c b/drivers/char/ipmi/ipmi_dmi.c

>> index ab78b3b..c5112b1 100644

>> --- a/drivers/char/ipmi/ipmi_dmi.c

>> +++ b/drivers/char/ipmi/ipmi_dmi.c

>> @@ -106,7 +106,10 @@ static void __init dmi_add_platform_ipmi(unsigned

>> long base_addr,

>>           pr_err("ipmi:dmi: Error allocation IPMI platform device\n");

>>           return;

>>       }

>> -    pdev->driver_override = override;

>> +    pdev->driver_override = kasprintf(GFP_KERNEL, "%s",

>> +                      override);


It would be simpler to use managed variant (to avoid the kfree()), but I 
don't think it's possible until the device probes.

John

>> +    if (!pdev->driver_override)

>> +        goto err;

>>         if (type == IPMI_DMI_TYPE_SSIF) {

>>           set_prop_entry(p[pidx++], "i2c-addr", u16, base_addr);

>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c

>> b/drivers/char/ipmi/ipmi_si_intf.c

>> index 779869e..2a19aa8 100644

>> --- a/drivers/char/ipmi/ipmi_si_intf.c

>> +++ b/drivers/char/ipmi/ipmi_si_intf.c

>> @@ -2426,6 +2426,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>>   {

>>       struct smi_info *e;

>>       int rv = -ENOENT;

>> +    struct platform_device *pdev = to_platform_device(dev);

>>         mutex_lock(&smi_infos_lock);

>>       list_for_each_entry(e, &smi_infos, link) {

>> @@ -2436,6 +2437,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>>           }

>>       }

>>       mutex_unlock(&smi_infos_lock);

>> +    kfree(pdev->driver_override);

>>         return rv;

>>   }

>

>

>

> .

>
Corey Minyard Jan. 22, 2018, 1:46 p.m. UTC | #3 
On 01/17/2018 11:06 AM, John Garry wrote:
> On 17/01/2018 16:49, Corey Minyard wrote:

>> On 01/17/2018 10:36 AM, John Garry wrote:

>>> Currently a crash can be seen if we reach the "err"

>>> label in dmi_add_platform_ipmi(), calling

>>> platform_device_put(), like here:

>>> [    7.270584]  (null): ipmi:dmi: Unable to add resources: -16

>>> [    7.330229] ------------[ cut here ]------------

>>> [    7.334889] kernel BUG at mm/slub.c:3894!

>>> [    7.338936] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP

>>> [    7.344475] Modules linked in:

>>> [    7.347556] CPU: 1 PID: 1 Comm: swapper/0 Not tainted

>>> 4.15.0-rc2-00004-gbe9cb7b-dirty #114

>>> [    7.355907] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon

>>> D05 IT17 Nemo 2.0 RC0 11/29/2017

>>> [    7.365137] task: 00000000c211f6d3 task.stack: 00000000f276e9af

>>> [    7.371116] pstate: 60000005 (nZCv daif -PAN -UAO)

>>> [    7.375957] pc : kfree+0x194/0x1b4

>>> [    7.379389] lr : platform_device_release+0xcc/0xd8

>>> [    7.384225] sp : ffff0000092dba90

>>> [    7.387567] x29: ffff0000092dba90 x28: ffff000008a83000

>>> [    7.392933] x27: ffff0000092dbc10 x26: 00000000000000e6

>>> [    7.398297] x25: 0000000000000003 x24: ffff0000085b51e8

>>> [    7.403662] x23: 0000000000000100 x22: ffff7e0000234cc0

>>> [    7.409027] x21: ffff000008af3660 x20: ffff8017d21acc10

>>> [    7.414392] x19: ffff8017d21acc00 x18: 0000000000000002

>>> [    7.419757] x17: 0000000000000001 x16: 0000000000000008

>>> [    7.425121] x15: 0000000000000001 x14: 6666666678303d65

>>> [    7.430486] x13: 6469727265766f5f x12: 7265766972642e76

>>> [    7.435850] x11: 6564703e2d617020 x10: 6530326435373638

>>> [    7.441215] x9 : 3030303030303030 x8 : 3d76656420657361

>>> [    7.446580] x7 : ffff000008f59df8 x6 : ffff8017fbe0ea50

>>> [    7.451945] x5 : 0000000000000000 x4 : 0000000000000000

>>> [    7.457309] x3 : ffffffffffffffff x2 : 0000000000000000

>>> [    7.462674] x1 : 0fffc00000000800 x0 : ffff7e0000234ce0

>>> [    7.468039] Process swapper/0 (pid: 1, stack limit =

>>> 0x00000000f276e9af)

>>> [    7.474809] Call trace:

>>> [    7.477272]  kfree+0x194/0x1b4

>>> [    7.480351]  platform_device_release+0xcc/0xd8

>>> [    7.484837]  device_release+0x34/0x90

>>> [    7.488531]  kobject_put+0x70/0xcc

>>> [    7.491961]  put_device+0x14/0x1c

>>> [    7.495304]  platform_device_put+0x14/0x1c

>>> [    7.499439]  dmi_add_platform_ipmi+0x348/0x3ac

>>> [    7.503923]  scan_for_dmi_ipmi+0xfc/0x10c

>>> [    7.507970]  do_one_initcall+0x38/0x124

>>> [    7.511840]  kernel_init_freeable+0x188/0x228

>>> [    7.516238]  kernel_init+0x10/0x100

>>> [    7.519756]  ret_from_fork+0x10/0x18

>>> [    7.523362] Code: f94002c0 37780080 f94012c0 37000040 (d4210000)

>>> [    7.529552] ---[ end trace 11750e4787deef9e ]---

>>> [    7.534228] Kernel panic - not syncing: Attempted to kill init!

>>> exitcode=0x0000000b

>>> [    7.534228]

>>>

>>> This is because when the device is released in

>>> platform_device_release(), we try to free

>>> pdev.driver_override. This is a const string, hence

>>> the crash.

>>> Fix by using dynamic memory for pdev->driver_override.

>>>

>>> Signed-off-by: John Garry <john.garry@huawei.com>

>>

>> Oops, sorry about that.  Yes, queued for the next release and 4.14.x.

>>

>> -corey

>>

>>>

>>> diff --git a/drivers/char/ipmi/ipmi_dmi.c 

>>> b/drivers/char/ipmi/ipmi_dmi.c

>>> index ab78b3b..c5112b1 100644

>>> --- a/drivers/char/ipmi/ipmi_dmi.c

>>> +++ b/drivers/char/ipmi/ipmi_dmi.c

>>> @@ -106,7 +106,10 @@ static void __init dmi_add_platform_ipmi(unsigned

>>> long base_addr,

>>>           pr_err("ipmi:dmi: Error allocation IPMI platform device\n");

>>>           return;

>>>       }

>>> -    pdev->driver_override = override;

>>> +    pdev->driver_override = kasprintf(GFP_KERNEL, "%s",

>>> +                      override);

>

> It would be simpler to use managed variant (to avoid the kfree()), but 

> I don't think it's possible until the device probes.

>


Actually, I don't think you should do the kfree() in ipmi_si_intf.c.  
It's done in
platform_device_release(), and will cause a double free if done in the 
IPMI code,
unless you set it to NULL.  I'm going to add a patch to remove that hunk.

-corey

> John

>

>>> +    if (!pdev->driver_override)

>>> +        goto err;

>>>         if (type == IPMI_DMI_TYPE_SSIF) {

>>>           set_prop_entry(p[pidx++], "i2c-addr", u16, base_addr);

>>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c

>>> b/drivers/char/ipmi/ipmi_si_intf.c

>>> index 779869e..2a19aa8 100644

>>> --- a/drivers/char/ipmi/ipmi_si_intf.c

>>> +++ b/drivers/char/ipmi/ipmi_si_intf.c

>>> @@ -2426,6 +2426,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>>>   {

>>>       struct smi_info *e;

>>>       int rv = -ENOENT;

>>> +    struct platform_device *pdev = to_platform_device(dev);

>>>         mutex_lock(&smi_infos_lock);

>>>       list_for_each_entry(e, &smi_infos, link) {

>>> @@ -2436,6 +2437,7 @@ int ipmi_si_remove_by_dev(struct device *dev)

>>>           }

>>>       }

>>>       mutex_unlock(&smi_infos_lock);

>>> +    kfree(pdev->driver_override);

>>>         return rv;

>>>   }

>>

>>

>>

>> .

>>

>

>
diff mbox series

Patch

diff --git a/drivers/char/ipmi/ipmi_dmi.c b/drivers/char/ipmi/ipmi_dmi.c
index ab78b3b..c5112b1 100644
--- a/drivers/char/ipmi/ipmi_dmi.c
+++ b/drivers/char/ipmi/ipmi_dmi.c
@@ -106,7 +106,10 @@  static void __init dmi_add_platform_ipmi(unsigned long base_addr,
 		pr_err("ipmi:dmi: Error allocation IPMI platform device\n");
 		return;
 	}
-	pdev->driver_override = override;
+	pdev->driver_override = kasprintf(GFP_KERNEL, "%s",
+					  override);
+	if (!pdev->driver_override)
+		goto err;
 
 	if (type == IPMI_DMI_TYPE_SSIF) {
 		set_prop_entry(p[pidx++], "i2c-addr", u16, base_addr);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 779869e..2a19aa8 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2426,6 +2426,7 @@  int ipmi_si_remove_by_dev(struct device *dev)
 {
 	struct smi_info *e;
 	int rv = -ENOENT;
+	struct platform_device *pdev = to_platform_device(dev);
 
 	mutex_lock(&smi_infos_lock);
 	list_for_each_entry(e, &smi_infos, link) {
@@ -2436,6 +2437,7 @@  int ipmi_si_remove_by_dev(struct device *dev)
 		}
 	}
 	mutex_unlock(&smi_infos_lock);
+	kfree(pdev->driver_override);
 
 	return rv;
 }