linux-kernel - Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions

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]

Date:   Fri, 5 May 2017 01:41:06 +0100
From:   André Przywara <andre.przywara@....com>
To:     Tejun Heo <tj@...nel.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Icenowy Zheng <icenowy@...c.xyz>,
        Adam Borowski <kilobyte@...band.pl>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Chen-Yu Tsai <wens@...e.org>, linux-sunxi@...glegroups.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions

On 04/05/17 17:00, Tejun Heo wrote:

Hi,

> Hello,
> 
> On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:
>>> @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
>>>  		radix_tree_delete(&pctldev->pin_group_tree, indices[i]);
>>>  		devm_kfree(pctldev->dev, group);
>>>  	}
>>> +	kfree(indices);
>>
>> We use devm_kfree for other allocations done here, maybe we can just
>> have the same thing here? We would be consistant, and we would still
>> keep the resource tracking.
> 
> It doesn't make any sense to use the managed functions from the
> release functions and if you're always matching devm_kmalloc() with
> devm_kfree(), the only thing it'd do is confusing its readers.

I agree, the idea of this patch is to get rid of managed functions. Also
my understanding is that devm_kfree needs to match some devm_kmalloc().

> And the function in question just looks weird to me - give up on
> freeing resources if memory allocation fails?  And why call
> devm_kfree() on objects which are being released anyway?  Or if the
> group can be released w/o the device being detached, why use devm
> allocations on the members at all?
> 
> As for removal, can't it just call radix_tree_iter_delete() while
> iterating?  Why allocate memory to iterate and store all indices and
> to look them up again and delete them?

Yeah, I was wondering about this as well. It looks like it just wants to
clean up everything; the implementation of ida_destroy() seems to have
the same intention and uses radix_tree_iter_delete().

Using this algorithm would avoid the memory allocation in the first
place, so fix the issue as well and simplify the code.

So is there anything we miss here why we store all indices and iterate
twice over the tree?

Cheers,
Andre.