linux-kernel - Re: [PATCH v2 16/23] counter: interrupt-cnt: Convert to new counter registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e3a4a67-4e97-8660-9437-0250588d5e5e@metafoo.de>
Date:   Mon, 27 Dec 2021 12:21:14 +0100
From:   Lars-Peter Clausen <lars@...afoo.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     William Breathitt Gray <vilhelm.gray@...il.com>,
        kernel@...gutronix.de,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ahmad Fatoum <a.fatoum@...gutronix.de>,
        Oleksij Rempel <linux@...pel-privat.de>
Subject: Re: [PATCH v2 16/23] counter: interrupt-cnt: Convert to new counter
 registration

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@...gutronix.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.