Input: ps2-gpio - flush TX work when closing port
diff mbox series

Message ID 20190207222740.GA38612@dtor-ws
State New
Headers show
Series
  • Input: ps2-gpio - flush TX work when closing port
Related show

Commit Message

dmitry.torokhov@gmail.com Feb. 7, 2019, 10:27 p.m. UTC
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(+)

Comments

dmitry.torokhov@gmail.com Feb. 7, 2019, 10:31 p.m. UTC | #1
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
Sven Van Asbroeck Feb. 7, 2019, 11:03 p.m. UTC | #2
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() ?
dmitry.torokhov@gmail.com Feb. 8, 2019, 7:31 a.m. UTC | #3
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.
Danilo Krummrich Feb. 8, 2019, 3:51 p.m. UTC | #4
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>
Sven Van Asbroeck Feb. 8, 2019, 4:21 p.m. UTC | #5
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>

Patch
diff mbox series

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);
 }