On Thu, Oct 25, 2018 at 09:59:38PM +0200, Johannes Berg wrote:
> > So, thinking about this more, can you guarantee (somehow) that the
> > workqueue is empty at this point?
>
> (I hadn't looked at the code then - obviously that's guaranteed)
We can guarantee it from someone who is looking at the code path. In
dio_set_defer_completion:
if (!sb->s_dio_done_wq)
return sb_init_dio_done_wq(sb);
And then sb_init_dio_done_wq:
int sb_init_dio_done_wq(struct super_block *sb)
{
struct workqueue_struct *old;
struct workqueue_struct *wq = alloc_workqueue("dio/%s",
WQ_MEM_RECLAIM, 0,
sb->s_id);
if (!wq)
return -ENOMEM;
/*
* This has to be atomic as more DIOs can race to create the workqueue
*/
old = cmpxchg(&sb->s_dio_done_wq, NULL, wq);
/* Someone created workqueue before us? Free ours... */
if (old)
destroy_workqueue(wq);
return 0;
}
The race found in the syzbot reproducer has multiple threads all
running DIO writes at the same time. So we have multiple threads
calling sb_init_dio_done_wq, but all but one will lose the race, and
then call destry_workqueue on the freshly created (but never used)
workqueue.
We could replace the destroy_workqueue(wq) with a
"I_solemnly_swear_this_workqueue_has_never_been_used_please_destroy(wq)".
Or, as Tejun suggested, "destroy_workqueue_skip_drain(wq)", but there is
no way for the workqueue code to know whether the caller was using the
interface correctly. So this basically becomes a philosophical
question about whether or not we trust the caller to be correct or
not.
I don't see an obvious way that we can test to make sure the workqueue
is never used without actually taking a performance. Am I correct
that we would need to take the wq->mutex before we can mess with the
wq->flags field?
- Ted
