* [PATCH] rtc: fix chardev initialization races
@ 2014-02-26 10:33 Ales Novak
2014-02-27 0:33 ` Andrew Morton
2018-05-21 12:25 ` Uwe Kleine-König
0 siblings, 2 replies; 7+ messages in thread
From: Ales Novak @ 2014-02-26 10:33 UTC (permalink / raw)
To: a.zummo, akpm; +Cc: rtc-linux, linux-kernel, jkosina, Ales Novak
In many rtc modules, the chardevice file in rtc module probe is
being created prematurely. If the probe fails after the chardevice
file has been created (e.g. after rtc_device_register), it's possible
for a program to open() it, which subsequently can cause memory
corruption.
The race looks like that (thanks Jiri):
CPU0: CPU1:
sys_load_module()
do_init_module()
do_one_initcall()
cmos_do_probe()
rtc_device_register()
__register_chrdev()
cdev->owner = struct module*
open("/dev/rtc0")
rtc_device_unregister()
module_put()
free_module()
module_free(mod->module_core)
/* struct module *module is now
freed */
chrdev_open()
spin_lock(cdev_lock)
cdev_get()
try_module_get()
module_is_live()
/* dereferences already
freed struct module* */
This patch is proposing a solution, splitting the function
{devm_,}rtc_device_register into {devm_,}rtc_device_register{_fs,}.
The {devm_}rtc_device_register_fs which is creating the files, should
be called after it is clear that the probe will pass. It will set the
RTC_DEV_FILES_EXIST into rtc_device->flags.
In case of probe not passing, the rtc_device_unregister will try to
delete the files only if RTC_DEV_FILES_EXIST is set in rtc_device->flags.
Acked-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Ales Novak <alnovak@suse.cz>
---
drivers/rtc/class.c | 47 ++++++++++++++++++++++++++++++++--------------
drivers/rtc/rtc-ab8500.c | 1 +
drivers/rtc/rtc-at91sam9.c | 2 ++
drivers/rtc/rtc-bfin.c | 2 ++
drivers/rtc/rtc-cmos.c | 2 ++
drivers/rtc/rtc-davinci.c | 2 ++
drivers/rtc/rtc-ds1305.c | 2 ++
drivers/rtc/rtc-ds1307.c | 2 ++
drivers/rtc/rtc-ds1511.c | 2 ++
drivers/rtc/rtc-ds1553.c | 2 ++
drivers/rtc/rtc-ds1672.c | 2 ++
drivers/rtc/rtc-ds1742.c | 2 ++
drivers/rtc/rtc-ds3232.c | 2 ++
drivers/rtc/rtc-ep93xx.c | 2 ++
drivers/rtc/rtc-isl1208.c | 2 ++
drivers/rtc/rtc-jz4740.c | 2 ++
drivers/rtc/rtc-m41t80.c | 2 ++
drivers/rtc/rtc-m48t59.c | 2 ++
drivers/rtc/rtc-max8998.c | 1 +
drivers/rtc/rtc-mrst.c | 2 ++
drivers/rtc/rtc-nuc900.c | 2 ++
drivers/rtc/rtc-omap.c | 2 ++
drivers/rtc/rtc-pcap.c | 2 ++
drivers/rtc/rtc-pcf2123.c | 1 +
drivers/rtc/rtc-pl031.c | 2 ++
drivers/rtc/rtc-r9701.c | 2 ++
drivers/rtc/rtc-rp5c01.c | 2 ++
drivers/rtc/rtc-rs5c372.c | 2 ++
drivers/rtc/rtc-rv3029c2.c | 1 +
drivers/rtc/rtc-rx8025.c | 2 ++
drivers/rtc/rtc-spear.c | 2 ++
drivers/rtc/rtc-stk17ta8.c | 2 ++
drivers/rtc/rtc-stmp3xxx.c | 2 ++
drivers/rtc/rtc-tegra.c | 2 ++
drivers/rtc/rtc-test.c | 2 ++
drivers/rtc/rtc-twl.c | 3 +++
drivers/rtc/rtc-tx4939.c | 3 +++
drivers/rtc/rtc-vr41xx.c | 2 ++
drivers/rtc/rtc-vt8500.c | 2 ++
drivers/rtc/rtc-x1205.c | 2 ++
include/linux/rtc.h | 7 ++++++-
41 files changed, 115 insertions(+), 15 deletions(-)
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 589351e..6af8355 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -230,13 +230,6 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
goto exit_kfree;
}
- rtc_dev_add_device(rtc);
- rtc_sysfs_add_device(rtc);
- rtc_proc_add_device(rtc);
-
- dev_info(dev, "rtc core: registered %s as %s\n",
- rtc->name, dev_name(&rtc->dev));
-
return rtc;
exit_kfree:
@@ -252,6 +245,21 @@ exit:
}
EXPORT_SYMBOL_GPL(rtc_device_register);
+/**
+ * rtc_device_register_fs - creates dev+sysfs+proc files
+ * for rtc device
+ */
+void rtc_device_register_fs(struct device *dev, struct rtc_device *rtc)
+{
+ rtc_dev_add_device(rtc);
+ rtc_sysfs_add_device(rtc);
+ rtc_proc_add_device(rtc);
+
+ set_bit(RTC_DEV_FILES_EXIST, &rtc->flags);
+ dev_info(dev, "rtc core: registered %s as %s\n",
+ rtc->name, dev_name(&rtc->dev));
+}
+EXPORT_SYMBOL_GPL(rtc_device_register_fs);
/**
* rtc_device_unregister - removes the previously registered RTC class device
@@ -262,13 +270,15 @@ void rtc_device_unregister(struct rtc_device *rtc)
{
if (get_device(&rtc->dev) != NULL) {
mutex_lock(&rtc->ops_lock);
- /* remove innards of this RTC, then disable it, before
- * letting any rtc_class_open() users access it again
- */
- rtc_sysfs_del_device(rtc);
- rtc_dev_del_device(rtc);
- rtc_proc_del_device(rtc);
- device_unregister(&rtc->dev);
+ if (test_and_clear_bit(RTC_DEV_FILES_EXIST, &rtc->flags)) {
+ /* remove innards of this RTC, then disable it, before
+ * letting any rtc_class_open() users access it again
+ */
+ rtc_sysfs_del_device(rtc);
+ rtc_dev_del_device(rtc);
+ rtc_proc_del_device(rtc);
+ device_unregister(&rtc->dev);
+ }
rtc->ops = NULL;
mutex_unlock(&rtc->ops_lock);
put_device(&rtc->dev);
@@ -328,6 +338,15 @@ struct rtc_device *devm_rtc_device_register(struct device *dev,
EXPORT_SYMBOL_GPL(devm_rtc_device_register);
/**
+ * devm_rtc_device_register_fs - create proc&dev&sys files for the rtc device
+ */
+void devm_rtc_device_register_fs(struct device *dev, struct rtc_device *rtc)
+{
+ rtc_device_register_fs(dev, rtc);
+}
+EXPORT_SYMBOL_GPL(devm_rtc_device_register_fs);
+
+/**
* devm_rtc_device_unregister - resource managed devm_rtc_device_unregister()
* @dev: the device to unregister
* @rtc: the RTC class device to unregister
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index 727e2f5..8e25da6 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -503,6 +503,7 @@ static int ab8500_rtc_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "sysfs RTC failed to register\n");
return err;
}
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
return 0;
}
diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 309b8b3..0108c6a 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -369,6 +369,8 @@ static int at91_rtc_probe(struct platform_device *pdev)
dev_warn(&pdev->dev, "%s: SET TIME!\n",
dev_name(&rtc->rtcdev->dev));
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 0c53f45..a7190b8 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -380,6 +380,8 @@ static int bfin_rtc_probe(struct platform_device *pdev)
bfin_rtc_reset(dev, RTC_ISTAT_WRITE_COMPLETE);
bfin_write_RTC_SWCNT(0);
+ devm_rtc_device_register_fs(&pdev->dev, rtc->rtc_dev);
+
return 0;
err:
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index cae212f..fff8b9d 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -784,6 +784,8 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
goto cleanup2;
}
+ rtc_device_register_fs(dev, cmos_rtc.rtc);
+
dev_info(dev, "%s%s, %zd bytes nvram%s\n",
!is_valid_irq(rtc_irq) ? "no alarms" :
cmos_rtc.mon_alrm ? "alarms up to one year" :
diff --git a/drivers/rtc/rtc-davinci.c b/drivers/rtc/rtc-davinci.c
index 24677ef8..57467ac 100644
--- a/drivers/rtc/rtc-davinci.c
+++ b/drivers/rtc/rtc-davinci.c
@@ -554,6 +554,8 @@ static int __init davinci_rtc_probe(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, 0);
+ devm_rtc_device_register_fs(&pdev->dev, davinci_rtc->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-ds1305.c b/drivers/rtc/rtc-ds1305.c
index 2dd586a..fb85d20 100644
--- a/drivers/rtc/rtc-ds1305.c
+++ b/drivers/rtc/rtc-ds1305.c
@@ -771,6 +771,8 @@ static int ds1305_probe(struct spi_device *spi)
return status;
}
+ devm_rtc_device_register_fs(&spi->dev, ds1305->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4e75345..31cffee 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -972,6 +972,8 @@ read_rtc:
dev_info(&client->dev, "%zu bytes nvram\n", ds1307->nvram->size);
}
+ devm_rtc_device_register_fs(&client->dev, ds1307->rtc);
+
return 0;
err_irq:
diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c
index bc7b4fc..14f076d 100644
--- a/drivers/rtc/rtc-ds1511.c
+++ b/drivers/rtc/rtc-ds1511.c
@@ -534,6 +534,8 @@ static int ds1511_rtc_probe(struct platform_device *pdev)
ret = sysfs_create_bin_file(&pdev->dev.kobj, &ds1511_nvram_attr);
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return ret;
}
diff --git a/drivers/rtc/rtc-ds1553.c b/drivers/rtc/rtc-ds1553.c
index fd31571..735ea10 100644
--- a/drivers/rtc/rtc-ds1553.c
+++ b/drivers/rtc/rtc-ds1553.c
@@ -329,6 +329,8 @@ static int ds1553_rtc_probe(struct platform_device *pdev)
ret = sysfs_create_bin_file(&pdev->dev.kobj, &ds1553_nvram_attr);
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return ret;
}
diff --git a/drivers/rtc/rtc-ds1672.c b/drivers/rtc/rtc-ds1672.c
index 18e2d84..2ad2578 100644
--- a/drivers/rtc/rtc-ds1672.c
+++ b/drivers/rtc/rtc-ds1672.c
@@ -189,6 +189,8 @@ static int ds1672_probe(struct i2c_client *client,
if (err)
goto exit_devreg;
+ devm_rtc_device_register_fs(&client->dev, rtc);
+
return 0;
exit_devreg:
diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
index 5a1f3b2..d4444b0 100644
--- a/drivers/rtc/rtc-ds1742.c
+++ b/drivers/rtc/rtc-ds1742.c
@@ -205,6 +205,8 @@ static int ds1742_rtc_probe(struct platform_device *pdev)
ret = sysfs_create_bin_file(&pdev->dev.kobj, &pdata->nvram_attr);
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return ret;
}
diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
index b83bb5a5..778a9d5 100644
--- a/drivers/rtc/rtc-ds3232.c
+++ b/drivers/rtc/rtc-ds3232.c
@@ -427,6 +427,8 @@ static int ds3232_probe(struct i2c_client *client,
}
}
+ devm_rtc_device_register_fs(&client->dev, ds3232->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-ep93xx.c b/drivers/rtc/rtc-ep93xx.c
index 5e4f5dc..fbe9a01 100644
--- a/drivers/rtc/rtc-ep93xx.c
+++ b/drivers/rtc/rtc-ep93xx.c
@@ -156,6 +156,8 @@ static int ep93xx_rtc_probe(struct platform_device *pdev)
if (err)
goto exit;
+ devm_rtc_device_register_fs(&pdev->dev, ep93xx_rtc->rtc);
+
return 0;
exit:
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index c3c549d..bc25c5b 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -681,6 +681,8 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (rc)
return rc;
+ devm_rtc_device_register_fs(&client->dev, rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index 1b126d2..925600b 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -278,6 +278,8 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
}
}
+ devm_rtc_device_register_fs(&pdev->dev, rtc->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index a5248aa..cd51429 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -707,6 +707,8 @@ static int m41t80_probe(struct i2c_client *client,
}
}
#endif
+ devm_rtc_device_register_fs(&client->dev, rtc);
+
return 0;
st_err:
diff --git a/drivers/rtc/rtc-m48t59.c b/drivers/rtc/rtc-m48t59.c
index 11880c1..027bcbd 100644
--- a/drivers/rtc/rtc-m48t59.c
+++ b/drivers/rtc/rtc-m48t59.c
@@ -489,6 +489,8 @@ static int m48t59_rtc_probe(struct platform_device *pdev)
if (ret)
return ret;
+ devm_rtc_device_register_fs(&pdev->dev, m48t59->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-max8998.c b/drivers/rtc/rtc-max8998.c
index f098ad8..c74020d 100644
--- a/drivers/rtc/rtc-max8998.c
+++ b/drivers/rtc/rtc-max8998.c
@@ -300,6 +300,7 @@ no_irq:
dev_warn(&pdev->dev, "LP3974 with RTC REGERR option."
" RTC updates will be extremely slow.\n");
}
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
return 0;
}
diff --git a/drivers/rtc/rtc-mrst.c b/drivers/rtc/rtc-mrst.c
index e2436d1..f61a2ef 100644
--- a/drivers/rtc/rtc-mrst.c
+++ b/drivers/rtc/rtc-mrst.c
@@ -374,6 +374,7 @@ static int vrtc_mrst_do_probe(struct device *dev, struct resource *iomem,
goto cleanup1;
}
}
+ rtc_device_register_fs(dev, mrst_rtc.rtc);
dev_dbg(dev, "initialised\n");
return 0;
@@ -403,6 +404,7 @@ static void rtc_mrst_do_remove(struct device *dev)
if (mrst->irq)
free_irq(mrst->irq, mrst->rtc);
+ rtc_device_unregister_fs(mrst->rtc);
rtc_device_unregister(mrst->rtc);
mrst->rtc = NULL;
diff --git a/drivers/rtc/rtc-nuc900.c b/drivers/rtc/rtc-nuc900.c
index 248653c..64dd6e2 100644
--- a/drivers/rtc/rtc-nuc900.c
+++ b/drivers/rtc/rtc-nuc900.c
@@ -257,6 +257,8 @@ static int __init nuc900_rtc_probe(struct platform_device *pdev)
return -EBUSY;
}
+ devm_rtc_device_register_fs(&pdev->dev, nuc900_rtc->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 26de5f8..5fb141f 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -449,6 +449,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
if (reg != new_ctrl)
rtc_write(new_ctrl, OMAP_RTC_CTRL_REG);
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return 0;
fail0:
diff --git a/drivers/rtc/rtc-pcap.c b/drivers/rtc/rtc-pcap.c
index 40b5c63..9aaac77 100644
--- a/drivers/rtc/rtc-pcap.c
+++ b/drivers/rtc/rtc-pcap.c
@@ -172,6 +172,8 @@ static int __init pcap_rtc_probe(struct platform_device *pdev)
if (err)
return err;
+ devm_rtc_device_register_fs(&pdev->dev, pcap_rtc->rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d1953bb..874d85d 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -313,6 +313,7 @@ static int pcf2123_probe(struct spi_device *spi)
goto sysfs_exit;
}
}
+ devm_rtc_device_register_fs(&spi->dev, rtc);
return 0;
diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
index 99181fff..5f04b29 100644
--- a/drivers/rtc/rtc-pl031.c
+++ b/drivers/rtc/rtc-pl031.c
@@ -306,6 +306,7 @@ static int pl031_remove(struct amba_device *adev)
struct pl031_local *ldata = dev_get_drvdata(&adev->dev);
free_irq(adev->irq[0], ldata);
+ rtc_device_unregister_fs(ldata->rtc);
rtc_device_unregister(ldata->rtc);
iounmap(ldata->base);
kfree(ldata);
@@ -384,6 +385,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
goto out_no_irq;
}
+ rtc_device_register_fs(&adev->dev, ldata->rtc);
return 0;
out_no_irq:
diff --git a/drivers/rtc/rtc-r9701.c b/drivers/rtc/rtc-r9701.c
index feeedbd..5f39f64 100644
--- a/drivers/rtc/rtc-r9701.c
+++ b/drivers/rtc/rtc-r9701.c
@@ -161,6 +161,8 @@ static int r9701_probe(struct spi_device *spi)
spi_set_drvdata(spi, rtc);
+ devm_rtc_device_register_fs(&spi->dev, rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-rp5c01.c b/drivers/rtc/rtc-rp5c01.c
index 89d0736..83d8655 100644
--- a/drivers/rtc/rtc-rp5c01.c
+++ b/drivers/rtc/rtc-rp5c01.c
@@ -259,6 +259,8 @@ static int __init rp5c01_rtc_probe(struct platform_device *dev)
if (error)
return error;
+ devm_rtc_device_register_fs(&dev->dev, rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index ccf54f0..a426eca 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -668,6 +668,8 @@ static int rs5c372_probe(struct i2c_client *client,
if (err)
goto exit;
+ devm_rtc_device_register_fs(&client->dev, rs5c372->rtc);
+
return 0;
exit:
diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c
index 1a779a6..57e7d2e 100644
--- a/drivers/rtc/rtc-rv3029c2.c
+++ b/drivers/rtc/rtc-rv3029c2.c
@@ -408,6 +408,7 @@ static int rv3029c2_probe(struct i2c_client *client,
dev_err(&client->dev, "reading status failed\n");
return rc;
}
+ devm_rtc_device_register_fs(&client->dev, rtc);
return 0;
}
diff --git a/drivers/rtc/rtc-rx8025.c b/drivers/rtc/rtc-rx8025.c
index 8fa23ea..b07fac1 100644
--- a/drivers/rtc/rtc-rx8025.c
+++ b/drivers/rtc/rtc-rx8025.c
@@ -597,6 +597,8 @@ static int rx8025_probe(struct i2c_client *client,
if (err)
goto errout_irq;
+ devm_rtc_device_register_fs(&client->dev, rx8025->rtc);
+
return 0;
errout_irq:
diff --git a/drivers/rtc/rtc-spear.c b/drivers/rtc/rtc-spear.c
index c492cf0..a644636 100644
--- a/drivers/rtc/rtc-spear.c
+++ b/drivers/rtc/rtc-spear.c
@@ -414,6 +414,8 @@ static int spear_rtc_probe(struct platform_device *pdev)
if (!device_can_wakeup(&pdev->dev))
device_init_wakeup(&pdev->dev, 1);
+ devm_rtc_device_register_fs(&pdev->dev, config->rtc);
+
return 0;
err_disable_clock:
diff --git a/drivers/rtc/rtc-stk17ta8.c b/drivers/rtc/rtc-stk17ta8.c
index a176ba6..ec53d2a 100644
--- a/drivers/rtc/rtc-stk17ta8.c
+++ b/drivers/rtc/rtc-stk17ta8.c
@@ -338,6 +338,8 @@ static int stk17ta8_rtc_probe(struct platform_device *pdev)
ret = sysfs_create_bin_file(&pdev->dev.kobj, &stk17ta8_nvram_attr);
+ devm_rtc_device_register_fs(&pdev->dev, pdata->rtc);
+
return ret;
}
diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index ea96492..0d8c575 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -304,6 +304,8 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
return err;
}
+ devm_rtc_device_register_fs(&pdev->dev, rtc_data->rtc);
+
stmp3xxx_wdt_register(pdev);
return 0;
}
diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
index 76af92a..084d1ab 100644
--- a/drivers/rtc/rtc-tegra.c
+++ b/drivers/rtc/rtc-tegra.c
@@ -365,6 +365,8 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
dev_notice(&pdev->dev, "Tegra internal Real Time Clock\n");
+ devm_rtc_device_register_fs(&pdev->dev, info->rtc_dev);
+
return 0;
}
diff --git a/drivers/rtc/rtc-test.c b/drivers/rtc/rtc-test.c
index 7746e65..0737eca 100644
--- a/drivers/rtc/rtc-test.c
+++ b/drivers/rtc/rtc-test.c
@@ -114,6 +114,8 @@ static int test_probe(struct platform_device *plat_dev)
platform_set_drvdata(plat_dev, rtc);
+ devm_rtc_device_register_fs(&plat_dev->dev, rtc);
+
return 0;
err:
diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
index 1915464..e9614e8 100644
--- a/drivers/rtc/rtc-twl.c
+++ b/drivers/rtc/rtc-twl.c
@@ -544,6 +544,9 @@ static int twl_rtc_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, rtc);
+
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return 0;
}
diff --git a/drivers/rtc/rtc-tx4939.c b/drivers/rtc/rtc-tx4939.c
index 4f87234..40b68f93 100644
--- a/drivers/rtc/rtc-tx4939.c
+++ b/drivers/rtc/rtc-tx4939.c
@@ -269,6 +269,9 @@ static int __init tx4939_rtc_probe(struct platform_device *pdev)
pdata->rtc = rtc;
ret = sysfs_create_bin_file(&pdev->dev.kobj, &tx4939_rtc_nvram_attr);
+ if (!ret)
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
return ret;
}
diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
index 88c9c92..d84865b 100644
--- a/drivers/rtc/rtc-vr41xx.c
+++ b/drivers/rtc/rtc-vr41xx.c
@@ -355,6 +355,8 @@ static int rtc_probe(struct platform_device *pdev)
disable_irq(aie_irq);
disable_irq(pie_irq);
+ devm_rtc_device_register_fs(&pdev->dev, rtc);
+
dev_info(&pdev->dev, "Real Time Clock of NEC VR4100 series\n");
return 0;
diff --git a/drivers/rtc/rtc-vt8500.c b/drivers/rtc/rtc-vt8500.c
index df2ef3e..121f544 100644
--- a/drivers/rtc/rtc-vt8500.c
+++ b/drivers/rtc/rtc-vt8500.c
@@ -269,6 +269,8 @@ static int vt8500_rtc_probe(struct platform_device *pdev)
goto err_return;
}
+ devm_rtc_device_register_fs(&pdev->dev, vt8500_rtc->rtc);
+
return 0;
err_return:
diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index 365dc65..679df80 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -662,6 +662,8 @@ static int x1205_probe(struct i2c_client *client,
if (err)
return err;
+ devm_rtc_device_register_fs(&client->dev, rtc);
+
return 0;
}
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index c2c2897..245432a 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -81,7 +81,9 @@ struct rtc_timer {
/* flags */
-#define RTC_DEV_BUSY 0
+#define RTC_DEV_BUSY 0
+/* flag indicating the files for the device have been created */
+#define RTC_DEV_FILES_EXIST 1
struct rtc_device
{
@@ -133,10 +135,13 @@ extern struct rtc_device *rtc_device_register(const char *name,
struct device *dev,
const struct rtc_class_ops *ops,
struct module *owner);
+
+extern void rtc_device_register_fs(struct device *dev, struct rtc_device *rtc);
extern struct rtc_device *devm_rtc_device_register(struct device *dev,
const char *name,
const struct rtc_class_ops *ops,
struct module *owner);
+extern void devm_rtc_device_register_fs(struct device *dev, struct rtc_device *rtc);
extern void rtc_device_unregister(struct rtc_device *rtc);
extern void devm_rtc_device_unregister(struct device *dev,
struct rtc_device *rtc);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] rtc: fix chardev initialization races
2014-02-26 10:33 [PATCH] rtc: fix chardev initialization races Ales Novak
@ 2014-02-27 0:33 ` Andrew Morton
2018-05-21 12:25 ` Uwe Kleine-König
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2014-02-27 0:33 UTC (permalink / raw)
To: Ales Novak; +Cc: a.zummo, rtc-linux, linux-kernel, jkosina
On Wed, 26 Feb 2014 11:33:13 +0100 Ales Novak <alnovak@suse.cz> wrote:
> In many rtc modules, the chardevice file in rtc module probe is
> being created prematurely. If the probe fails after the chardevice
> file has been created (e.g. after rtc_device_register), it's possible
> for a program to open() it, which subsequently can cause memory
> corruption.
>
> The race looks like that (thanks Jiri):
>
> CPU0: CPU1:
> sys_load_module()
> do_init_module()
> do_one_initcall()
> cmos_do_probe()
> rtc_device_register()
> __register_chrdev()
> cdev->owner = struct module*
> open("/dev/rtc0")
> rtc_device_unregister()
> module_put()
> free_module()
> module_free(mod->module_core)
> /* struct module *module is now
> freed */
> chrdev_open()
> spin_lock(cdev_lock)
> cdev_get()
> try_module_get()
> module_is_live()
> /* dereferences already
> freed struct module* */
>
> This patch is proposing a solution, splitting the function
> {devm_,}rtc_device_register into {devm_,}rtc_device_register{_fs,}.
> The {devm_}rtc_device_register_fs which is creating the files, should
> be called after it is clear that the probe will pass. It will set the
> RTC_DEV_FILES_EXIST into rtc_device->flags.
>
> In case of probe not passing, the rtc_device_unregister will try to
> delete the files only if RTC_DEV_FILES_EXIST is set in rtc_device->flags.
>
> ..
>
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -252,6 +245,21 @@ exit:
> }
> EXPORT_SYMBOL_GPL(rtc_device_register);
>
> +/**
> + * rtc_device_register_fs - creates dev+sysfs+proc files
> + * for rtc device
> + */
> +void rtc_device_register_fs(struct device *dev, struct rtc_device *rtc)
> +{
> + rtc_dev_add_device(rtc);
> + rtc_sysfs_add_device(rtc);
> + rtc_proc_add_device(rtc);
It's a bit sloppy that rtc core simply ignores the error codes from the
driver core functions. All these things could have failed and the
driver just keeps on trying to work.
I guess that can be fixed separately, but perhaps you'd like to take a
look, see how complex it will be to correctly unwind after such errors?
> + set_bit(RTC_DEV_FILES_EXIST, &rtc->flags);
> + dev_info(dev, "rtc core: registered %s as %s\n",
> + rtc->name, dev_name(&rtc->dev));
> +}
> +EXPORT_SYMBOL_GPL(rtc_device_register_fs);
>
> /**
> * rtc_device_unregister - removes the previously registered RTC class device
> @@ -262,13 +270,15 @@ void rtc_device_unregister(struct rtc_device *rtc)
> {
> if (get_device(&rtc->dev) != NULL) {
> mutex_lock(&rtc->ops_lock);
> - /* remove innards of this RTC, then disable it, before
> - * letting any rtc_class_open() users access it again
> - */
> - rtc_sysfs_del_device(rtc);
> - rtc_dev_del_device(rtc);
> - rtc_proc_del_device(rtc);
> - device_unregister(&rtc->dev);
> + if (test_and_clear_bit(RTC_DEV_FILES_EXIST, &rtc->flags)) {
> + /* remove innards of this RTC, then disable it, before
> + * letting any rtc_class_open() users access it again
> + */
> + rtc_sysfs_del_device(rtc);
> + rtc_dev_del_device(rtc);
> + rtc_proc_del_device(rtc);
> + device_unregister(&rtc->dev);
Presumably it doesn't matter, but it would be nice to do the del
functions in the reverse order of the add functions.
rtc_proc_del_device() then rtc_sysfs_del_device() then
rtc_dev_del_device().
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: rtc: fix chardev initialization races
2014-02-26 10:33 [PATCH] rtc: fix chardev initialization races Ales Novak
2014-02-27 0:33 ` Andrew Morton
@ 2018-05-21 12:25 ` Uwe Kleine-König
2018-05-22 12:09 ` Jiri Kosina
1 sibling, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2018-05-21 12:25 UTC (permalink / raw)
To: Ales Novak; +Cc: a.zummo, akpm, rtc-linux, linux-kernel, jkosina
[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]
On Wed, Feb 26, 2014 at 11:33:13AM +0100, Ales Novak wrote:
> In many rtc modules, the chardevice file in rtc module probe is
> being created prematurely. If the probe fails after the chardevice
> file has been created (e.g. after rtc_device_register), it's possible
> for a program to open() it, which subsequently can cause memory
> corruption.
>
> The race looks like that (thanks Jiri):
>
> CPU0: CPU1:
> sys_load_module()
> do_init_module()
> do_one_initcall()
> cmos_do_probe()
> rtc_device_register()
> __register_chrdev()
> cdev->owner = struct module*
> open("/dev/rtc0")
> rtc_device_unregister()
> module_put()
> free_module()
> module_free(mod->module_core)
> /* struct module *module is now
> freed */
> chrdev_open()
> spin_lock(cdev_lock)
> cdev_get()
> try_module_get()
> module_is_live()
> /* dereferences already
> freed struct module* */
[Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
fail after rtc_device_register successfully finished and pointed to this
reasoning as explaination.]
If there is really such a race then (I hope) there is
something in the cdev code that needs fixing. According to my
understanding, when rtc_device_unregister returned, the cdev is gone and
so chrdev_open is supposed to fail.
Otherwise the same problem can be triggered when the device is unbound
and the module unloaded while another cpu opens the char dev.
I added a msleep(5000) to chrdev_open like this:
if (current->pid != 1) {
pr_info("%s:%d: sleeping to open race window\n", __func__, __LINE__);
msleep(5000);
pr_info("%s:%d: done sleeping\n", __func__, __LINE__);
}
before the spinlock is taken. If I trigger that (using
a = open("/dev/rtc0")
in a Python shell) and then do rmmod rtc_pcf2127 (which is the driver
backing my rtc0), I get:
OSError: [Errno 6] No such device or address: '/dev/rtc0'
> This patch is proposing a solution, splitting the function
> {devm_,}rtc_device_register into {devm_,}rtc_device_register{_fs,}.
> The {devm_}rtc_device_register_fs which is creating the files, should
> be called after it is clear that the probe will pass. It will set the
> RTC_DEV_FILES_EXIST into rtc_device->flags.
So this split is not a complete fix but only a work around for some
cases at best.
Maybe there isn't even a problem?
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: rtc: fix chardev initialization races
2018-05-21 12:25 ` Uwe Kleine-König
@ 2018-05-22 12:09 ` Jiri Kosina
2018-05-22 13:39 ` Uwe Kleine-König
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Kosina @ 2018-05-22 12:09 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ales Novak, a.zummo, akpm, rtc-linux, linux-kernel
On Mon, 21 May 2018, Uwe Kleine-König wrote:
> > The race looks like that (thanks Jiri):
> >
> > CPU0: CPU1:
> > sys_load_module()
> > do_init_module()
> > do_one_initcall()
> > cmos_do_probe()
> > rtc_device_register()
> > __register_chrdev()
> > cdev->owner = struct module*
> > open("/dev/rtc0")
> > rtc_device_unregister()
> > module_put()
> > free_module()
> > module_free(mod->module_core)
> > /* struct module *module is now
> > freed */
> > chrdev_open()
> > spin_lock(cdev_lock)
> > cdev_get()
> > try_module_get()
> > module_is_live()
> > /* dereferences already
> > freed struct module* */
>
> [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> fail after rtc_device_register successfully finished and pointed to this
> reasoning as explaination.]
>
> If there is really such a race then (I hope) there is
> something in the cdev code that needs fixing. According to my
> understanding, when rtc_device_unregister returned, the cdev is gone and
> so chrdev_open is supposed to fail.
Oh wow, hello back to 4 years ago!
Looking at the current code, I don't think there is no such race any more,
as the last thing cmos_do_probe() -> __rtc_register_device() does that can
potentially fail is the chardev creation itself.
IOW if it exists, it's guaranteed to not go away on a probe error handling
path (and the rest is protected via ops_lock mutex).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: rtc: fix chardev initialization races
2018-05-22 12:09 ` Jiri Kosina
@ 2018-05-22 13:39 ` Uwe Kleine-König
2018-05-22 15:33 ` Jiri Kosina
0 siblings, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2018-05-22 13:39 UTC (permalink / raw)
To: Jiri Kosina
Cc: Ales Novak, a.zummo, akpm, rtc-linux, linux-kernel, linux-rtc,
Alexandre Belloni
[adding linux-rtc ML and Alexandre to Cc:]
Hello,
On Tue, May 22, 2018 at 02:09:36PM +0200, Jiri Kosina wrote:
> On Mon, 21 May 2018, Uwe Kleine-König wrote:
>
> > > The race looks like that (thanks Jiri):
> > >
> > > CPU0: CPU1:
> > > sys_load_module()
> > > do_init_module()
> > > do_one_initcall()
> > > cmos_do_probe()
> > > rtc_device_register()
> > > __register_chrdev()
> > > cdev->owner = struct module*
> > > open("/dev/rtc0")
> > > rtc_device_unregister()
> > > module_put()
> > > free_module()
> > > module_free(mod->module_core)
> > > /* struct module *module is now
> > > freed */
> > > chrdev_open()
> > > spin_lock(cdev_lock)
> > > cdev_get()
> > > try_module_get()
> > > module_is_live()
> > > /* dereferences already
> > > freed struct module* */
> >
> > [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> > fail after rtc_device_register successfully finished and pointed to this
> > reasoning as explaination.]
> >
> > If there is really such a race then (I hope) there is
> > something in the cdev code that needs fixing. According to my
> > understanding, when rtc_device_unregister returned, the cdev is gone and
> > so chrdev_open is supposed to fail.
>
> Oh wow, hello back to 4 years ago!
:-)
> Looking at the current code, I don't think there is no such race any more,
> as the last thing cmos_do_probe() -> __rtc_register_device() does that can
> potentially fail is the chardev creation itself.
OK, so you agree that it's also save to do something in a driver's probe
after rtc_device_register() and call rtc_device_unregister() in the error
path, right? (That's the motivation for me to discuss this old topic.)
> IOW if it exists, it's guaranteed to not go away on a probe error handling
> path (and the rest is protected via ops_lock mutex).
Putting it in other words (assuming I got you right):
If there is a race, it needs fixing in the cdev code. The rtc driver can
stay as is.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: rtc: fix chardev initialization races
2018-05-22 13:39 ` Uwe Kleine-König
@ 2018-05-22 15:33 ` Jiri Kosina
2018-09-04 22:02 ` Alexandre Belloni
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Kosina @ 2018-05-22 15:33 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Ales Novak, a.zummo, akpm, rtc-linux, linux-kernel, linux-rtc,
Alexandre Belloni
On Tue, 22 May 2018, Uwe Kleine-König wrote:
> [adding linux-rtc ML and Alexandre to Cc:]
>
> Hello,
>
> On Tue, May 22, 2018 at 02:09:36PM +0200, Jiri Kosina wrote:
> > On Mon, 21 May 2018, Uwe Kleine-König wrote:
> >
> > > > The race looks like that (thanks Jiri):
> > > >
> > > > CPU0: CPU1:
> > > > sys_load_module()
> > > > do_init_module()
> > > > do_one_initcall()
> > > > cmos_do_probe()
> > > > rtc_device_register()
> > > > __register_chrdev()
> > > > cdev->owner = struct module*
> > > > open("/dev/rtc0")
> > > > rtc_device_unregister()
> > > > module_put()
> > > > free_module()
> > > > module_free(mod->module_core)
> > > > /* struct module *module is now
> > > > freed */
> > > > chrdev_open()
> > > > spin_lock(cdev_lock)
> > > > cdev_get()
> > > > try_module_get()
> > > > module_is_live()
> > > > /* dereferences already
> > > > freed struct module* */
> > >
> > > [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> > > fail after rtc_device_register successfully finished and pointed to this
> > > reasoning as explaination.]
> > >
> > > If there is really such a race then (I hope) there is
> > > something in the cdev code that needs fixing. According to my
> > > understanding, when rtc_device_unregister returned, the cdev is gone and
> > > so chrdev_open is supposed to fail.
> >
> > Oh wow, hello back to 4 years ago!
>
> :-)
>
> > Looking at the current code, I don't think there is no such race any more,
> > as the last thing cmos_do_probe() -> __rtc_register_device() does that can
> > potentially fail is the chardev creation itself.
>
> OK, so you agree that it's also save to do something in a driver's probe
> after rtc_device_register() and call rtc_device_unregister() in the error
> path, right?
Hmm, not really; that's what the code apparently did 4 years ago (judging
from the scenario in the old mail, I of course forgot all the details),
but doesn't do it any more.
Looking at the current code, if you call rtc_device_unregister() in the
probe path, where is the guarantee that cdev_get() will not derefernce
already freed struct module*?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: rtc: fix chardev initialization races
2018-05-22 15:33 ` Jiri Kosina
@ 2018-09-04 22:02 ` Alexandre Belloni
0 siblings, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2018-09-04 22:02 UTC (permalink / raw)
To: Jiri Kosina
Cc: Uwe Kleine-König, Ales Novak, a.zummo, akpm, rtc-linux,
linux-kernel, linux-rtc
On 22/05/2018 17:33:40+0200, Jiri Kosina wrote:
> On Tue, 22 May 2018, Uwe Kleine-König wrote:
> > [adding linux-rtc ML and Alexandre to Cc:]
> >
> > Hello,
> >
> > On Tue, May 22, 2018 at 02:09:36PM +0200, Jiri Kosina wrote:
> > > On Mon, 21 May 2018, Uwe Kleine-König wrote:
> > >
> > > > > The race looks like that (thanks Jiri):
> > > > >
> > > > > CPU0: CPU1:
> > > > > sys_load_module()
> > > > > do_init_module()
> > > > > do_one_initcall()
> > > > > cmos_do_probe()
> > > > > rtc_device_register()
> > > > > __register_chrdev()
> > > > > cdev->owner = struct module*
> > > > > open("/dev/rtc0")
> > > > > rtc_device_unregister()
> > > > > module_put()
> > > > > free_module()
> > > > > module_free(mod->module_core)
> > > > > /* struct module *module is now
> > > > > freed */
> > > > > chrdev_open()
> > > > > spin_lock(cdev_lock)
> > > > > cdev_get()
> > > > > try_module_get()
> > > > > module_is_live()
> > > > > /* dereferences already
> > > > > freed struct module* */
> > > >
> > > > [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> > > > fail after rtc_device_register successfully finished and pointed to this
> > > > reasoning as explaination.]
> > > >
> > > > If there is really such a race then (I hope) there is
> > > > something in the cdev code that needs fixing. According to my
> > > > understanding, when rtc_device_unregister returned, the cdev is gone and
> > > > so chrdev_open is supposed to fail.
> > >
> > > Oh wow, hello back to 4 years ago!
> >
> > :-)
> >
>
> > > Looking at the current code, I don't think there is no such race any more,
> > > as the last thing cmos_do_probe() -> __rtc_register_device() does that can
> > > potentially fail is the chardev creation itself.
> >
> > OK, so you agree that it's also save to do something in a driver's probe
> > after rtc_device_register() and call rtc_device_unregister() in the error
> > path, right?
>
> Hmm, not really; that's what the code apparently did 4 years ago (judging
> from the scenario in the old mail, I of course forgot all the details),
> but doesn't do it any more.
>
> Looking at the current code, if you call rtc_device_unregister() in the
> probe path, where is the guarantee that cdev_get() will not derefernce
> already freed struct module*?
>
I've also tried to produce it and couldn't. The whole magic happens in
kobj_lookup. Freeing the module before ends up with the application
opening the char dev getting -ENXIO. Freeing the module after
kobj_lookup is not possible and ends up with -EBUSY.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-04 22:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-26 10:33 [PATCH] rtc: fix chardev initialization races Ales Novak
2014-02-27 0:33 ` Andrew Morton
2018-05-21 12:25 ` Uwe Kleine-König
2018-05-22 12:09 ` Jiri Kosina
2018-05-22 13:39 ` Uwe Kleine-König
2018-05-22 15:33 ` Jiri Kosina
2018-09-04 22:02 ` Alexandre Belloni
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).