* [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif @ 2018-08-24 11:10 George Cherian 2018-08-24 11:10 ` [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi George Cherian 2018-08-24 13:07 ` [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif Corey Minyard 0 siblings, 2 replies; 9+ messages in thread From: George Cherian @ 2018-08-24 11:10 UTC (permalink / raw) To: linux-kernel, openipmi-developer; +Cc: minyard, arnd, gregkh, George Cherian 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 <george.cherian@cavium.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi 2018-08-24 11:10 [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif George Cherian @ 2018-08-24 11:10 ` George Cherian 2018-08-24 13:08 ` Corey Minyard 2018-08-24 13:07 ` [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif Corey Minyard 1 sibling, 1 reply; 9+ messages in thread From: George Cherian @ 2018-08-24 11:10 UTC (permalink / raw) To: linux-kernel, openipmi-developer; +Cc: minyard, arnd, gregkh, George Cherian 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 <george.cherian@cavium.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi 2018-08-24 11:10 ` [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi George Cherian @ 2018-08-24 13:08 ` Corey Minyard 2018-08-27 5:55 ` George Cherian 0 siblings, 1 reply; 9+ messages in thread From: Corey Minyard @ 2018-08-24 13:08 UTC (permalink / raw) To: George Cherian, linux-kernel, openipmi-developer; +Cc: arnd, gregkh 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 <george.cherian@cavium.com> > --- > 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) { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi 2018-08-24 13:08 ` Corey Minyard @ 2018-08-27 5:55 ` George Cherian 0 siblings, 0 replies; 9+ messages in thread From: George Cherian @ 2018-08-27 5:55 UTC (permalink / raw) To: minyard, George Cherian, linux-kernel, openipmi-developer; +Cc: arnd, gregkh 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 <george.cherian@cavium.com> >> --- >> 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) { > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif 2018-08-24 11:10 [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif George Cherian 2018-08-24 11:10 ` [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi George Cherian @ 2018-08-24 13:07 ` Corey Minyard 2018-08-27 6:07 ` George Cherian 1 sibling, 1 reply; 9+ messages in thread From: Corey Minyard @ 2018-08-24 13:07 UTC (permalink / raw) To: George Cherian, linux-kernel, openipmi-developer; +Cc: arnd, gregkh 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 <george.cherian@cavium.com> > --- > 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. */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif 2018-08-24 13:07 ` [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif Corey Minyard @ 2018-08-27 6:07 ` George Cherian 2018-08-27 23:29 ` Corey Minyard 0 siblings, 1 reply; 9+ messages in thread From: George Cherian @ 2018-08-27 6:07 UTC (permalink / raw) To: minyard, George Cherian, linux-kernel, openipmi-developer; +Cc: arnd, gregkh 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 <george.cherian@cavium.com> >> --- >> 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. */ > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif 2018-08-27 6:07 ` George Cherian @ 2018-08-27 23:29 ` Corey Minyard 2018-08-28 14:32 ` George Cherian 0 siblings, 1 reply; 9+ messages in thread From: Corey Minyard @ 2018-08-27 23:29 UTC (permalink / raw) To: George Cherian, George Cherian, linux-kernel, openipmi-developer Cc: arnd, gregkh 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 <george.cherian@cavium.com> >>> --- >>> 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. */ >> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif 2018-08-27 23:29 ` Corey Minyard @ 2018-08-28 14:32 ` George Cherian 2018-08-28 16:57 ` Corey Minyard 0 siblings, 1 reply; 9+ messages in thread From: George Cherian @ 2018-08-28 14:32 UTC (permalink / raw) To: minyard, George Cherian, linux-kernel, openipmi-developer; +Cc: arnd, gregkh 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 <george.cherian@cavium.com> >>>> --- >>>> 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. */ >>> >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif 2018-08-28 14:32 ` George Cherian @ 2018-08-28 16:57 ` Corey Minyard 0 siblings, 0 replies; 9+ messages in thread From: Corey Minyard @ 2018-08-28 16:57 UTC (permalink / raw) To: George Cherian, George Cherian, linux-kernel, openipmi-developer Cc: arnd, gregkh 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-28 16:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-24 11:10 [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif George Cherian 2018-08-24 11:10 ` [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi George Cherian 2018-08-24 13:08 ` Corey Minyard 2018-08-27 5:55 ` George Cherian 2018-08-24 13:07 ` [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif Corey Minyard 2018-08-27 6:07 ` George Cherian 2018-08-27 23:29 ` Corey Minyard 2018-08-28 14:32 ` George Cherian 2018-08-28 16:57 ` Corey Minyard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).