Re: [PATCH] input: polldev can cause crash in case of polling disabled

On 02/16, Dmitry Torokhov wrote:

On Tue, Feb 16, 2010 at 07:37:49PM +0100, samu.p.onkalo@nokia.com wrote:

quoted

Queue_delayed_work updates the work struct. Workqueue itself is ok.

I think that the sequence goes about this way (no other polled devices open):
1. Polled device is opened with polling enabled
2. It first creates workqueue and then queue the first polling. Kernels
Workqueue functions updates current workqueue information to the work-struct
3. polled device is closed
4. workqueue is destroyed

5. polling interval is set to 0
6. device is reopened
7. New workqueue is created
8. polled device is closed without queueing a work
9. work struct for polled device contains pointer to the old (created in 2.) wq
10. cancel_workqueue... can access unallocated memory causing crash.

Ah, I see. In this case I think it should be fixed in workqueue code by
clearing work data so it does not point to the [potentially] non-existing
workqueue when we cancel

I think yes, cancel_() can clear ->data. Instead of work_clear_pending()
__cancel_work_timer() should do something like
"work->data &= ~WORK_STRUCT_STATIC", but we should somehow do this
atomically, to avoid the race with test_and_set_bit(PENDING).

or complete work.

No, run_workqueue() can't do this.

First of all, we shouldn't clear work->data before work->func() returns,
and when it returns we must not use this pointer: the work can be already
freed/reused.

But even if we could do this, it would be wrong. This can break flush
or cancel, the same work can be queued/running again (on the same or
other CPUs).

Oleg.