On Sat, Dec 25, 2021 at 05:34:51PM +0100, Marc Kleine-Budde wrote:
> On 25.12.2021 17:10:46, Uwe Kleine-König 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@xxxxxxxxxxxxxx>
> > ---
> > drivers/counter/counter-core.c | 149 ++++++++++++++++++++++++++++++++-
> > include/linux/counter.h | 15 ++++
> > 2 files changed, 163 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > index 00c41f28c101..17a93e6c018a 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,11 @@
> > /* Provides a unique ID for each counter device */
> > static DEFINE_IDA(counter_ida);
> >
> > +struct counter_device_allochelper {
> > + struct counter_device counter;
> > + unsigned long privdata[0];
> > +};
>
> Is this a use case for DECLARE_FLEX_ARRAY()?
Probably I want to drop the 0 to make this c99 instead of c89 gcc
extention. I didn't know DECLARE_FLEX_ARRAY, not sure if this is more
beneficial.
Anyhow, having said that, the loop hooping with struct
counter_device_allochelper can maybe be replaced by some addition and
pointer alignment. At least that's how it's done in netdev_priv().
I'm open to whatever William and/or Greg prefers.
One thing we/they might want to decide quickly is if the fixes included
here should go into v5.16. Maybe it's too late in the cycle and given
the problem isn't actually new, maybe fix this for v5.17? (It's as old
as the counter framework which was included first in v5.2-rc1 in Apr
2019.) Iff this is considered important enough to go into 5.16, maybe we
should discuss privdata[0] vs privdata[] vs DECLARE_FLEX_ARRAY only
later?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
