diff mbox series

ALSA: Use del_timer_sync() before freeing timer

Message ID	20221026231236.6834b551@gandalf.local.home
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Date: Wed, 26 Oct 2022 23:12:36 -0400 From: Steven Rostedt <rostedt@goodmis.org> To: LKML <linux-kernel@vger.kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org>, Thomas Gleixner <tglx@linutronix.de>, Guenter Roeck <linux@roeck-us.net>, Stephen Boyd <sboyd@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org Subject: [PATCH] ALSA: Use del_timer_sync() before freeing timer Message-ID: <20221026231236.6834b551@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk
Series	ALSA: Use del_timer_sync() before freeing timer \| ALSA: Use del_timer_sync() before freeing timer

Commit Message

Steven Rostedt Oct. 27, 2022, 3:12 a.m. UTC

  From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The current code for freeing the emux timer is extremely dangerous:

  CPU0				CPU1
  ----				----
snd_emux_timer_callback()
			    snd_emux_free()
			      spin_lock(&emu->voice_lock)
			      del_timer(&emu->tlist); <-- returns immediately
			      spin_unlock(&emu->voice_lock);
			      [..]
			      kfree(emu);

  spin_lock(&emu->voice_lock);

 [BOOM!]

Instead just use del_timer_sync() which will wait for the timer to finish
before continuing. No need to check if the timer is active or not when
doing so.

This doesn't fix the race of a possible re-arming of the timer, but at
least it won't use the data that has just been freed.

Cc: stable@vger.kernel.org
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 sound/synth/emux/emux.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Guenter Roeck Oct. 27, 2022, 4:04 a.m. UTC | #1

On 10/26/22 20:12, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The current code for freeing the emux timer is extremely dangerous:
> 
>    CPU0				CPU1
>    ----				----
> snd_emux_timer_callback()
> 			    snd_emux_free()
> 			      spin_lock(&emu->voice_lock)
> 			      del_timer(&emu->tlist); <-- returns immediately
> 			      spin_unlock(&emu->voice_lock);
> 			      [..]
> 			      kfree(emu);
> 
>    spin_lock(&emu->voice_lock);
> 
>   [BOOM!]
> 
> Instead just use del_timer_sync() which will wait for the timer to finish
> before continuing. No need to check if the timer is active or not when
> doing so.
> 
> This doesn't fix the race of a possible re-arming of the timer, but at
> least it won't use the data that has just been freed.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   sound/synth/emux/emux.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
> index 5ed8e36d2e04..a2ee78809cfb 100644
> --- a/sound/synth/emux/emux.c
> +++ b/sound/synth/emux/emux.c
> @@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu)
>   	if (! emu)
>   		return -EINVAL;
>   
> -	spin_lock_irqsave(&emu->voice_lock, flags);
> -	if (emu->timer_active)
> -		del_timer(&emu->tlist);
> -	spin_unlock_irqrestore(&emu->voice_lock, flags);
> +	del_timer_sync(&emu->tlist);
>   
>   	snd_emux_proc_free(emu);
>   	snd_emux_delete_virmidi(emu);

kernel test robot Oct. 27, 2022, 5:58 a.m. UTC | #2

Hi Steven,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tiwai-sound/for-next]
[also build test WARNING on linux/master linus/master v6.1-rc2 next-20221027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Steven-Rostedt/ALSA-Use-del_timer_sync-before-freeing-timer/20221027-111423
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20221026231236.6834b551%40gandalf.local.home
patch subject: [PATCH] ALSA: Use del_timer_sync() before freeing timer
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/998e4b5d9793fb6deb720110f9038ed04e822603
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Steven-Rostedt/ALSA-Use-del_timer_sync-before-freeing-timer/20221027-111423
        git checkout 998e4b5d9793fb6deb720110f9038ed04e822603
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash sound/synth/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   sound/synth/emux/emux.c: In function 'snd_emux_free':
>> sound/synth/emux/emux.c:129:23: warning: unused variable 'flags' [-Wunused-variable]
     129 |         unsigned long flags;
         |                       ^~~~~


vim +/flags +129 sound/synth/emux/emux.c

^1da177e4c3f41 Linus Torvalds          2005-04-16  124  
^1da177e4c3f41 Linus Torvalds          2005-04-16  125  /*
^1da177e4c3f41 Linus Torvalds          2005-04-16  126   */
03da312ac080b4 Takashi Iwai            2005-11-17  127  int snd_emux_free(struct snd_emux *emu)
^1da177e4c3f41 Linus Torvalds          2005-04-16  128  {
^1da177e4c3f41 Linus Torvalds          2005-04-16 @129  	unsigned long flags;
^1da177e4c3f41 Linus Torvalds          2005-04-16  130  
^1da177e4c3f41 Linus Torvalds          2005-04-16  131  	if (! emu)
^1da177e4c3f41 Linus Torvalds          2005-04-16  132  		return -EINVAL;
^1da177e4c3f41 Linus Torvalds          2005-04-16  133  
998e4b5d9793fb Steven Rostedt (Google  2022-10-26  134) 	del_timer_sync(&emu->tlist);
^1da177e4c3f41 Linus Torvalds          2005-04-16  135  
^1da177e4c3f41 Linus Torvalds          2005-04-16  136  	snd_emux_proc_free(emu);
^1da177e4c3f41 Linus Torvalds          2005-04-16  137  	snd_emux_delete_virmidi(emu);
3d774d5ef06697 Takashi Iwai            2017-06-09  138  #if IS_ENABLED(CONFIG_SND_SEQUENCER_OSS)
^1da177e4c3f41 Linus Torvalds          2005-04-16  139  	snd_emux_detach_seq_oss(emu);
^1da177e4c3f41 Linus Torvalds          2005-04-16  140  #endif
^1da177e4c3f41 Linus Torvalds          2005-04-16  141  	snd_emux_detach_seq(emu);
^1da177e4c3f41 Linus Torvalds          2005-04-16  142  	snd_emux_delete_hwdep(emu);
^1da177e4c3f41 Linus Torvalds          2005-04-16  143  	snd_sf_free(emu->sflist);
^1da177e4c3f41 Linus Torvalds          2005-04-16  144  	kfree(emu->voices);
^1da177e4c3f41 Linus Torvalds          2005-04-16  145  	kfree(emu->name);
^1da177e4c3f41 Linus Torvalds          2005-04-16  146  	kfree(emu);
^1da177e4c3f41 Linus Torvalds          2005-04-16  147  	return 0;
^1da177e4c3f41 Linus Torvalds          2005-04-16  148  }
^1da177e4c3f41 Linus Torvalds          2005-04-16  149

Takashi Iwai Oct. 27, 2022, 6:43 a.m. UTC | #3

On Thu, 27 Oct 2022 05:12:36 +0200,
Steven Rostedt wrote:
> 
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The current code for freeing the emux timer is extremely dangerous:
> 
>   CPU0				CPU1
>   ----				----
> snd_emux_timer_callback()
> 			    snd_emux_free()
> 			      spin_lock(&emu->voice_lock)
> 			      del_timer(&emu->tlist); <-- returns immediately
> 			      spin_unlock(&emu->voice_lock);
> 			      [..]
> 			      kfree(emu);
> 
>   spin_lock(&emu->voice_lock);
> 
>  [BOOM!]
> 
> Instead just use del_timer_sync() which will wait for the timer to finish
> before continuing. No need to check if the timer is active or not when
> doing so.
> 
> This doesn't fix the race of a possible re-arming of the timer, but at
> least it won't use the data that has just been freed.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Applied now with the fix of unused variable warning.


Thanks!

Takashi


> ---
>  sound/synth/emux/emux.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
> index 5ed8e36d2e04..a2ee78809cfb 100644
> --- a/sound/synth/emux/emux.c
> +++ b/sound/synth/emux/emux.c
> @@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu)
>  	if (! emu)
>  		return -EINVAL;
>  
> -	spin_lock_irqsave(&emu->voice_lock, flags);
> -	if (emu->timer_active)
> -		del_timer(&emu->tlist);
> -	spin_unlock_irqrestore(&emu->voice_lock, flags);
> +	del_timer_sync(&emu->tlist);
>  
>  	snd_emux_proc_free(emu);
>  	snd_emux_delete_virmidi(emu);
> -- 
> 2.35.1
>

Steven Rostedt Oct. 27, 2022, 1:37 p.m. UTC | #4

On Thu, 27 Oct 2022 08:43:32 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> Applied now with the fix of unused variable warning.

Thanks Takashi!

-- Steve

diff mbox series

Patch

diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
index 5ed8e36d2e04..a2ee78809cfb 100644
--- a/sound/synth/emux/emux.c
+++ b/sound/synth/emux/emux.c
@@ -131,10 +131,7 @@  int snd_emux_free(struct snd_emux *emu)
 	if (! emu)
 		return -EINVAL;
 
-	spin_lock_irqsave(&emu->voice_lock, flags);
-	if (emu->timer_active)
-		del_timer(&emu->tlist);
-	spin_unlock_irqrestore(&emu->voice_lock, flags);
+	del_timer_sync(&emu->tlist);
 
 	snd_emux_proc_free(emu);
 	snd_emux_delete_virmidi(emu);