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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
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);
disable_irq(drvdata->irq);
}
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
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 <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> 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
--
Dmitry
On Thu, Feb 7, 2019 at 5:27 PM Dmitry Torokhov
<[email protected]> 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
> <[email protected]> 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.
--
Dmitry
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
>> <[email protected]> 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 <[email protected]>
On Fri, Feb 8, 2019 at 10:51 AM Danilo Krummrich
<[email protected]> 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 <[email protected]>