| Message ID | 20191008163956.GB566933@t480s.localdomain |
|---|---|
| State | Under Review |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | [RFCv3] i2c: hold the core_lock for the whole execution of i2c_register_adapter() | expand |
Hi Slawomir, On Tue, Oct 08, 2019 at 06:39:56PM +0200, Slawomir Stepien wrote: > From: Sławomir Stępień <slawomir.stepien@nokia.com> > > There is a race condition between the i2c_get_adapter() and the > i2c_add_adapter() if this mutex isn't hold for the whole execution of > i2c_register_adapter(). > > If the mutex isn't locked, it is possible to find idr that points to > adapter that hasn't been registered yet (i.e. it's > kobj.state_initialized is still false), which will end up with warning > message: > > "... is not initialized, yet kobject_get() is being called." > > This patch will change how the locking is arranged around > i2c_register_adapter() call and will prevent such situations. The part > of the i2c_register_adapter() that do not need to be under the lock has > been moved to a new function i2c_process_adapter. > > Signed-off-by: Sławomir Stępień <slawomir.stepien@nokia.com> Thank you for tackling this one and sorry for the late reply. Do you have a test case for me so I could reproduce the bad case here? Kind regards, Wolfram
On mar 21, 2020 20:15, Wolfram Sang wrote: > Hi Slawomir, Hello Wolfram, > On Tue, Oct 08, 2019 at 06:39:56PM +0200, Slawomir Stepien wrote: > > From: Sławomir Stępień <slawomir.stepien@nokia.com> > > > > There is a race condition between the i2c_get_adapter() and the > > i2c_add_adapter() if this mutex isn't hold for the whole execution of > > i2c_register_adapter(). > > > > If the mutex isn't locked, it is possible to find idr that points to > > adapter that hasn't been registered yet (i.e. it's > > kobj.state_initialized is still false), which will end up with warning > > message: > > > > "... is not initialized, yet kobject_get() is being called." > > > > This patch will change how the locking is arranged around > > i2c_register_adapter() call and will prevent such situations. The part > > of the i2c_register_adapter() that do not need to be under the lock has > > been moved to a new function i2c_process_adapter. > > > > Signed-off-by: Sławomir Stępień <slawomir.stepien@nokia.com> > > Thank you for tackling this one and sorry for the late reply. > > Do you have a test case for me so I could reproduce the bad case here? I don't have any test case ready on hand, but please take a look at this flow: Note: The assumption is that i2c_add_adapter() and i2c_get_adapter() are called from separate threads of execution. time | i2c_add_adapter() | i2c_get_adapter() ------------------------------------------------ 0001 | lock of core_lock | 0002 | new idr via idr_alloc | 0003 | unlock of core_lock | 0004 | | lock of core_lock 0005 | | idr_find 0006 | | get_device [1] 0007 | i2c_register_adapter | At point [1], the i2c_get_adapter() assumes the device is ready only because it was found in idr. It calls get_device() which causes kobject_get() to fail.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 5f6a4985f2bc..cf9c5d18a24c 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1352,6 +1352,23 @@ static int i2c_register_adapter(struct i2c_adapter *adap) dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); + return 0; + +out_reg: + init_completion(&adap->dev_released); + device_unregister(&adap->dev); + wait_for_completion(&adap->dev_released); +out_list: + idr_remove(&i2c_adapter_idr, adap->nr); + return res; +} + +static void i2c_process_adapter(struct i2c_adapter *adap) +{ +#ifdef CONFIG_I2C_COMPAT + int res; +#endif + pm_runtime_no_callbacks(&adap->dev); pm_suspend_ignore_children(&adap->dev, true); pm_runtime_enable(&adap->dev); @@ -1378,18 +1395,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap) mutex_lock(&core_lock); bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter); mutex_unlock(&core_lock); - - return 0; - -out_reg: - init_completion(&adap->dev_released); - device_unregister(&adap->dev); - wait_for_completion(&adap->dev_released); -out_list: - mutex_lock(&core_lock); - idr_remove(&i2c_adapter_idr, adap->nr); - mutex_unlock(&core_lock); - return res; } /** @@ -1401,15 +1406,24 @@ static int i2c_register_adapter(struct i2c_adapter *adap) */ static int __i2c_add_numbered_adapter(struct i2c_adapter *adap) { - int id; + int id, ret; mutex_lock(&core_lock); id = idr_alloc(&i2c_adapter_idr, adap, adap->nr, adap->nr + 1, GFP_KERNEL); - mutex_unlock(&core_lock); - if (WARN(id < 0, "couldn't get idr")) + if (WARN(id < 0, "couldn't get idr")) { + mutex_unlock(&core_lock); return id == -ENOSPC ? -EBUSY : id; + } + + ret = i2c_register_adapter(adap); + mutex_unlock(&core_lock); + + if (ret < 0) + return ret; - return i2c_register_adapter(adap); + i2c_process_adapter(adap); + + return 0; } /** @@ -1429,7 +1443,7 @@ static int __i2c_add_numbered_adapter(struct i2c_adapter *adap) int i2c_add_adapter(struct i2c_adapter *adapter) { struct device *dev = &adapter->dev; - int id; + int id, ret; if (dev->of_node) { id = of_alias_get_id(dev->of_node, "i2c"); @@ -1442,13 +1456,22 @@ int i2c_add_adapter(struct i2c_adapter *adapter) mutex_lock(&core_lock); id = idr_alloc(&i2c_adapter_idr, adapter, __i2c_first_dynamic_bus_num, 0, GFP_KERNEL); - mutex_unlock(&core_lock); - if (WARN(id < 0, "couldn't get idr")) + if (WARN(id < 0, "couldn't get idr")) { + mutex_unlock(&core_lock); return id; + } adapter->nr = id; - return i2c_register_adapter(adapter); + ret = i2c_register_adapter(adapter); + mutex_unlock(&core_lock); + + if (ret < 0) + return ret; + + i2c_process_adapter(adapter); + + return 0; } EXPORT_SYMBOL(i2c_add_adapter);