linux-kernel - Re: [PATCH] reset: Don't WARN if trying to get a used reset control

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:   Thu, 7 Feb 2019 09:27:51 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Jon Hunter <jonathanh@...dia.com>, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] reset: Don't WARN if trying to get a used reset control

On Wed, Feb 06, 2019 at 07:12:04PM +0100, Philipp Zabel wrote:
> On Wed, 2019-02-06 at 17:00 +0100, Thierry Reding wrote:
[...]
> > That way we operate on the same reset control, but we wouldn't need to
> > iterate over all existing reset controls in order to determine whether
> > we can acquire or not.
> 
> How could we decide in reset_control_assert whether the provided rstc is
> allowed to change the reset line if both rstc handles point to the same
> struct reset_control?

The idea was that acquire/release would basically act as lock/unlock for
the reset control. So consumers would always have to call acquire()
before assert()/deassert()/reset() and they would be allowed to continue
only if acquire() returns success. Of course that's something you can
only enforce during code review, but that's pretty much the same as with
any type of locking.

So basically the idea is that if a consumers acquire() call succeeds,
the acquired flag gets set on the reset control and that consumer
becomes the only user allowed to modify the reset control. Any other
consumers would call acquire() and fail because the acquired is already
true.

But what you proposed works for me. We can always find constructive ways
to optimize this later if we need or want to.

Another possibility would be to keep an additional, separate list of the
temporarily exclusive resets so that only that list would have to be
iterated to find the ones that are relevant.

> > >  			if (WARN_ON(!rstc->shared || !shared))
> > >  				return ERR_PTR(-EBUSY);
> > 
> > With the above I think we could just extend this list of conditions with
> > 
> > 	|| acquired
> > 
> > and things should work the same. Or perhaps I'm missing something.
> >
> > Other than that this really looks like a very nice solution.
> 
> Well, apart from the API function names...
> devm_reset_control_get_optional_exclusive_released(dev, "id");
> would be a mouthful.

Yeah, the combinations are somewhat awkward. However, I would expect the
temporarily exclusive resets to be required in most cases, so that would
at least make the name a little shorter.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)