linux-kernel - Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization

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: <10138182.WjU9yqx6Gs@vostro.rjw.lan>
Date:	Tue, 08 Dec 2015 15:19:31 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	linux-pm@...r.kernel.org, linaro-kernel@...ts.linaro.org,
	ashwin.chaugule@...aro.org,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization

On Tuesday, December 08, 2015 07:06:33 PM Viresh Kumar wrote:
> On 08-12-15, 14:30, Rafael J. Wysocki wrote:
> > OK, but instead of relying on the spinlock to wait for the already running
> 
> That's the purpose of the spinlock, not a side-effect.
> 
> > dbs_timer_handler() in gov_cancel_work() (which is really easy to overlook
> > and should at least be mentioned in a comment) we can wait for it explicitly.
> 
> I agree, and I will add explicit comment about it.
> 
> > That is, if the relevant code in gov_cancel_work() is like this:
> > 
> > 
> > 	atomic_inc(&shared->skip_work);
> > 	gov_cancel_timers(shared->policy);
> > 	cancel_work_sync(&shared->work);
> > 	gov_cancel_timers(shared->policy);
> 
> Apart from it being *really* ugly (we should know exactly what should
> be done, it rather looks like hit and try),

We know what should be done.  We need to wait for the timer function to
complete, then cancel the work item spawned by it (if any) and then
cancel the timers set by that work item.

> it is still racy.
> 
> > 	atomic_set(&shared->skip_work, 0);
> > 
> > then the work item should not be leaked behind the cancel_work_sync() any more
> > AFAICS.
> 
> Suppose queue_work() isn't done within the spin lock.
> 
> CPU0                                            CPU1
> 
> cpufreq_governor_stop()                         dbs_timer_handler()
> -> gov_cancel_work()                            -> lock
>                                                 -> shared->skip_work++, as skip_work was 0. //skip_work=1
>                                                 -> unlock
>    -> lock
>    -> shared->skip_work++; //skip_work=2

(*)

>    -> unlock
>                                                 -> queue_work();
>    -> gov_cancel_timers(shared->policy);
>                                                 dbs_work_handler();
>                                                 -> queue-timers again (as we aren't checking skip_work here)

skip_work = 1 (because dbs_work_handler() decrements it).

>    -> cancel_work_sync(&shared->work);
>                                                 dbs_timer_handler()
>                                                 -> lock
>                                                 -> shared->skip_work++, as skip_work was 0.

No, it wasn't 0, it was 1, because (*) incremented it
and it has only been decremented once by dbs_work_handler().

> //skip_work=1
>                                                 -> unlock

And the below won't happen.

>                                                 ->queue_work()
>
>    -> gov_cancel_timers(shared->policy);
>    -> shared->skip_work = 0;
> 
> 
> And we have the same situation again.

I don't really think so.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/