Mailing List Archive

Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution
On Thu, Jul 19, 2012 at 2:15 PM, Tejun Heo <tj@kernel.org> 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.

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution [ In reply to ]
On Mon, Sep 17, 2012 at 12:40:16PM -0700, Tejun Heo wrote:
> On Fri, Sep 14, 2012 at 03:50:40PM -0700, Colin Cross wrote:
> > 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.
>
> Yeah, you're right. I wonder why this didn't come up before. Greg,
> can you please pick up these two commits?

Ok, will do, thanks for letting me know.

greg k-h

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution [ In reply to ]
On Mon, Sep 17, 2012 at 01:28:50PM -0700, Greg KH wrote:
> On Mon, Sep 17, 2012 at 12:40:16PM -0700, Tejun Heo wrote:
> > On Fri, Sep 14, 2012 at 03:50:40PM -0700, Colin Cross wrote:
> > > 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.
> >
> > Yeah, you're right. I wonder why this didn't come up before. Greg,
> > can you please pick up these two commits?
>
> Ok, will do, thanks for letting me know.

Now applied, thanks.

greg k-h

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel