[PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[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

[PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi

[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

Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[Corey Minyard]

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. */

Re: [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi

[Corey Minyard]

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) {

Re: [PATCH 2/2] ipmi_ssif: Fix crash seen while ipmi_unregister_smi

[George Cherian]

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) {
> 
> 

Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[George Cherian]

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. */
> 
> 

Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[Corey Minyard]

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. */
>>
>>

Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[George Cherian]

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. */
>>>
>>>
> 

Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

[Corey Minyard]

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