| Message ID | 20211229154441.38045-14-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: cleanups and device lifetime fixes | expand |
On Wed, 29 Dec 2021 16:44:31 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The current implementation gets device lifetime tracking wrong. The > problem is that allocation of struct counter_device is controlled by the > individual drivers but this structure contains a struct device that > might have to live longer than a driver is bound. As a result a command > sequence like: > > { sleep 5; echo bang; } > /dev/counter0 & > sleep 1; > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > can keep a reference to the struct device and unbinding results in > freeing the memory occupied by this device resulting in an oops. > > This commit provides two new functions (plus some helpers): > - counter_alloc() to allocate a struct counter_device that is > automatically freed once the embedded struct device is released > - counter_add() to register such a device. > > Note that this commit doesn't fix any issues, all drivers have to be > converted to these new functions to correct the lifetime problems. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Basically fine - a few trivial comments inline that I'm not that fussed about whether you take notice of or not. As such Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- I'd have liked to have seen a change log here. Quite a few comments on this one and not all had 'obvious' resolutions. > drivers/counter/counter-core.c | 168 ++++++++++++++++++++++++++++++++- > include/linux/counter.h | 15 +++ > 2 files changed, 181 insertions(+), 2 deletions(-) > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > index 00c41f28c101..b3fa15bbcbdb 100644 > --- a/drivers/counter/counter-core.c > +++ b/drivers/counter/counter-core.c > @@ -15,6 +15,7 @@ > #include <linux/kdev_t.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/slab.h> > #include <linux/types.h> > #include <linux/wait.h> > > @@ -24,6 +25,16 @@ > /* Provides a unique ID for each counter device */ > static DEFINE_IDA(counter_ida); > > +struct counter_device_allochelper { > + struct counter_device counter; > + > + /* > + * This is cache line aligned to ensure private data behaves like if it > + * were kmalloced separately. > + */ > + unsigned long privdata[] ____cacheline_aligned; Change log for the patch would have made it easier to see you decided to make this change after the discussion in v2. > +}; > + ... > > +/** > + * counter_alloc - allocate a counter_device > + * @sizeof_priv: size of the driver private data > + * > + * This is part one of counter registration. The structure is allocated > + * dynamically to ensure the right lifetime for the embedded struct device. > + * > + * If this succeeds, call counter_put() to get rid of the counter_device again. > + */ > +struct counter_device *counter_alloc(size_t sizeof_priv) > +{ > + struct counter_device_allochelper *ch; > + struct counter_device *counter; > + struct device *dev; > + int err; > + > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > + if (!ch) { > + err = -ENOMEM; > + goto err_alloc_ch; Slight preference for a direct return here even though it means replicating the ERR_PTR() statement. Makes for one less error path where a reviewer has to go see what is being done. > + } > + > + counter = &ch->counter; > + dev = &counter->dev; > + > + /* Acquire unique ID */ > + err = ida_alloc(&counter_ida, GFP_KERNEL); > + if (err < 0) > + goto err_ida_alloc; > + dev->id = err; > + > + mutex_init(&counter->ops_exist_lock); > + dev->type = &counter_device_type; > + dev->bus = &counter_bus_type; > + dev->devt = MKDEV(MAJOR(counter_devt), dev->id); > + > + err = counter_chrdev_add(counter); > + if (err < 0) > + goto err_chrdev_add; > + > + device_initialize(dev); > + > + return counter; > + > +err_chrdev_add: > + Nitpick: Unusual spacing (to my eye anyway). I wouldn't expect to see a blank line after a label as the label indentation makes a visual separation anyway. > + ida_free(&counter_ida, dev->id); > +err_ida_alloc: > + > + kfree(ch); > +err_alloc_ch: > + > + return ERR_PTR(err); > +} > +EXPORT_SYMBOL_GPL(counter_alloc);
Hello Jonathan, On Wed, Dec 29, 2021 at 05:06:12PM +0000, Jonathan Cameron wrote: > On Wed, 29 Dec 2021 16:44:31 +0100 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > The current implementation gets device lifetime tracking wrong. The > > problem is that allocation of struct counter_device is controlled by the > > individual drivers but this structure contains a struct device that > > might have to live longer than a driver is bound. As a result a command > > sequence like: > > > > { sleep 5; echo bang; } > /dev/counter0 & > > sleep 1; > > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > > > can keep a reference to the struct device and unbinding results in > > freeing the memory occupied by this device resulting in an oops. > > > > This commit provides two new functions (plus some helpers): > > - counter_alloc() to allocate a struct counter_device that is > > automatically freed once the embedded struct device is released > > - counter_add() to register such a device. > > > > Note that this commit doesn't fix any issues, all drivers have to be > > converted to these new functions to correct the lifetime problems. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Basically fine - a few trivial comments inline that I'm not that fussed > about whether you take notice of or not. As such > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks. > I'd have liked to have seen a change log here. Quite a few comments on this > one and not all had 'obvious' resolutions. yeah, should have been a bit less lazy and comment a bit more than the range diff in the cover letter. To catch up: - privdata is now cache line aligned - consistent bracing for oneline if blocks - fixed the warning by the 0day bot which explained that only one device can be bound - reordered the body of counter_add() to better match counter_register() - Add an EXPORT_SYMBOL_GPL(counter_put); - Drop one pair of get_device/put_device - kernel doc for devm_counter_a{lloc,dd} > > drivers/counter/counter-core.c | 168 ++++++++++++++++++++++++++++++++- > > include/linux/counter.h | 15 +++ > > 2 files changed, 181 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > > index 00c41f28c101..b3fa15bbcbdb 100644 > > --- a/drivers/counter/counter-core.c > > +++ b/drivers/counter/counter-core.c > > @@ -15,6 +15,7 @@ > > #include <linux/kdev_t.h> > > #include <linux/module.h> > > #include <linux/mutex.h> > > +#include <linux/slab.h> > > #include <linux/types.h> > > #include <linux/wait.h> > > > > @@ -24,6 +25,16 @@ > > /* Provides a unique ID for each counter device */ > > static DEFINE_IDA(counter_ida); > > > > +struct counter_device_allochelper { > > + struct counter_device counter; > > + > > + /* > > + * This is cache line aligned to ensure private data behaves like if it > > + * were kmalloced separately. > > + */ > > + unsigned long privdata[] ____cacheline_aligned; > > Change log for the patch would have made it easier to see you decided > to make this change after the discussion in v2. Yeah, this was a wim of the moment after I saw that this usually only results in a 32 byte alignment. > > +}; > > + > > ... > > > > > +/** > > + * counter_alloc - allocate a counter_device > > + * @sizeof_priv: size of the driver private data > > + * > > + * This is part one of counter registration. The structure is allocated > > + * dynamically to ensure the right lifetime for the embedded struct device. > > + * > > + * If this succeeds, call counter_put() to get rid of the counter_device again. > > + */ > > +struct counter_device *counter_alloc(size_t sizeof_priv) > > +{ > > + struct counter_device_allochelper *ch; > > + struct counter_device *counter; > > + struct device *dev; > > + int err; > > + > > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > > + if (!ch) { > > + err = -ENOMEM; > > + goto err_alloc_ch; > > Slight preference for a direct return here even though it means > replicating the ERR_PTR() statement. Makes for one less error > path where a reviewer has to go see what is being done. I'll consider it if it comes to a v4. > > + } > > + > > + counter = &ch->counter; > > + dev = &counter->dev; > > + > > + /* Acquire unique ID */ > > + err = ida_alloc(&counter_ida, GFP_KERNEL); > > + if (err < 0) > > + goto err_ida_alloc; > > + dev->id = err; > > + > > + mutex_init(&counter->ops_exist_lock); > > + dev->type = &counter_device_type; > > + dev->bus = &counter_bus_type; > > + dev->devt = MKDEV(MAJOR(counter_devt), dev->id); > > + > > + err = counter_chrdev_add(counter); > > + if (err < 0) > > + goto err_chrdev_add; > > + > > + device_initialize(dev); > > + > > + return counter; > > + > > +err_chrdev_add: > > Nitpick: Unusual spacing (to my eye anyway). I wouldn't expect to see a blank line after a label > as the label indentation makes a visual separation anyway. I know this is unusual, but I like this approach. The error label is named after what failed (instead of the more usual what has to be undone first) and then it's grouped to the matching undo-function. See https://lore.kernel.org/linux-pwm/20201106093435.4mlr6ujivvkzkd5z@pengutronix.de for a more verbose reasoning (which however failed to convince my fellow pwm maintainers :-\). > > + ida_free(&counter_ida, dev->id); > > +err_ida_alloc: > > + > > + kfree(ch); > > +err_alloc_ch: > > + > > + return ERR_PTR(err); > > +} Best regards and thanks for your feedback, Uwe
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c index 00c41f28c101..b3fa15bbcbdb 100644 --- a/drivers/counter/counter-core.c +++ b/drivers/counter/counter-core.c @@ -15,6 +15,7 @@ #include <linux/kdev_t.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/slab.h> #include <linux/types.h> #include <linux/wait.h> @@ -24,6 +25,16 @@ /* Provides a unique ID for each counter device */ static DEFINE_IDA(counter_ida); +struct counter_device_allochelper { + struct counter_device counter; + + /* + * This is cache line aligned to ensure private data behaves like if it + * were kmalloced separately. + */ + unsigned long privdata[] ____cacheline_aligned; +}; + static void counter_device_release(struct device *dev) { struct counter_device *const counter = @@ -31,6 +42,9 @@ static void counter_device_release(struct device *dev) counter_chrdev_remove(counter); ida_free(&counter_ida, dev->id); + + if (!counter->legacy_device) + kfree(container_of(counter, struct counter_device_allochelper, counter)); } static struct device_type counter_device_type = { @@ -53,7 +67,14 @@ static dev_t counter_devt; */ void *counter_priv(const struct counter_device *const counter) { - return counter->priv; + if (counter->legacy_device) { + return counter->priv; + } else { + struct counter_device_allochelper *ch = + container_of(counter, struct counter_device_allochelper, counter); + + return &ch->privdata; + } } EXPORT_SYMBOL_GPL(counter_priv); @@ -74,6 +95,8 @@ int counter_register(struct counter_device *const counter) int id; int err; + counter->legacy_device = true; + /* Acquire unique ID */ id = ida_alloc(&counter_ida, GFP_KERNEL); if (id < 0) @@ -114,6 +137,95 @@ int counter_register(struct counter_device *const counter) } EXPORT_SYMBOL_GPL(counter_register); +/** + * counter_alloc - allocate a counter_device + * @sizeof_priv: size of the driver private data + * + * This is part one of counter registration. The structure is allocated + * dynamically to ensure the right lifetime for the embedded struct device. + * + * If this succeeds, call counter_put() to get rid of the counter_device again. + */ +struct counter_device *counter_alloc(size_t sizeof_priv) +{ + struct counter_device_allochelper *ch; + struct counter_device *counter; + struct device *dev; + int err; + + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); + if (!ch) { + err = -ENOMEM; + goto err_alloc_ch; + } + + counter = &ch->counter; + dev = &counter->dev; + + /* Acquire unique ID */ + err = ida_alloc(&counter_ida, GFP_KERNEL); + if (err < 0) + goto err_ida_alloc; + dev->id = err; + + mutex_init(&counter->ops_exist_lock); + dev->type = &counter_device_type; + dev->bus = &counter_bus_type; + dev->devt = MKDEV(MAJOR(counter_devt), dev->id); + + err = counter_chrdev_add(counter); + if (err < 0) + goto err_chrdev_add; + + device_initialize(dev); + + return counter; + +err_chrdev_add: + + ida_free(&counter_ida, dev->id); +err_ida_alloc: + + kfree(ch); +err_alloc_ch: + + return ERR_PTR(err); +} +EXPORT_SYMBOL_GPL(counter_alloc); + +void counter_put(struct counter_device *counter) +{ + put_device(&counter->dev); +} +EXPORT_SYMBOL_GPL(counter_put); + +/** + * counter_add - complete registration of a counter + * @counter: the counter to add + * + * This is part two of counter registration. + * + * If this succeeds, call counter_unregister() to get rid of the counter_device again. + */ +int counter_add(struct counter_device *counter) +{ + int err; + struct device *dev = &counter->dev; + + if (counter->parent) { + dev->parent = counter->parent; + dev->of_node = counter->parent->of_node; + } + + err = counter_sysfs_add(counter); + if (err < 0) + return err; + + /* implies device_add(dev) */ + return cdev_device_add(&counter->chrdev, dev); +} +EXPORT_SYMBOL_GPL(counter_add); + /** * counter_unregister - unregister Counter from the system * @counter: pointer to Counter to unregister @@ -134,7 +246,8 @@ void counter_unregister(struct counter_device *const counter) mutex_unlock(&counter->ops_exist_lock); - put_device(&counter->dev); + if (counter->legacy_device) + put_device(&counter->dev); } EXPORT_SYMBOL_GPL(counter_unregister); @@ -168,6 +281,57 @@ int devm_counter_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_counter_register); +static void devm_counter_put(void *counter) +{ + counter_put(counter); +} + +/** + * devm_counter_alloc - allocate a counter_device + * @dev: the device to register the release callback for + * @sizeof_priv: size of the driver private data + * + * This is the device managed version of counter_add(). It registers a cleanup + * callback to care for calling counter_put(). + */ +struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv) +{ + struct counter_device *counter; + int err; + + counter = counter_alloc(sizeof_priv); + if (IS_ERR(counter)) + return counter; + + err = devm_add_action_or_reset(dev, devm_counter_put, counter); + if (err < 0) + return ERR_PTR(err); + + return counter; +} +EXPORT_SYMBOL_GPL(devm_counter_alloc); + +/** + * devm_counter_add - complete registration of a counter + * @dev: the device to register the release callback for + * @counter: the counter to add + * + * This is the device managed version of counter_add(). It registers a cleanup + * callback to care for calling counter_unregister(). + */ +int devm_counter_add(struct device *dev, + struct counter_device *const counter) +{ + int err; + + err = counter_add(counter); + if (err < 0) + return err; + + return devm_add_action_or_reset(dev, devm_counter_release, counter); +} +EXPORT_SYMBOL_GPL(devm_counter_add); + #define COUNTER_DEV_MAX 256 static int __init counter_init(void) diff --git a/include/linux/counter.h b/include/linux/counter.h index 8daaa38c71d8..f1350a43cd48 100644 --- a/include/linux/counter.h +++ b/include/linux/counter.h @@ -327,14 +327,29 @@ struct counter_device { spinlock_t events_in_lock; struct mutex events_out_lock; struct mutex ops_exist_lock; + + /* + * This can go away once all drivers are converted to + * counter_alloc()/counter_add(). + */ + bool legacy_device; }; void *counter_priv(const struct counter_device *const counter); int counter_register(struct counter_device *const counter); + +struct counter_device *counter_alloc(size_t sizeof_priv); +void counter_put(struct counter_device *const counter); +int counter_add(struct counter_device *const counter); + void counter_unregister(struct counter_device *const counter); int devm_counter_register(struct device *dev, struct counter_device *const counter); +struct counter_device *devm_counter_alloc(struct device *dev, + size_t sizeof_priv); +int devm_counter_add(struct device *dev, + struct counter_device *const counter); void counter_push_event(struct counter_device *const counter, const u8 event, const u8 channel);
The current implementation gets device lifetime tracking wrong. The problem is that allocation of struct counter_device is controlled by the individual drivers but this structure contains a struct device that might have to live longer than a driver is bound. As a result a command sequence like: { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind can keep a reference to the struct device and unbinding results in freeing the memory occupied by this device resulting in an oops. This commit provides two new functions (plus some helpers): - counter_alloc() to allocate a struct counter_device that is automatically freed once the embedded struct device is released - counter_add() to register such a device. Note that this commit doesn't fix any issues, all drivers have to be converted to these new functions to correct the lifetime problems. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/counter/counter-core.c | 168 ++++++++++++++++++++++++++++++++- include/linux/counter.h | 15 +++ 2 files changed, 181 insertions(+), 2 deletions(-)