linux-kernel - Re: complete_all and "forever" completions

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: <20161026154213.GD3117@twins.programming.kicks-ass.net>
Date:   Wed, 26 Oct 2016 17:42:13 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, Tejun Heo <tj@...nel.org>,
        computersforpeace@...il.com, Ingo Molnar <mingo@...nel.org>,
        der.herr@...r.at
Subject: Re: complete_all and "forever" completions

On Wed, Oct 26, 2016 at 05:10:01AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 26, 2016 at 10:45:35AM +0200, Peter Zijlstra wrote:
> > On Tue, Oct 25, 2016 at 03:30:54PM -0700, Dmitry Torokhov wrote:

> > > Or do we need something like this in
> > > do_wait_for_common():
> > > 
> > > 	if (x->done < UINT_MAX/2)
> > > 		x->done--;
> > 
> > Depends a bit, do you really want this? Seems a bit daft to keep asking
> > if its done already, seems like a waste of cycles to me.
> > 
> 
> The use case I am after is:
> 
> 1. There is a device that is extremely dumb without firmware
> 2. The driver uses request_firmware_nowait() and signals completion from
> the firmware loading callback to let the reset of the driver know that
> firmware has been done loading (successfully or otherwise)
> 3. The driver uses wait_for_completion() in both remove() and suspend()
> methods to wait for the firmware to finish loading.
> 
> While remove() happens at most once per device instance, suspend() may
> happen unbound number of times (theoretically).
> 
> So the question is: should complete_all have this "forever" semantic
> (IOW is documentation right about the intent) or do we need a new
> primitive for this? From the cursory glance of users of complete_all()
> all of them expect completion to stay in signalled state either forever,
> or until they call reinit_completion() explicitly.

Nah, if we need this we should fix this one. Adding similar but slightly
different primitives is a pain.

But I think you might need slightly more than the proposed change, the
case I worry about is doing complete_all() when done != 0 (which isn't
all that strange).


Does something like so work?

---
 kernel/sched/completion.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index 8d0f35debf35..5deab9c789df 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -51,7 +51,7 @@ void complete_all(struct completion *x)
 	unsigned long flags;
 
 	spin_lock_irqsave(&x->wait.lock, flags);
-	x->done += UINT_MAX/2;
+	x->done = UINT_MAX/2;
 	__wake_up_locked(&x->wait, TASK_NORMAL, 0);
 	spin_unlock_irqrestore(&x->wait.lock, flags);
 }
@@ -79,7 +79,10 @@ do_wait_for_common(struct completion *x,
 		if (!x->done)
 			return timeout;
 	}
-	x->done--;
+
+	if (x->done != UINT_MAX/2)
+		x->done--;
+
 	return timeout ?: 1;
 }