linux-kernel - Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device

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:   Sat, 25 Jun 2022 12:24:29 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Kent Gustavsson <kent@...oris.se>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version
 of iio_device_register

On Fri, 24 Jun 2022 08:29:20 +0200
Marcus Folkesson <marcus.folkesson@...il.com> wrote:

> Thank you for your comments (all of them) Andy!
> 
> On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> > <marcus.folkesson@...il.com> wrote:  
> > >
> > > Keep using managed resources as much as possible.  
> > 
> > You may not mix devm_ and non-devm_ API calls like this.
> > So, you rule of thumb that goto is most of the time wrong after devm_ call.  
> 
> Can you please confirm that clocks and regulators are disabled when the
> resources are handed back?
> I cannot see where when I'm trying to follow the code.
Andy isn't arguing that the goto is wrong but rather that you cannot
in general safely use devm_* calls if their failure leads to having to
do any cleanup.  The reason is the ordering is hard to reason about. Sometimes
it's safe, but often enough causes problems that we basically refuse to think
hard enough to figure out if it is.  Hence basic rule is don't do it.

The issue is this.
probe() {

	non_devm_call_1();
	ret = devm_call_2()
	if (ret)
		goto err;

	return 0;
err:
	unwind_non_devm_call_1()
}

remove() {
	unwind_non_devm_call_1()
}

remove or error path should unwind in opposite order of what happens in probe.
On the rare occasion where that isn't the right choice, there should be very
clear comments to say why.

Order is

remove() -> unwind_non_devm_call_1()
devm_managed_cleanup() -> unwind_devm_call_2()

Whereas should be

remove()-> unwind_call_2() then unwind_call_1()


There are two ways to solve this.  Either only use devm for those
elements in probe() that happen before the first thing you need to
unwind manually or make everything devm managed (it unwinds in reverse
order of setup) devm_add_action_or_reset() allows you to use your
own devm_ managed callbacks if there isn't a standard one available.

Jonathan




> 
> Best regards
> Marcus Folkesson