In ssif_probe error path the i2c client is left hanging, so that
ssif_platform_remove will remove the client. But it is quite
possible that ssif would never call an i2c_new_device.
This condition would lead to kernel crash as below.
To fix this leave only the client ssif registered hanging in error
path. All other non-registered clients are set to NULL.
CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference firmware version 7.0 08/04/2018
pstate: 60400009 (nZCv daif +PAN -UAO)
pc : kernfs_find_ns+0x28/0x120
lr : kernfs_find_and_get_ns+0x40/0x60
sp : ffff00002310fb50
x29: ffff00002310fb50 x28: ffff800a8240f800
x27: 0000000000000000 x26: 0000000000000000
x25: 0000000056000000 x24: ffff000009073000
x23: ffff000008998b38 x22: 0000000000000000
x21: ffff800ed86de820 x20: 0000000000000000
x19: ffff00000913a1d8 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000000 x14: 5300737265766972
x13: 643d4d4554535953 x12: 0000000000000030
x11: 0000000000000030 x10: 0101010101010101
x9 : ffff800ea06cc3f9 x8 : 0000000000000000
x7 : 0000000000000141 x6 : ffff000009073000
x5 : ffff800adb706b00 x4 : 0000000000000000
x3 : 00000000ffffffff x2 : 0000000000000000
x1 : ffff000008998b38 x0 : ffff000008356760
Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
Call trace:
kernfs_find_ns+0x28/0x120
kernfs_find_and_get_ns+0x40/0x60
sysfs_unmerge_group+0x2c/0x6c
dpm_sysfs_remove+0x34/0x70
device_del+0x58/0x30c
device_unregister+0x30/0x7c
i2c_unregister_device+0x84/0x90 [i2c_core]
ssif_platform_remove+0x38/0x98 [ipmi_ssif]
platform_drv_remove+0x2c/0x6c
device_release_driver_internal+0x168/0x1f8
driver_detach+0x50/0xbc
bus_remove_driver+0x74/0xe8
driver_unregister+0x34/0x5c
platform_driver_unregister+0x20/0x2c
cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
__arm64_sys_delete_module+0x1b4/0x220
el0_svc_handler+0x104/0x160
el0_svc+0x8/0xc
Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
---[ end trace 09f0e34cce8e2d8c ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x23800c38
Signed-off-by: George Cherian <[email protected]>
---
drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 18e4650..ccdf6b1 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -181,6 +181,7 @@ struct ssif_addr_info {
struct device *dev;
struct i2c_client *client;
+ bool client_registered;
struct mutex clients_mutex;
struct list_head clients;
@@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
* the client like it should.
*/
dev_err(&client->dev, "Unable to start IPMI SSIF: %d\n", rv);
+ if (!addr_info->client_registered)
+ addr_info->client = NULL;
kfree(ssif_info);
}
kfree(resp);
@@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
static int ssif_adapter_handler(struct device *adev, void *opaque)
{
struct ssif_addr_info *addr_info = opaque;
+ struct i2c_client *client;
if (adev->type != &i2c_adapter_type)
return 0;
- i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
+ client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
+ if (client)
+ addr_info->client_registered = true;
if (!addr_info->adapter_name)
return 1; /* Only try the first I2C adapter by default. */
--
1.8.3.1
Dont set ssif_info->intf to NULL before ipmi_unresgiter_smi.
shutdown_ssif will anyways free ssif_info.
Following crash is obsearved if ssif_info->intf is set to NULL
before ipmi_unregister_smi.
CPU: 119 PID: 7317 Comm: kssif000e Not tainted 4.18.0+ #80
Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference firmware version 7.0 08/04/2018
pstate: 20400009 (nzCv daif +PAN -UAO)
pc : ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
lr : deliver_recv_msg+0x30/0x5c [ipmi_ssif]
sp : ffff000037a0fd20
x29: ffff000037a0fd20 x28: 0000000000000000
x27: ffff0000047e08f0 x26: ffff800ed9375800
x25: ffff000037a0fe00 x24: ffff000009073000
x23: 0000000000000013 x22: 0000000000000000
x21: 0000000000007000 x20: ffff800adce18400
x19: 0000000000000000 x18: ffff00003742fd38
x17: ffff0000089960f0 x16: 000000000000000e
x15: 0000000000000007 x14: 0000000000000000
x13: 0000000000000000 x12: 0000000000000033
x11: 0000000000000381 x10: 0000000000000ba0
x9 : 0000000000000000 x8 : ffff800ac001fc00
x7 : ffff7fe003b4d800 x6 : ffff800adce1854b
x5 : 0000000000000014 x4 : 0000000000000004
x3 : 0000000000000000 x2 : 0000000000000002
x1 : 567cb12f8b916b00 x0 : 0000000000000002
Process kssif000e (pid: 7317, stack limit = 0x0000000041077d8a)
Call trace:
ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
deliver_recv_msg+0x30/0x5c [ipmi_ssif]
msg_done_handler+0x2f0/0x66c [ipmi_ssif]
ipmi_ssif_thread+0x108/0x124 [ipmi_ssif]
kthread+0x108/0x134
ret_from_fork+0x10/0x18
Code: b9402280 91401e75 f90037a1 7100041f (b945bab6)
---[ end trace fb7d748bc7b17490 ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x23800c38
Memory Limit: none
---[ end Kernel panic - not syncing: Fatal exception ]---
Signed-off-by: George Cherian <[email protected]>
---
drivers/char/ipmi/ipmi_ssif.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index ccdf6b1..1490636 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1226,7 +1226,6 @@ static void shutdown_ssif(void *send_info)
static int ssif_remove(struct i2c_client *client)
{
struct ssif_info *ssif_info = i2c_get_clientdata(client);
- struct ipmi_smi *intf;
struct ssif_addr_info *addr_info;
if (!ssif_info)
@@ -1236,9 +1235,7 @@ static int ssif_remove(struct i2c_client *client)
* After this point, we won't deliver anything asychronously
* to the message handler. We can unregister ourself.
*/
- intf = ssif_info->intf;
- ssif_info->intf = NULL;
- ipmi_unregister_smi(intf);
+ ipmi_unregister_smi(ssif_info->intf);
list_for_each_entry(addr_info, &ssif_infos, link) {
if (addr_info->client == client) {
--
1.8.3.1
On 08/24/2018 06:10 AM, George Cherian wrote:
> In ssif_probe error path the i2c client is left hanging, so that
> ssif_platform_remove will remove the client. But it is quite
> possible that ssif would never call an i2c_new_device.
> This condition would lead to kernel crash as below.
> To fix this leave only the client ssif registered hanging in error
> path. All other non-registered clients are set to NULL.
I'm having a hard time seeing how this could happen.
The i2c_new_device() call is only done in the case of dmi_ipmi_probe
(called from
ssif_platform_probe) or a hard-coded entry. How does
ssif_platform_remove get
called on a device that was not registered with ssif_platform_probe?
Small style comment inline...
> CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference firmware version 7.0 08/04/2018
> pstate: 60400009 (nZCv daif +PAN -UAO)
> pc : kernfs_find_ns+0x28/0x120
> lr : kernfs_find_and_get_ns+0x40/0x60
> sp : ffff00002310fb50
> x29: ffff00002310fb50 x28: ffff800a8240f800
> x27: 0000000000000000 x26: 0000000000000000
> x25: 0000000056000000 x24: ffff000009073000
> x23: ffff000008998b38 x22: 0000000000000000
> x21: ffff800ed86de820 x20: 0000000000000000
> x19: ffff00000913a1d8 x18: 0000000000000000
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000000000 x14: 5300737265766972
> x13: 643d4d4554535953 x12: 0000000000000030
> x11: 0000000000000030 x10: 0101010101010101
> x9 : ffff800ea06cc3f9 x8 : 0000000000000000
> x7 : 0000000000000141 x6 : ffff000009073000
> x5 : ffff800adb706b00 x4 : 0000000000000000
> x3 : 00000000ffffffff x2 : 0000000000000000
> x1 : ffff000008998b38 x0 : ffff000008356760
> Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
> Call trace:
> kernfs_find_ns+0x28/0x120
> kernfs_find_and_get_ns+0x40/0x60
> sysfs_unmerge_group+0x2c/0x6c
> dpm_sysfs_remove+0x34/0x70
> device_del+0x58/0x30c
> device_unregister+0x30/0x7c
> i2c_unregister_device+0x84/0x90 [i2c_core]
> ssif_platform_remove+0x38/0x98 [ipmi_ssif]
> platform_drv_remove+0x2c/0x6c
> device_release_driver_internal+0x168/0x1f8
> driver_detach+0x50/0xbc
> bus_remove_driver+0x74/0xe8
> driver_unregister+0x34/0x5c
> platform_driver_unregister+0x20/0x2c
> cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
> __arm64_sys_delete_module+0x1b4/0x220
> el0_svc_handler+0x104/0x160
> el0_svc+0x8/0xc
> Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
> ---[ end trace 09f0e34cce8e2d8c ]---
> Kernel panic - not syncing: Fatal exception
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x23800c38
>
> Signed-off-by: George Cherian <[email protected]>
> ---
> drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 18e4650..ccdf6b1 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -181,6 +181,7 @@ struct ssif_addr_info {
> struct device *dev;
> struct i2c_client *client;
>
> + bool client_registered;
> struct mutex clients_mutex;
> struct list_head clients;
>
> @@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
> * the client like it should.
> */
> dev_err(&client->dev, "Unable to start IPMI SSIF: %d\n", rv);
> + if (!addr_info->client_registered)
> + addr_info->client = NULL;
> kfree(ssif_info);
> }
> kfree(resp);
> @@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
> static int ssif_adapter_handler(struct device *adev, void *opaque)
> {
> struct ssif_addr_info *addr_info = opaque;
> + struct i2c_client *client;
>
> if (adev->type != &i2c_adapter_type)
> return 0;
>
> - i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
> + client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
> + if (client)
> + addr_info->client_registered = true;
>
How about..
if (i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo))
addr_info->client_registered = true;
No need for the client variable.
-corey
> if (!addr_info->adapter_name)
> return 1; /* Only try the first I2C adapter by default. */
On 08/24/2018 06:10 AM, George Cherian wrote:
> Dont set ssif_info->intf to NULL before ipmi_unresgiter_smi.
> shutdown_ssif will anyways free ssif_info.
This is correct, but it goes a little deeper. I just sent out a
patch yesterday that included this.
Thanks,
-corey
> Following crash is obsearved if ssif_info->intf is set to NULL
> before ipmi_unregister_smi.
>
> CPU: 119 PID: 7317 Comm: kssif000e Not tainted 4.18.0+ #80
> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference firmware version 7.0 08/04/2018
> pstate: 20400009 (nzCv daif +PAN -UAO)
> pc : ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
> lr : deliver_recv_msg+0x30/0x5c [ipmi_ssif]
> sp : ffff000037a0fd20
> x29: ffff000037a0fd20 x28: 0000000000000000
> x27: ffff0000047e08f0 x26: ffff800ed9375800
> x25: ffff000037a0fe00 x24: ffff000009073000
> x23: 0000000000000013 x22: 0000000000000000
> x21: 0000000000007000 x20: ffff800adce18400
> x19: 0000000000000000 x18: ffff00003742fd38
> x17: ffff0000089960f0 x16: 000000000000000e
> x15: 0000000000000007 x14: 0000000000000000
> x13: 0000000000000000 x12: 0000000000000033
> x11: 0000000000000381 x10: 0000000000000ba0
> x9 : 0000000000000000 x8 : ffff800ac001fc00
> x7 : ffff7fe003b4d800 x6 : ffff800adce1854b
> x5 : 0000000000000014 x4 : 0000000000000004
> x3 : 0000000000000000 x2 : 0000000000000002
> x1 : 567cb12f8b916b00 x0 : 0000000000000002
> Process kssif000e (pid: 7317, stack limit = 0x0000000041077d8a)
> Call trace:
> ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
> deliver_recv_msg+0x30/0x5c [ipmi_ssif]
> msg_done_handler+0x2f0/0x66c [ipmi_ssif]
> ipmi_ssif_thread+0x108/0x124 [ipmi_ssif]
> kthread+0x108/0x134
> ret_from_fork+0x10/0x18
> Code: b9402280 91401e75 f90037a1 7100041f (b945bab6)
> ---[ end trace fb7d748bc7b17490 ]---
> Kernel panic - not syncing: Fatal exception
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x23800c38
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Fatal exception ]---
>
> Signed-off-by: George Cherian <[email protected]>
> ---
> drivers/char/ipmi/ipmi_ssif.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index ccdf6b1..1490636 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1226,7 +1226,6 @@ static void shutdown_ssif(void *send_info)
> static int ssif_remove(struct i2c_client *client)
> {
> struct ssif_info *ssif_info = i2c_get_clientdata(client);
> - struct ipmi_smi *intf;
> struct ssif_addr_info *addr_info;
>
> if (!ssif_info)
> @@ -1236,9 +1235,7 @@ static int ssif_remove(struct i2c_client *client)
> * After this point, we won't deliver anything asychronously
> * to the message handler. We can unregister ourself.
> */
> - intf = ssif_info->intf;
> - ssif_info->intf = NULL;
> - ipmi_unregister_smi(intf);
> + ipmi_unregister_smi(ssif_info->intf);
>
> list_for_each_entry(addr_info, &ssif_infos, link) {
> if (addr_info->client == client) {
Hi Corey,
On 08/24/2018 06:38 PM, Corey Minyard wrote:
>
> On 08/24/2018 06:10 AM, George Cherian wrote:
>> Dont set ssif_info->intf to NULL before ipmi_unresgiter_smi.
>> shutdown_ssif will anyways free ssif_info.
>
> This is correct, but it goes a little deeper. I just sent out a
> patch yesterday that included this.
Yes I saw the patch now,
https://sourceforge.net/p/openipmi/mailman/message/36397896/
I will test and update in that thread.
>
> Thanks,
>
> -corey
>
>> Following crash is obsearved if ssif_info->intf is set to NULL
>> before ipmi_unregister_smi.
>>
>> CPU: 119 PID: 7317 Comm: kssif000e Not tainted 4.18.0+ #80
>> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference
>> firmware version 7.0 08/04/2018
>> pstate: 20400009 (nzCv daif +PAN -UAO)
>> pc : ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
>> lr : deliver_recv_msg+0x30/0x5c [ipmi_ssif]
>> sp : ffff000037a0fd20
>> x29: ffff000037a0fd20 x28: 0000000000000000
>> x27: ffff0000047e08f0 x26: ffff800ed9375800
>> x25: ffff000037a0fe00 x24: ffff000009073000
>> x23: 0000000000000013 x22: 0000000000000000
>> x21: 0000000000007000 x20: ffff800adce18400
>> x19: 0000000000000000 x18: ffff00003742fd38
>> x17: ffff0000089960f0 x16: 000000000000000e
>> x15: 0000000000000007 x14: 0000000000000000
>> x13: 0000000000000000 x12: 0000000000000033
>> x11: 0000000000000381 x10: 0000000000000ba0
>> x9 : 0000000000000000 x8 : ffff800ac001fc00
>> x7 : ffff7fe003b4d800 x6 : ffff800adce1854b
>> x5 : 0000000000000014 x4 : 0000000000000004
>> x3 : 0000000000000000 x2 : 0000000000000002
>> x1 : 567cb12f8b916b00 x0 : 0000000000000002
>> Process kssif000e (pid: 7317, stack limit = 0x0000000041077d8a)
>> Call trace:
>> ipmi_smi_msg_received+0x44/0x3bc [ipmi_msghandler]
>> deliver_recv_msg+0x30/0x5c [ipmi_ssif]
>> msg_done_handler+0x2f0/0x66c [ipmi_ssif]
>> ipmi_ssif_thread+0x108/0x124 [ipmi_ssif]
>> kthread+0x108/0x134
>> ret_from_fork+0x10/0x18
>> Code: b9402280 91401e75 f90037a1 7100041f (b945bab6)
>> ---[ end trace fb7d748bc7b17490 ]---
>> Kernel panic - not syncing: Fatal exception
>> SMP: stopping secondary CPUs
>> Kernel Offset: disabled
>> CPU features: 0x23800c38
>> Memory Limit: none
>> ---[ end Kernel panic - not syncing: Fatal exception ]---
>>
>> Signed-off-by: George Cherian <[email protected]>
>> ---
>> drivers/char/ipmi/ipmi_ssif.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_ssif.c
>> b/drivers/char/ipmi/ipmi_ssif.c
>> index ccdf6b1..1490636 100644
>> --- a/drivers/char/ipmi/ipmi_ssif.c
>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>> @@ -1226,7 +1226,6 @@ static void shutdown_ssif(void *send_info)
>> static int ssif_remove(struct i2c_client *client)
>> {
>> struct ssif_info *ssif_info = i2c_get_clientdata(client);
>> - struct ipmi_smi *intf;
>> struct ssif_addr_info *addr_info;
>>
>> if (!ssif_info)
>> @@ -1236,9 +1235,7 @@ static int ssif_remove(struct i2c_client *client)
>> * After this point, we won't deliver anything asychronously
>> * to the message handler. We can unregister ourself.
>> */
>> - intf = ssif_info->intf;
>> - ssif_info->intf = NULL;
>> - ipmi_unregister_smi(intf);
>> + ipmi_unregister_smi(ssif_info->intf);
>>
>> list_for_each_entry(addr_info, &ssif_infos, link) {
>> if (addr_info->client == client) {
>
>
Hi Corey,
On 08/24/2018 06:37 PM, Corey Minyard wrote:
>
> On 08/24/2018 06:10 AM, George Cherian wrote:
>> In ssif_probe error path the i2c client is left hanging, so that
>> ssif_platform_remove will remove the client. But it is quite
>> possible that ssif would never call an i2c_new_device.
>> This condition would lead to kernel crash as below.
>> To fix this leave only the client ssif registered hanging in error
>> path. All other non-registered clients are set to NULL.
>
> I'm having a hard time seeing how this could happen.
>
> The i2c_new_device() call is only done in the case of dmi_ipmi_probe
> (called from
> ssif_platform_probe) or a hard-coded entry. How does
> ssif_platform_remove get
> called on a device that was not registered with ssif_platform_probe?
>
Initially I also had the same doubt but then,
ssif_adapter_hanlder is called for each i2c_dev only after initialized
is true. So we end up not calling i2c_new_device for devices probed
during the module_init time.
ssif_platform_remove() get called during removal of ipmi_ssif.
In case during ssif_probe() if there is a failure before
ipmi_smi_register then we leave the addr_info->client hanging.
In case of normal functioning without error, we set addr_info->client
to NULL after ipmi_unregiter_smi in ssif_remove.
> Small style comment inline...
I will make the changess and sent out a v2!!
Thanks,
-George
>
>> CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
>> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference
>> firmware version 7.0 08/04/2018
>> pstate: 60400009 (nZCv daif +PAN -UAO)
>> pc : kernfs_find_ns+0x28/0x120
>> lr : kernfs_find_and_get_ns+0x40/0x60
>> sp : ffff00002310fb50
>> x29: ffff00002310fb50 x28: ffff800a8240f800
>> x27: 0000000000000000 x26: 0000000000000000
>> x25: 0000000056000000 x24: ffff000009073000
>> x23: ffff000008998b38 x22: 0000000000000000
>> x21: ffff800ed86de820 x20: 0000000000000000
>> x19: ffff00000913a1d8 x18: 0000000000000000
>> x17: 0000000000000000 x16: 0000000000000000
>> x15: 0000000000000000 x14: 5300737265766972
>> x13: 643d4d4554535953 x12: 0000000000000030
>> x11: 0000000000000030 x10: 0101010101010101
>> x9 : ffff800ea06cc3f9 x8 : 0000000000000000
>> x7 : 0000000000000141 x6 : ffff000009073000
>> x5 : ffff800adb706b00 x4 : 0000000000000000
>> x3 : 00000000ffffffff x2 : 0000000000000000
>> x1 : ffff000008998b38 x0 : ffff000008356760
>> Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
>> Call trace:
>> kernfs_find_ns+0x28/0x120
>> kernfs_find_and_get_ns+0x40/0x60
>> sysfs_unmerge_group+0x2c/0x6c
>> dpm_sysfs_remove+0x34/0x70
>> device_del+0x58/0x30c
>> device_unregister+0x30/0x7c
>> i2c_unregister_device+0x84/0x90 [i2c_core]
>> ssif_platform_remove+0x38/0x98 [ipmi_ssif]
>> platform_drv_remove+0x2c/0x6c
>> device_release_driver_internal+0x168/0x1f8
>> driver_detach+0x50/0xbc
>> bus_remove_driver+0x74/0xe8
>> driver_unregister+0x34/0x5c
>> platform_driver_unregister+0x20/0x2c
>> cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
>> __arm64_sys_delete_module+0x1b4/0x220
>> el0_svc_handler+0x104/0x160
>> el0_svc+0x8/0xc
>> Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
>> ---[ end trace 09f0e34cce8e2d8c ]---
>> Kernel panic - not syncing: Fatal exception
>> SMP: stopping secondary CPUs
>> Kernel Offset: disabled
>> CPU features: 0x23800c38
>>
>> Signed-off-by: George Cherian <[email protected]>
>> ---
>> drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_ssif.c
>> b/drivers/char/ipmi/ipmi_ssif.c
>> index 18e4650..ccdf6b1 100644
>> --- a/drivers/char/ipmi/ipmi_ssif.c
>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>> @@ -181,6 +181,7 @@ struct ssif_addr_info {
>> struct device *dev;
>> struct i2c_client *client;
>>
>> + bool client_registered;
>> struct mutex clients_mutex;
>> struct list_head clients;
>>
>> @@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> * the client like it should.
>> */
>> dev_err(&client->dev, "Unable to start IPMI SSIF: %d\n",
>> rv);
>> + if (!addr_info->client_registered)
>> + addr_info->client = NULL;
>> kfree(ssif_info);
>> }
>> kfree(resp);
>> @@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client
>> *client, const struct i2c_device_id *id)
>> static int ssif_adapter_handler(struct device *adev, void *opaque)
>> {
>> struct ssif_addr_info *addr_info = opaque;
>> + struct i2c_client *client;
>>
>> if (adev->type != &i2c_adapter_type)
>> return 0;
>>
>> - i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>> + client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>> + if (client)
>> + addr_info->client_registered = true;
>>
>
> How about..
> if (i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo))
> addr_info->client_registered = true;
>
> No need for the client variable.
>
> -corey
>
>> if (!addr_info->adapter_name)
>> return 1; /* Only try the first I2C adapter by default. */
>
>
On 08/27/2018 01:07 AM, George Cherian wrote:
>
> Hi Corey,
>
> On 08/24/2018 06:37 PM, Corey Minyard wrote:
>>
>> On 08/24/2018 06:10 AM, George Cherian wrote:
>>> In ssif_probe error path the i2c client is left hanging, so that
>>> ssif_platform_remove will remove the client. But it is quite
>>> possible that ssif would never call an i2c_new_device.
>>> This condition would lead to kernel crash as below.
>>> To fix this leave only the client ssif registered hanging in error
>>> path. All other non-registered clients are set to NULL.
>>
>> I'm having a hard time seeing how this could happen.
>>
>> The i2c_new_device() call is only done in the case of dmi_ipmi_probe
>> (called from
>> ssif_platform_probe) or a hard-coded entry. How does
>> ssif_platform_remove get
>> called on a device that was not registered with ssif_platform_probe?
>>
>
> Initially I also had the same doubt but then,
> ssif_adapter_hanlder is called for each i2c_dev only after initialized
> is true. So we end up not calling i2c_new_device for devices probed
> during the module_init time.
>
I spent some time looking at this, and I don't think that's what is
happening.
I think that i2c_del_driver() in cleanup_ipmi_ssif() is causing
i2c_unregister_device() to be called on all the devices, and
platform_driver_unregister() causes it to be called on the
devices that came in through the platform method. It's
a double-free.
Try reversing the order of i2c_del_driver() and platform_driver_unregister()
in cleanup_ipmi_ssif() to test this.
-corey
> ssif_platform_remove() get called during removal of ipmi_ssif.
> In case during ssif_probe() if there is a failure before
> ipmi_smi_register then we leave the addr_info->client hanging.
>
> In case of normal functioning without error, we set addr_info->client
> to NULL after ipmi_unregiter_smi in ssif_remove.
>
>> Small style comment inline...
> I will make the changess and sent out a v2!!
>
> Thanks,
> -George
>>
>>> CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
>>> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference
>>> firmware version 7.0 08/04/2018
>>> pstate: 60400009 (nZCv daif +PAN -UAO)
>>> pc : kernfs_find_ns+0x28/0x120
>>> lr : kernfs_find_and_get_ns+0x40/0x60
>>> sp : ffff00002310fb50
>>> x29: ffff00002310fb50 x28: ffff800a8240f800
>>> x27: 0000000000000000 x26: 0000000000000000
>>> x25: 0000000056000000 x24: ffff000009073000
>>> x23: ffff000008998b38 x22: 0000000000000000
>>> x21: ffff800ed86de820 x20: 0000000000000000
>>> x19: ffff00000913a1d8 x18: 0000000000000000
>>> x17: 0000000000000000 x16: 0000000000000000
>>> x15: 0000000000000000 x14: 5300737265766972
>>> x13: 643d4d4554535953 x12: 0000000000000030
>>> x11: 0000000000000030 x10: 0101010101010101
>>> x9 : ffff800ea06cc3f9 x8 : 0000000000000000
>>> x7 : 0000000000000141 x6 : ffff000009073000
>>> x5 : ffff800adb706b00 x4 : 0000000000000000
>>> x3 : 00000000ffffffff x2 : 0000000000000000
>>> x1 : ffff000008998b38 x0 : ffff000008356760
>>> Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
>>> Call trace:
>>> kernfs_find_ns+0x28/0x120
>>> kernfs_find_and_get_ns+0x40/0x60
>>> sysfs_unmerge_group+0x2c/0x6c
>>> dpm_sysfs_remove+0x34/0x70
>>> device_del+0x58/0x30c
>>> device_unregister+0x30/0x7c
>>> i2c_unregister_device+0x84/0x90 [i2c_core]
>>> ssif_platform_remove+0x38/0x98 [ipmi_ssif]
>>> platform_drv_remove+0x2c/0x6c
>>> device_release_driver_internal+0x168/0x1f8
>>> driver_detach+0x50/0xbc
>>> bus_remove_driver+0x74/0xe8
>>> driver_unregister+0x34/0x5c
>>> platform_driver_unregister+0x20/0x2c
>>> cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
>>> __arm64_sys_delete_module+0x1b4/0x220
>>> el0_svc_handler+0x104/0x160
>>> el0_svc+0x8/0xc
>>> Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
>>> ---[ end trace 09f0e34cce8e2d8c ]---
>>> Kernel panic - not syncing: Fatal exception
>>> SMP: stopping secondary CPUs
>>> Kernel Offset: disabled
>>> CPU features: 0x23800c38
>>>
>>> Signed-off-by: George Cherian <[email protected]>
>>> ---
>>> drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c
>>> b/drivers/char/ipmi/ipmi_ssif.c
>>> index 18e4650..ccdf6b1 100644
>>> --- a/drivers/char/ipmi/ipmi_ssif.c
>>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>>> @@ -181,6 +181,7 @@ struct ssif_addr_info {
>>> struct device *dev;
>>> struct i2c_client *client;
>>>
>>> + bool client_registered;
>>> struct mutex clients_mutex;
>>> struct list_head clients;
>>>
>>> @@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client
>>> *client, const struct i2c_device_id *id)
>>> * the client like it should.
>>> */
>>> dev_err(&client->dev, "Unable to start IPMI SSIF:
>>> %d\n", rv);
>>> + if (!addr_info->client_registered)
>>> + addr_info->client = NULL;
>>> kfree(ssif_info);
>>> }
>>> kfree(resp);
>>> @@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client
>>> *client, const struct i2c_device_id *id)
>>> static int ssif_adapter_handler(struct device *adev, void *opaque)
>>> {
>>> struct ssif_addr_info *addr_info = opaque;
>>> + struct i2c_client *client;
>>>
>>> if (adev->type != &i2c_adapter_type)
>>> return 0;
>>>
>>> - i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>>> + client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>>> + if (client)
>>> + addr_info->client_registered = true;
>>>
>>
>> How about..
>> if (i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo))
>> addr_info->client_registered = true;
>>
>> No need for the client variable.
>>
>> -corey
>>
>>> if (!addr_info->adapter_name)
>>> return 1; /* Only try the first I2C adapter by
>>> default. */
>>
>>
Hi Corey,
On 08/28/2018 04:59 AM, Corey Minyard wrote:
>
> On 08/27/2018 01:07 AM, George Cherian wrote:
>>
>> Hi Corey,
>>
>> On 08/24/2018 06:37 PM, Corey Minyard wrote:
>>>
>>> On 08/24/2018 06:10 AM, George Cherian wrote:
>>>> In ssif_probe error path the i2c client is left hanging, so that
>>>> ssif_platform_remove will remove the client. But it is quite
>>>> possible that ssif would never call an i2c_new_device.
>>>> This condition would lead to kernel crash as below.
>>>> To fix this leave only the client ssif registered hanging in error
>>>> path. All other non-registered clients are set to NULL.
>>>
>>> I'm having a hard time seeing how this could happen.
>>>
>>> The i2c_new_device() call is only done in the case of dmi_ipmi_probe
>>> (called from
>>> ssif_platform_probe) or a hard-coded entry. How does
>>> ssif_platform_remove get
>>> called on a device that was not registered with ssif_platform_probe?
>>>
>>
>> Initially I also had the same doubt but then,
>> ssif_adapter_hanlder is called for each i2c_dev only after initialized
>> is true. So we end up not calling i2c_new_device for devices probed
>> during the module_init time.
>>
>
> I spent some time looking at this, and I don't think that's what is
> happening.
>
> I think that i2c_del_driver() in cleanup_ipmi_ssif() is causing
> i2c_unregister_device() to be called on all the devices, and
> platform_driver_unregister() causes it to be called on the
> devices that came in through the platform method. It's
> a double-free.
>
> Try reversing the order of i2c_del_driver() and
> platform_driver_unregister()
> in cleanup_ipmi_ssif() to test this.
>
Reversing the call order didn't help, I am still getting the trace.
You are partly correct on the double free scenario. I dont see double
free in normal operation. I see a double free only in probe failure case.
I have added prints in i2c_unregister_device() to print the client.
pr_err("client = %px\n", client);
In normal case, I get 2 calls to i2c_unregister_device()
Call 1: i2c-core: client = ffff800ada315400 => called from
i2c_del_driver()
This in turn calls ssif_remove, where we set addr_info->client to NULL.
Call 2: i2c-core: client = 0000000000000000 => called from
ssif_platform_remove()
This is fine since i2c_unregister_device is NULL aware.
This works fine without crashing .
Now in the probe failing case, I get 2 calls to i2c_unregister_device()
Call 1: i2c-core: client = ffff800ad9897400 => called from
i2c_del_driver()
This never calls ssif_remove, since the probe failed.
Call 2: i2c-core: client = ffff800ad9897400 => called from
ssif_platform_remove()
This is a case of double free.
Do you think the proposed patch itself is the solution or
Is it that we should really leave addr_info->client hanging in probe
error path at all?
NB: For easy simulation of the ssif_probe failing case I just replaced
the
ssif_info->thread = kthread_run(....) with
ssif_info->thread = ERR_PTR(-4); so that the probe takes the goto out path.
-George
> -corey
>
>> ssif_platform_remove() get called during removal of ipmi_ssif.
>> In case during ssif_probe() if there is a failure before
>> ipmi_smi_register then we leave the addr_info->client hanging.
>>
>> In case of normal functioning without error, we set addr_info->client
>> to NULL after ipmi_unregiter_smi in ssif_remove.
>>
>>> Small style comment inline...
>> I will make the changess and sent out a v2!!
>>
>> Thanks,
>> -George
>>>
>>>> CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
>>>> Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference
>>>> firmware version 7.0 08/04/2018
>>>> pstate: 60400009 (nZCv daif +PAN -UAO)
>>>> pc : kernfs_find_ns+0x28/0x120
>>>> lr : kernfs_find_and_get_ns+0x40/0x60
>>>> sp : ffff00002310fb50
>>>> x29: ffff00002310fb50 x28: ffff800a8240f800
>>>> x27: 0000000000000000 x26: 0000000000000000
>>>> x25: 0000000056000000 x24: ffff000009073000
>>>> x23: ffff000008998b38 x22: 0000000000000000
>>>> x21: ffff800ed86de820 x20: 0000000000000000
>>>> x19: ffff00000913a1d8 x18: 0000000000000000
>>>> x17: 0000000000000000 x16: 0000000000000000
>>>> x15: 0000000000000000 x14: 5300737265766972
>>>> x13: 643d4d4554535953 x12: 0000000000000030
>>>> x11: 0000000000000030 x10: 0101010101010101
>>>> x9 : ffff800ea06cc3f9 x8 : 0000000000000000
>>>> x7 : 0000000000000141 x6 : ffff000009073000
>>>> x5 : ffff800adb706b00 x4 : 0000000000000000
>>>> x3 : 00000000ffffffff x2 : 0000000000000000
>>>> x1 : ffff000008998b38 x0 : ffff000008356760
>>>> Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
>>>> Call trace:
>>>> kernfs_find_ns+0x28/0x120
>>>> kernfs_find_and_get_ns+0x40/0x60
>>>> sysfs_unmerge_group+0x2c/0x6c
>>>> dpm_sysfs_remove+0x34/0x70
>>>> device_del+0x58/0x30c
>>>> device_unregister+0x30/0x7c
>>>> i2c_unregister_device+0x84/0x90 [i2c_core]
>>>> ssif_platform_remove+0x38/0x98 [ipmi_ssif]
>>>> platform_drv_remove+0x2c/0x6c
>>>> device_release_driver_internal+0x168/0x1f8
>>>> driver_detach+0x50/0xbc
>>>> bus_remove_driver+0x74/0xe8
>>>> driver_unregister+0x34/0x5c
>>>> platform_driver_unregister+0x20/0x2c
>>>> cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
>>>> __arm64_sys_delete_module+0x1b4/0x220
>>>> el0_svc_handler+0x104/0x160
>>>> el0_svc+0x8/0xc
>>>> Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
>>>> ---[ end trace 09f0e34cce8e2d8c ]---
>>>> Kernel panic - not syncing: Fatal exception
>>>> SMP: stopping secondary CPUs
>>>> Kernel Offset: disabled
>>>> CPU features: 0x23800c38
>>>>
>>>> Signed-off-by: George Cherian <[email protected]>
>>>> ---
>>>> drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c
>>>> b/drivers/char/ipmi/ipmi_ssif.c
>>>> index 18e4650..ccdf6b1 100644
>>>> --- a/drivers/char/ipmi/ipmi_ssif.c
>>>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>>>> @@ -181,6 +181,7 @@ struct ssif_addr_info {
>>>> struct device *dev;
>>>> struct i2c_client *client;
>>>>
>>>> + bool client_registered;
>>>> struct mutex clients_mutex;
>>>> struct list_head clients;
>>>>
>>>> @@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client
>>>> *client, const struct i2c_device_id *id)
>>>> * the client like it should.
>>>> */
>>>> dev_err(&client->dev, "Unable to start IPMI SSIF:
>>>> %d\n", rv);
>>>> + if (!addr_info->client_registered)
>>>> + addr_info->client = NULL;
>>>> kfree(ssif_info);
>>>> }
>>>> kfree(resp);
>>>> @@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client
>>>> *client, const struct i2c_device_id *id)
>>>> static int ssif_adapter_handler(struct device *adev, void *opaque)
>>>> {
>>>> struct ssif_addr_info *addr_info = opaque;
>>>> + struct i2c_client *client;
>>>>
>>>> if (adev->type != &i2c_adapter_type)
>>>> return 0;
>>>>
>>>> - i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>>>> + client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
>>>> + if (client)
>>>> + addr_info->client_registered = true;
>>>>
>>>
>>> How about..
>>> if (i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo))
>>> addr_info->client_registered = true;
>>>
>>> No need for the client variable.
>>>
>>> -corey
>>>
>>>> if (!addr_info->adapter_name)
>>>> return 1; /* Only try the first I2C adapter by
>>>> default. */
>>>
>>>
>
On 08/28/2018 09:32 AM, George Cherian wrote:
>
> Hi Corey,
>
> On 08/28/2018 04:59 AM, Corey Minyard wrote:
>>
>> On 08/27/2018 01:07 AM, George Cherian wrote:
>>>
>>> Hi Corey,
>>>
>>> On 08/24/2018 06:37 PM, Corey Minyard wrote:
>>>>
>>>> On 08/24/2018 06:10 AM, George Cherian wrote:
>>>>> In ssif_probe error path the i2c client is left hanging, so that
>>>>> ssif_platform_remove will remove the client. But it is quite
>>>>> possible that ssif would never call an i2c_new_device.
>>>>> This condition would lead to kernel crash as below.
>>>>> To fix this leave only the client ssif registered hanging in error
>>>>> path. All other non-registered clients are set to NULL.
>>>>
>>>> I'm having a hard time seeing how this could happen.
>>>>
>>>> The i2c_new_device() call is only done in the case of dmi_ipmi_probe
>>>> (called from
>>>> ssif_platform_probe) or a hard-coded entry. How does
>>>> ssif_platform_remove get
>>>> called on a device that was not registered with ssif_platform_probe?
>>>>
>>>
>>> Initially I also had the same doubt but then,
>>> ssif_adapter_hanlder is called for each i2c_dev only after initialized
>>> is true. So we end up not calling i2c_new_device for devices probed
>>> during the module_init time.
>>>
>>
>> I spent some time looking at this, and I don't think that's what is
>> happening.
>>
>> I think that i2c_del_driver() in cleanup_ipmi_ssif() is causing
>> i2c_unregister_device() to be called on all the devices, and
>> platform_driver_unregister() causes it to be called on the
>> devices that came in through the platform method. It's
>> a double-free.
>>
>> Try reversing the order of i2c_del_driver() and
>> platform_driver_unregister()
>> in cleanup_ipmi_ssif() to test this.
>>
> Reversing the call order didn't help, I am still getting the trace.
That's really strange. Calling ssif_platform_remove() should result in
i2c_del_driver() being called on the device, and thus i2c_del_driver()
should't free it. At least per you later analysis in this mail.
>
> You are partly correct on the double free scenario. I dont see double
> free in normal operation. I see a double free only in probe failure case.
>
>
> I have added prints in i2c_unregister_device() to print the client.
> pr_err("client = %px\n", client);
>
> In normal case, I get 2 calls to i2c_unregister_device()
> Call 1: i2c-core: client = ffff800ada315400 => called from
> i2c_del_driver()
> This in turn calls ssif_remove, where we set addr_info->client to NULL.
>
> Call 2: i2c-core: client = 0000000000000000 => called from
> ssif_platform_remove()
> This is fine since i2c_unregister_device is NULL aware.
> This works fine without crashing .
>
> Now in the probe failing case, I get 2 calls to i2c_unregister_device()
> Call 1: i2c-core: client = ffff800ad9897400 => called from
> i2c_del_driver()
> This never calls ssif_remove, since the probe failed.
>
> Call 2: i2c-core: client = ffff800ad9897400 => called from
> ssif_platform_remove()
> This is a case of double free.
>
> Do you think the proposed patch itself is the solution or
> Is it that we should really leave addr_info->client hanging in probe
> error path at all?
I have been wondering that.
>
> NB: For easy simulation of the ssif_probe failing case I just replaced
> the
>
> ssif_info->thread = kthread_run(....) with
>
> ssif_info->thread = ERR_PTR(-4); so that the probe takes the goto out
> path.
>
Ok, that was my next step, trying to reproduce this. I can try that and
look.
Quick question, though: Is this device coming through DMI? Or are you
registering this as a platform device someplace else?
-corey