| Message ID | 20211227094526.698714-17-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2,01/23] counter: Use container_of instead of drvdata to track counter_device | expand |
On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: > This fixes device lifetime issues where it was possible to free a live > struct device. > > Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > index 4bf706ef46e2..9e99702470c2 100644 > --- a/drivers/counter/interrupt-cnt.c > +++ b/drivers/counter/interrupt-cnt.c > @@ -16,7 +16,6 @@ > > struct interrupt_cnt_priv { > atomic_t count; > - struct counter_device counter; > struct gpio_desc *gpio; > int irq; > bool enabled; > @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { > static int interrupt_cnt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct counter_device *counter; > struct interrupt_cnt_priv *priv; > int ret; > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > + counter = devm_counter_alloc(dev, sizeof(*priv)); I just picked one of these patches at random, nothing specific about this driver... You can not have a 'struct device' in memory allocated by devm_*() functions for the obvious reason that now that memory is being controlled by a reference count that is OUTSIDE of the structure itself. So while your goal might be good here, this is not the correct solution at all, sorry. thanks, greg k-h
On 12/27/21 11:59 AM, Greg Kroah-Hartman wrote: > On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: >> This fixes device lifetime issues where it was possible to free a live >> struct device. >> >> Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> --- >> drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ >> 1 file changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c >> index 4bf706ef46e2..9e99702470c2 100644 >> --- a/drivers/counter/interrupt-cnt.c >> +++ b/drivers/counter/interrupt-cnt.c >> @@ -16,7 +16,6 @@ >> >> struct interrupt_cnt_priv { >> atomic_t count; >> - struct counter_device counter; >> struct gpio_desc *gpio; >> int irq; >> bool enabled; >> @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { >> static int interrupt_cnt_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> + struct counter_device *counter; >> struct interrupt_cnt_priv *priv; >> int ret; >> >> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> - if (!priv) >> + counter = devm_counter_alloc(dev, sizeof(*priv)); > I just picked one of these patches at random, nothing specific about > this driver... > > You can not have a 'struct device' in memory allocated by devm_*() > functions for the obvious reason that now that memory is being > controlled by a reference count that is OUTSIDE of the structure itself. > > So while your goal might be good here, this is not the correct solution > at all, sorry. Before this patch the memory for the struct device was devm_kzalloc'ed. Which as you point out is a bug. After this patch the memory is reference counted and will be freed when the last reference is dropped, in the release callback of the struct device. The alloc function is still a devm_ function, but on 'free' it will only drop the reference to the struct device that it holds. This is a very common pattern that is used by basically any driver subsystem in the kernel. This is the correct solution to the problem.
On Mon, Dec 27, 2021 at 12:21:14PM +0100, Lars-Peter Clausen wrote: > On 12/27/21 11:59 AM, Greg Kroah-Hartman wrote: > > On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: > > > This fixes device lifetime issues where it was possible to free a live > > > struct device. > > > > > > Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ > > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > > > index 4bf706ef46e2..9e99702470c2 100644 > > > --- a/drivers/counter/interrupt-cnt.c > > > +++ b/drivers/counter/interrupt-cnt.c > > > @@ -16,7 +16,6 @@ > > > struct interrupt_cnt_priv { > > > atomic_t count; > > > - struct counter_device counter; > > > struct gpio_desc *gpio; > > > int irq; > > > bool enabled; > > > @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { > > > static int interrupt_cnt_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > + struct counter_device *counter; > > > struct interrupt_cnt_priv *priv; > > > int ret; > > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > > - if (!priv) > > > + counter = devm_counter_alloc(dev, sizeof(*priv)); > > I just picked one of these patches at random, nothing specific about > > this driver... > > > > You can not have a 'struct device' in memory allocated by devm_*() > > functions for the obvious reason that now that memory is being > > controlled by a reference count that is OUTSIDE of the structure itself. > > > > So while your goal might be good here, this is not the correct solution > > at all, sorry. > > Before this patch the memory for the struct device was devm_kzalloc'ed. > Which as you point out is a bug. > > After this patch the memory is reference counted and will be freed when the > last reference is dropped, in the release callback of the struct device. > > The alloc function is still a devm_ function, but on 'free' it will only > drop the reference to the struct device that it holds. This is a very common > pattern that is used by basically any driver subsystem in the kernel. Then it is not a real devm_() call, let's not call it that please as it is obviously very confusing :) Just call it counter_alloc(), or , counter_create(), or something a bit more in line with the rest of all driver subsystems. thanks, greg k-h
On 12/27/21 12:34 PM, Greg Kroah-Hartman wrote: > On Mon, Dec 27, 2021 at 12:21:14PM +0100, Lars-Peter Clausen wrote: >> On 12/27/21 11:59 AM, Greg Kroah-Hartman wrote: >>> On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: >>>> This fixes device lifetime issues where it was possible to free a live >>>> struct device. >>>> >>>> Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") >>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>>> --- >>>> drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ >>>> 1 file changed, 16 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c >>>> index 4bf706ef46e2..9e99702470c2 100644 >>>> --- a/drivers/counter/interrupt-cnt.c >>>> +++ b/drivers/counter/interrupt-cnt.c >>>> @@ -16,7 +16,6 @@ >>>> struct interrupt_cnt_priv { >>>> atomic_t count; >>>> - struct counter_device counter; >>>> struct gpio_desc *gpio; >>>> int irq; >>>> bool enabled; >>>> @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { >>>> static int interrupt_cnt_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> + struct counter_device *counter; >>>> struct interrupt_cnt_priv *priv; >>>> int ret; >>>> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>> - if (!priv) >>>> + counter = devm_counter_alloc(dev, sizeof(*priv)); >>> I just picked one of these patches at random, nothing specific about >>> this driver... >>> >>> You can not have a 'struct device' in memory allocated by devm_*() >>> functions for the obvious reason that now that memory is being >>> controlled by a reference count that is OUTSIDE of the structure itself. >>> >>> So while your goal might be good here, this is not the correct solution >>> at all, sorry. >> Before this patch the memory for the struct device was devm_kzalloc'ed. >> Which as you point out is a bug. >> >> After this patch the memory is reference counted and will be freed when the >> last reference is dropped, in the release callback of the struct device. >> >> The alloc function is still a devm_ function, but on 'free' it will only >> drop the reference to the struct device that it holds. This is a very common >> pattern that is used by basically any driver subsystem in the kernel. > Then it is not a real devm_() call, let's not call it that please as it > is obviously very confusing :) > > Just call it counter_alloc(), or , counter_create(), or something a bit > more in line with the rest of all driver subsystems. But all the other driver subsystems call this kind of function devm_... :) Usually for everything you call in probe() you need a corresponding action in remove(). With the devm_... function is remove action will be automatically called.
On Mon, Dec 27, 2021 at 12:34:25PM +0100, Greg Kroah-Hartman wrote: > On Mon, Dec 27, 2021 at 12:21:14PM +0100, Lars-Peter Clausen wrote: > > On 12/27/21 11:59 AM, Greg Kroah-Hartman wrote: > > > On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: > > > > This fixes device lifetime issues where it was possible to free a live > > > > struct device. > > > > > > > > Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ > > > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > > > > index 4bf706ef46e2..9e99702470c2 100644 > > > > --- a/drivers/counter/interrupt-cnt.c > > > > +++ b/drivers/counter/interrupt-cnt.c > > > > @@ -16,7 +16,6 @@ > > > > struct interrupt_cnt_priv { > > > > atomic_t count; > > > > - struct counter_device counter; > > > > struct gpio_desc *gpio; > > > > int irq; > > > > bool enabled; > > > > @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { > > > > static int interrupt_cnt_probe(struct platform_device *pdev) > > > > { > > > > struct device *dev = &pdev->dev; > > > > + struct counter_device *counter; > > > > struct interrupt_cnt_priv *priv; > > > > int ret; > > > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > > > - if (!priv) > > > > + counter = devm_counter_alloc(dev, sizeof(*priv)); > > > I just picked one of these patches at random, nothing specific about > > > this driver... > > > > > > You can not have a 'struct device' in memory allocated by devm_*() > > > functions for the obvious reason that now that memory is being > > > controlled by a reference count that is OUTSIDE of the structure itself. > > > > > > So while your goal might be good here, this is not the correct solution > > > at all, sorry. > > > > Before this patch the memory for the struct device was devm_kzalloc'ed. > > Which as you point out is a bug. > > > > After this patch the memory is reference counted and will be freed when the > > last reference is dropped, in the release callback of the struct device. @Lars, thanks for arguing for my case. > > The alloc function is still a devm_ function, but on 'free' it will only > > drop the reference to the struct device that it holds. This is a very common > > pattern that is used by basically any driver subsystem in the kernel. > > Then it is not a real devm_() call, let's not call it that please as it > is obviously very confusing :) It's only confusing if you consider the m in devm to stand for "malloc", but it's for "managed". I agree to Lars that it's a sane approach. > Just call it counter_alloc(), or , counter_create(), or something a bit > more in line with the rest of all driver subsystems. There is a counter_alloc() and in contrast to devm_counter_alloc() the caller has to care about dropping the reference to it by hand. Best regards Uwe
On Mon, 27 Dec 2021 10:45:19 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > This fixes device lifetime issues where it was possible to free a live > struct device. > > Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > index 4bf706ef46e2..9e99702470c2 100644 > --- a/drivers/counter/interrupt-cnt.c > +++ b/drivers/counter/interrupt-cnt.c > @@ -16,7 +16,6 @@ > > struct interrupt_cnt_priv { > atomic_t count; > - struct counter_device counter; > struct gpio_desc *gpio; > int irq; > bool enabled; > @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { > static int interrupt_cnt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct counter_device *counter; > struct interrupt_cnt_priv *priv; > int ret; > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > + counter = devm_counter_alloc(dev, sizeof(*priv)); > + if (!counter) > return -ENOMEM; > + priv = counter_priv(counter); > > priv->irq = platform_get_irq_optional(pdev, 0); > if (priv->irq == -ENXIO) > @@ -184,8 +185,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > if (!priv->signals.name) > return -ENOMEM; > > - priv->counter.signals = &priv->signals; > - priv->counter.num_signals = 1; > + counter->signals = &priv->signals; > + counter->num_signals = 1; > > priv->synapses.actions_list = interrupt_cnt_synapse_actions; > priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actions); > @@ -199,12 +200,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > priv->cnts.ext = interrupt_cnt_ext; > priv->cnts.num_ext = ARRAY_SIZE(interrupt_cnt_ext); > > - priv->counter.priv = priv; > - priv->counter.name = dev_name(dev); > - priv->counter.parent = dev; > - priv->counter.ops = &interrupt_cnt_ops; > - priv->counter.counts = &priv->cnts; > - priv->counter.num_counts = 1; > + counter->name = dev_name(dev); > + counter->parent = dev; > + counter->ops = &interrupt_cnt_ops; > + counter->counts = &priv->cnts; > + counter->num_counts = 1; > > irq_set_status_flags(priv->irq, IRQ_NOAUTOEN); > ret = devm_request_irq(dev, priv->irq, interrupt_cnt_isr, > @@ -213,7 +213,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > if (ret) > return ret; > > - return devm_counter_register(dev, &priv->counter); > + ret = devm_counter_add(dev, counter); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to add counter\n"); > + > + return 0; > } > > static const struct of_device_id interrupt_cnt_of_match[] = {
On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: > This fixes device lifetime issues where it was possible to free a live > struct device. > > Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- > drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > index 4bf706ef46e2..9e99702470c2 100644 > --- a/drivers/counter/interrupt-cnt.c > +++ b/drivers/counter/interrupt-cnt.c > @@ -16,7 +16,6 @@ > > struct interrupt_cnt_priv { > atomic_t count; > - struct counter_device counter; > struct gpio_desc *gpio; > int irq; > bool enabled; > @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { > static int interrupt_cnt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct counter_device *counter; > struct interrupt_cnt_priv *priv; > int ret; > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > + counter = devm_counter_alloc(dev, sizeof(*priv)); > + if (!counter) > return -ENOMEM; > + priv = counter_priv(counter); > > priv->irq = platform_get_irq_optional(pdev, 0); > if (priv->irq == -ENXIO) > @@ -184,8 +185,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > if (!priv->signals.name) > return -ENOMEM; > > - priv->counter.signals = &priv->signals; > - priv->counter.num_signals = 1; > + counter->signals = &priv->signals; > + counter->num_signals = 1; > > priv->synapses.actions_list = interrupt_cnt_synapse_actions; > priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actions); > @@ -199,12 +200,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > priv->cnts.ext = interrupt_cnt_ext; > priv->cnts.num_ext = ARRAY_SIZE(interrupt_cnt_ext); > > - priv->counter.priv = priv; > - priv->counter.name = dev_name(dev); > - priv->counter.parent = dev; > - priv->counter.ops = &interrupt_cnt_ops; > - priv->counter.counts = &priv->cnts; > - priv->counter.num_counts = 1; > + counter->name = dev_name(dev); > + counter->parent = dev; > + counter->ops = &interrupt_cnt_ops; > + counter->counts = &priv->cnts; > + counter->num_counts = 1; > > irq_set_status_flags(priv->irq, IRQ_NOAUTOEN); > ret = devm_request_irq(dev, priv->irq, interrupt_cnt_isr, > @@ -213,7 +213,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) > if (ret) > return ret; > > - return devm_counter_register(dev, &priv->counter); > + ret = devm_counter_add(dev, counter); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to add counter\n"); > + > + return 0; > } > > static const struct of_device_id interrupt_cnt_of_match[] = { > -- > 2.33.0 >
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c index 4bf706ef46e2..9e99702470c2 100644 --- a/drivers/counter/interrupt-cnt.c +++ b/drivers/counter/interrupt-cnt.c @@ -16,7 +16,6 @@ struct interrupt_cnt_priv { atomic_t count; - struct counter_device counter; struct gpio_desc *gpio; int irq; bool enabled; @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { static int interrupt_cnt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct counter_device *counter; struct interrupt_cnt_priv *priv; int ret; - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); - if (!priv) + counter = devm_counter_alloc(dev, sizeof(*priv)); + if (!counter) return -ENOMEM; + priv = counter_priv(counter); priv->irq = platform_get_irq_optional(pdev, 0); if (priv->irq == -ENXIO) @@ -184,8 +185,8 @@ static int interrupt_cnt_probe(struct platform_device *pdev) if (!priv->signals.name) return -ENOMEM; - priv->counter.signals = &priv->signals; - priv->counter.num_signals = 1; + counter->signals = &priv->signals; + counter->num_signals = 1; priv->synapses.actions_list = interrupt_cnt_synapse_actions; priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actions); @@ -199,12 +200,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) priv->cnts.ext = interrupt_cnt_ext; priv->cnts.num_ext = ARRAY_SIZE(interrupt_cnt_ext); - priv->counter.priv = priv; - priv->counter.name = dev_name(dev); - priv->counter.parent = dev; - priv->counter.ops = &interrupt_cnt_ops; - priv->counter.counts = &priv->cnts; - priv->counter.num_counts = 1; + counter->name = dev_name(dev); + counter->parent = dev; + counter->ops = &interrupt_cnt_ops; + counter->counts = &priv->cnts; + counter->num_counts = 1; irq_set_status_flags(priv->irq, IRQ_NOAUTOEN); ret = devm_request_irq(dev, priv->irq, interrupt_cnt_isr, @@ -213,7 +213,11 @@ static int interrupt_cnt_probe(struct platform_device *pdev) if (ret) return ret; - return devm_counter_register(dev, &priv->counter); + ret = devm_counter_add(dev, counter); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to add counter\n"); + + return 0; } static const struct of_device_id interrupt_cnt_of_match[] = {
This fixes device lifetime issues where it was possible to free a live struct device. Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)