linux-kernel - Re: [PATCH 1/3] input: do not use tasklet_disable before tasklet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20121125075846.GA21091@core.coreip.homeip.net>
Date:	Sat, 24 Nov 2012 23:58:46 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Xiaotian Feng <xtfeng@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Xiaotian Feng <dannyfeng@...cent.com>,
	Tony Lindgren <tony@...mide.com>,
	Sourav Poddar <sourav.poddar@...com>,
	Josh <joshua.taylor0@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-input@...r.kernel.org
Subject: Re: [PATCH 1/3] input: do not use tasklet_disable before tasklet_kill

Hi Xiaotian,

On Wed, Oct 31, 2012 at 04:05:59PM +0800, Xiaotian Feng wrote:
> If tasklet_disable() is called before related tasklet handled,
> tasklet_kill will never be finished. tasklet_kill is enough.
> 

Could you please elaborate on this? Needing to disable tasket before
killing it is quite often needed when dealing with self-rescheduling
tasklets and so tasklet_disable() followed by tasklet_kill() must work.
If it does not we need to take care of it in softirq code instead of
individual drivers.

> Signed-off-by: Xiaotian Feng <dannyfeng@...cent.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@...il.com> 
> Cc: Tony Lindgren <tony@...mide.com>
> Cc: Sourav Poddar <sourav.poddar@...com>
> Cc: Josh <joshua.taylor0@...il.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-input@...r.kernel.org
> ---
>  drivers/input/keyboard/omap-keypad.c |    3 +--
>  drivers/input/serio/hil_mlc.c        |    1 -
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap-keypad.c b/drivers/input/keyboard/omap-keypad.c
> index 4a5fcc8..6c52447 100644
> --- a/drivers/input/keyboard/omap-keypad.c
> +++ b/drivers/input/keyboard/omap-keypad.c
> @@ -362,12 +362,11 @@ static int __devexit omap_kp_remove(struct platform_device *pdev)
>  	struct omap_kp *omap_kp = platform_get_drvdata(pdev);
>  
>  	/* disable keypad interrupt handling */
> -	tasklet_disable(&kp_tasklet);
> +	tasklet_kill(&kp_tasklet);
>  	omap_writew(1, OMAP1_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
>  	free_irq(omap_kp->irq, omap_kp);
>  
>  	del_timer_sync(&omap_kp->timer);
> -	tasklet_kill(&kp_tasklet);

Exactly like here. If we do not disable tasklet before disabling IRQ and
freeing timer we may get into scenario when timer schedules tasket and
tasklet schedules timer again after we canceled it.

>  
>  	/* unregister everything */
>  	input_unregister_device(omap_kp->input);
> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
> index bfd3865..7fc1700 100644
> --- a/drivers/input/serio/hil_mlc.c
> +++ b/drivers/input/serio/hil_mlc.c
> @@ -1011,7 +1011,6 @@ static void __exit hil_mlc_exit(void)
>  {
>  	del_timer_sync(&hil_mlcs_kicker);
>  
> -	tasklet_disable(&hil_mlcs_tasklet);
>  	tasklet_kill(&hil_mlcs_tasklet);

This seems like safe change.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/