On 08/28/2018 04:59 AM, Corey Minyard wrote:
Reversing the call order didn't help, I am still getting the trace.
On 08/27/2018 01:07 AM, George Cherian wrote:
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
ssif_platform_probe) or a hard-coded entry.Â How does
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
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
Try reversing the order of i2c_del_driver() and platform_driver_unregister()
in cleanup_ipmi_ssif() to test this.
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
ssif_info->thread = kthread_run(....) with
ssif_info->thread = ERR_PTR(-4); so that the probe takes the goto out path.