linux-kernel - Re: [PATCH 7/9] PM: Add suspend blocking work.

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, 24 Apr 2010 00:21:58 -0700
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dmitri Vorobiev <dmitri.vorobiev@...ial.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 7/9] PM: Add suspend blocking work.

2010/4/23 Tejun Heo <tj@...nel.org>:
> Hello,
>
> On 04/24/2010 12:49 AM, Arve Hjønnevåg wrote:
>> I want the suspend blocker active when the work is pending or running.
>> I did not see a way to do this on top of the workqueue api without
>> adding additional locking.
>
> Well, then add additional locking.  suspend_blocker is far away from
> being a hot path and there's nothing wrong with additional locking as
> long as everything is wrapped inside proper APIs.  Adding stuff to the
> core code for things as specific as this is much worse.
>

OK, I'll try to do this. Do you want it in a separate header file as well?

>> If the work is both queued and starts running on another workqueue
>> between "get_wq_data(work) == cwq" and "!work_pending(work)", then
>> suspend_unblock will be called when it shouldn't. It should work fine
>> if I change to it check pending first though, since it cannot move
>> back to the current workqueue without locking cwq->lock first.
>
> The code is more fundamentally broken.  Once work->func has started
> executing, the work pointer can't be dereferenced as work callback is
> allowed to free or re-purpose the work data structure and in the same
> manner you can't check for pending status after execution has
> completed.
>

I only touch the work structure after the callback has returned for
suspend blocking work, which does not allow that.

>> Or are you talking about the race when the callback is running on
>> multiple (cpu) workqueues at the same time. In that case the suspend
>> blocker is released when the callback returns from the last workqueue
>> is was queued on, not when all the callbacks have returned. On that
>> note, is it ever safe to use flush_work and cancel_work_sync for work
>> queues on anything other than a single single threaded workqueue?
>
> flush_work() is guaranteed only to flush from the last queued cpu but
> cancel_work_sync() will guarantee that no cpu is executing or holding
> onto the work.  So, yeah, as long as the limitations of flush_work()
> is understood, it's safe.
>

Sorry, I didn't see the for_each_cpu(cpu, cpu_map) part of
wait_on_work(). cancel_work_sync() does look safe as long as the work
has not moved to completely different workqueue.

> Going back to the original subject, just add simplistic outside
> locking in suspend_blocker_work API (or whatever other name you
> prefer).  That will be much cleaner and safer.  Let's think about
> moving them into workqueue implementation proper after the number of
> the API users grows to hundreds.
>

OK.

-- 
Arve Hjønnevåg
--
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/