| Message ID | 20190207222740.GA38612@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: ps2-gpio - flush TX work when closing port | expand |
On Thu, Feb 07, 2019 at 02:27:40PM -0800, Dmitry Torokhov wrote: > To ensure that TX work is not running after serio port has been torn down, > let's flush it when closing the port. > > Reported-by: Sven Van Asbroeck <thesven73@gmail.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/serio/ps2-gpio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c > index c62cceb97bb1..9e1dbde2e15b 100644 > --- a/drivers/input/serio/ps2-gpio.c > +++ b/drivers/input/serio/ps2-gpio.c > @@ -76,6 +76,7 @@ static void ps2_gpio_close(struct serio *serio) > { > struct ps2_gpio_data *drvdata = serio->port_data; > > + flush_work(&drvdata->tx_work.work); Ah, we have flush_delayed_work() now, I'll change it before committing once we agree on the patch in principle. > disable_irq(drvdata->irq); > } > > -- > 2.20.1.611.gfbb209baf1-goog > > > -- > Dmitry
On Thu, Feb 7, 2019 at 5:27 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > + flush_work(&drvdata->tx_work.work); Would cancel_work_sync() be better than flush_work() ?
On Thu, Feb 07, 2019 at 06:03:03PM -0500, Sven Van Asbroeck wrote: > On Thu, Feb 7, 2019 at 5:27 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > + flush_work(&drvdata->tx_work.work); > > Would cancel_work_sync() be better than flush_work() ? No, because we want to have interrupt and gpios in a consistent state. If we cancel then we need to see if we should disable it or it may already be disabled, etc. This way we know it is enabled after flush_delayed_work() returns, and we need to disable it. Thanks.
On 2019-02-08 08:31, Dmitry Torokhov wrote: > On Thu, Feb 07, 2019 at 06:03:03PM -0500, Sven Van Asbroeck wrote: >> On Thu, Feb 7, 2019 at 5:27 PM Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > >> > + flush_work(&drvdata->tx_work.work); >> >> Would cancel_work_sync() be better than flush_work() ? > > No, because we want to have interrupt and gpios in a consistent state. > If we cancel then we need to see if we should disable it or it may > already be disabled, etc. This way we know it is enabled after > flush_delayed_work() returns, and we need to disable it. > > Thanks. I agree with Dmitry - thanks for the fix. Acked-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
On Fri, Feb 8, 2019 at 10:51 AM Danilo Krummrich <danilokrummrich@dk-develop.de> wrote: > > I agree with Dmitry > So do I, you guys are absolutely right. As far as I can see, this patch fixes the user-after-free. So, after Dmitry changes flush_work() to flush_delayed_work() : Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c index c62cceb97bb1..9e1dbde2e15b 100644 --- a/drivers/input/serio/ps2-gpio.c +++ b/drivers/input/serio/ps2-gpio.c @@ -76,6 +76,7 @@ static void ps2_gpio_close(struct serio *serio) { struct ps2_gpio_data *drvdata = serio->port_data; + flush_work(&drvdata->tx_work.work); disable_irq(drvdata->irq); }
To ensure that TX work is not running after serio port has been torn down, let's flush it when closing the port. Reported-by: Sven Van Asbroeck <thesven73@gmail.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/serio/ps2-gpio.c | 1 + 1 file changed, 1 insertion(+)