Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

On Thu, Jul 19, 2012 at 2:15 PM, Tejun Heo [off-list ref] wrote:

Hello,

kthread_worker was introduced together with concurrency managed
workqueue to serve workqueue users which need a special dedicated
worker - e.g. RT scheduling.  This is minimal queue / flush / flush
all iterface on top of kthread and each provided interface matches the
workqueue counterpart so that switching isn't difficult.

However, one noticeable difference was that kthread_worker doesn't
allow a work item to be freed while being executed.  The intention was
to keep the code simpler but it didn't really and the restriction is
subtle and does prevent some valid use cases.

This two-patch series reimplements flush_kthread_work() so that it
uses an extra work item for flushing.  While this takes a bit more
lines, this is easier to understand and removes the annoying
difference.

This patchset contains the following two patches.

 0001-kthread_worker-reorganize-to-prepare-for-flush_kthre.patch
 0002-kthread_worker-reimplement-flush_kthread_work-to-all.patch

The first one is a prep patch which makes no functional changes.  The
second reimplements flush_kthread_work().

All current kthread_worker users are cc'd.  If no one objects, I'll
push it through the workqueue branch.  This patchset is also available
in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-kthread_worker-flush

diffstat follows.  Thanks.

 include/linux/kthread.h |    8 +---
 kernel/kthread.c        |   86 +++++++++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 42 deletions(-)

--
tejun

This patch set fixes a reproducible crash I'm seeing on a 3.4.10
kernel.  flush_kthread_worker (which is different from
flush_kthread_work) is initializing a kthread_work and a completion on
the stack, then queuing it and calling wait_for_completion.  Once the
completion is signaled, flush_kthread_worker exits and the stack
region used by the kthread_work may be immediately reused by another
object on the stack, but kthread_worker_fn continues accessing its
work pointer:
                work->func(work);         <- calls complete,
effectively frees work
                smp_wmb();      /* wmb worker-b0 paired with flush-b1 */
                work->done_seq = work->queue_seq;   <- overwrites a
new stack object
                smp_mb();       /* mb worker-b1 paired with flush-b0 */
                if (atomic_read(&work->flushing))
                        wake_up_all(&work->done);  <- or crashes here

These patches fix the problem by not accessing work after work->func
is called, and should be backported to stable.  They apply cleanly to
3.4.10.  Upstream commits are 9a2e03d8ed518a61154f18d83d6466628e519f94
and 46f3d976213452350f9d10b0c2780c2681f7075b.