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

[Oleg Nesterov <oleg@redhat.com>]

On 02/16, Dmitry Torokhov wrote:
>
> On Tue, Feb 16, 2010 at 07:37:49PM +0100, samu.p.onkalo@nokia.com wrote:
> >
> > 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.