Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932355AbWI0OiI (ORCPT ); Wed, 27 Sep 2006 10:38:08 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S932360AbWI0OiH (ORCPT ); Wed, 27 Sep 2006 10:38:07 -0400 Received: from ns2.suse.de ([195.135.220.15]:41357 "EHLO mx2.suse.de") by vger.kernel.org with ESMTP id S932355AbWI0OiG (ORCPT ); Wed, 27 Sep 2006 10:38:06 -0400 Date: Wed, 27 Sep 2006 16:38:04 +0200 Message-ID: From: Takashi Iwai To: Jean-Marc Saffroy Cc: linux-kernel@vger.kernel.org Subject: Re: oops in :snd_pcm_oss:resample_expand+0x19c/0x1f0 In-Reply-To: References: User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta25) (eggplant) (+CVS-20060326) (i386-suse-linux) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8236 Lines: 231 At Sun, 24 Sep 2006 19:01:27 +0200 (CEST), Jean-Marc Saffroy wrote: > > The kernel is vanilla 2.6.18 on Debian etch, the box is a dual core Athlon > in x86-64 mode (in case you didn't notice the long pointers ;-). The > offending process is a chroot'ed IA32 Firefox. At first I suspected a > problem with 32-bit compatibility, but stack traces below and source code > suggest the problem could be different: > > - CPU #0 crashes in resample_expand() at line 116: > > 116 *dst = val; > > The target address is at ffffc200100e4466, which looks like a vmalloc'ed > buffer (according to Documentation/x86_64/mm.txt). > > - CPU #1 is somewhere down vfree() called by snd_pcm_oss_change_params(), > at line 1010: > > 1010 vfree(runtime->oss.buffer); > 1011 runtime->oss.buffer = vmalloc(runtime->oss.period_bytes); > > Now it really looks like it *could* be a race between two threads using > the same device, maybe the same buffers somehow, but I'm getting lost in > the data structures, so help from knowledgeable people would be welcome. Thanks for the detailed analysis. It looks indeed like a race between two threads that we didn't consider much for OSS emulation (although ALSA parts are coded to be thread-safe). Could you try the patch below? It simply adds mutex around the parts handling buffers concurrently. Takashi diff -r 7cc57ec28195 core/oss/pcm_oss.c --- a/core/oss/pcm_oss.c Tue Sep 26 15:32:35 2006 +0200 +++ b/core/oss/pcm_oss.c Wed Sep 27 16:29:36 2006 +0200 @@ -810,6 +810,8 @@ static int snd_pcm_oss_change_params(str struct snd_mask sformat_mask; struct snd_mask mask; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -EINTR; sw_params = kmalloc(sizeof(*sw_params), GFP_KERNEL); params = kmalloc(sizeof(*params), GFP_KERNEL); sparams = kmalloc(sizeof(*sparams), GFP_KERNEL); @@ -1020,6 +1022,7 @@ failure: kfree(sw_params); kfree(params); kfree(sparams); + mutex_unlock(&runtime->oss.params_lock); return err; } @@ -1307,14 +1310,17 @@ static ssize_t snd_pcm_oss_write1(struct if ((tmp = snd_pcm_oss_make_ready(substream)) < 0) return tmp; + mutex_lock(&runtime->oss.params_lock); while (bytes > 0) { if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) { tmp = bytes; if (tmp + runtime->oss.buffer_used > runtime->oss.period_bytes) tmp = runtime->oss.period_bytes - runtime->oss.buffer_used; if (tmp > 0) { - if (copy_from_user(runtime->oss.buffer + runtime->oss.buffer_used, buf, tmp)) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : -EFAULT; + if (copy_from_user(runtime->oss.buffer + runtime->oss.buffer_used, buf, tmp)) { + tmp = -EFAULT; + goto err; + } } runtime->oss.buffer_used += tmp; buf += tmp; @@ -1325,22 +1331,24 @@ static ssize_t snd_pcm_oss_write1(struct tmp = snd_pcm_oss_write2(substream, runtime->oss.buffer + runtime->oss.period_ptr, runtime->oss.buffer_used - runtime->oss.period_ptr, 1); if (tmp <= 0) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; + goto err; runtime->oss.bytes += tmp; runtime->oss.period_ptr += tmp; runtime->oss.period_ptr %= runtime->oss.period_bytes; if (runtime->oss.period_ptr == 0 || runtime->oss.period_ptr == runtime->oss.buffer_used) runtime->oss.buffer_used = 0; - else if ((substream->f_flags & O_NONBLOCK) != 0) - return xfer > 0 ? xfer : -EAGAIN; + else if ((substream->f_flags & O_NONBLOCK) != 0) { + tmp = -EAGAIN; + goto err; + } } } else { tmp = snd_pcm_oss_write2(substream, (const char __force *)buf, runtime->oss.period_bytes, 0); if (tmp <= 0) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; + goto err; runtime->oss.bytes += tmp; buf += tmp; bytes -= tmp; @@ -1350,7 +1358,12 @@ static ssize_t snd_pcm_oss_write1(struct break; } } + mutex_unlock(&runtime->oss.params_lock); return xfer; + + err: + mutex_unlock(&runtime->oss.params_lock); + return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; } static ssize_t snd_pcm_oss_read2(struct snd_pcm_substream *substream, char *buf, size_t bytes, int in_kernel) @@ -1397,12 +1410,13 @@ static ssize_t snd_pcm_oss_read1(struct if ((tmp = snd_pcm_oss_make_ready(substream)) < 0) return tmp; + mutex_lock(&runtime->oss.params_lock); while (bytes > 0) { if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) { if (runtime->oss.buffer_used == 0) { tmp = snd_pcm_oss_read2(substream, runtime->oss.buffer, runtime->oss.period_bytes, 1); if (tmp <= 0) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; + goto err; runtime->oss.bytes += tmp; runtime->oss.period_ptr = tmp; runtime->oss.buffer_used = tmp; @@ -1410,8 +1424,10 @@ static ssize_t snd_pcm_oss_read1(struct tmp = bytes; if ((size_t) tmp > runtime->oss.buffer_used) tmp = runtime->oss.buffer_used; - if (copy_to_user(buf, runtime->oss.buffer + (runtime->oss.period_ptr - runtime->oss.buffer_used), tmp)) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : -EFAULT; + if (copy_to_user(buf, runtime->oss.buffer + (runtime->oss.period_ptr - runtime->oss.buffer_used), tmp)) { + tmp = -EFAULT; + goto err; + } buf += tmp; bytes -= tmp; xfer += tmp; @@ -1420,14 +1436,19 @@ static ssize_t snd_pcm_oss_read1(struct tmp = snd_pcm_oss_read2(substream, (char __force *)buf, runtime->oss.period_bytes, 0); if (tmp <= 0) - return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; + goto err; runtime->oss.bytes += tmp; buf += tmp; bytes -= tmp; xfer += tmp; } } + mutex_unlock(&runtime->oss.params_lock); return xfer; + + err: + mutex_unlock(&runtime->oss.params_lock); + return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; } static int snd_pcm_oss_reset(struct snd_pcm_oss_file *pcm_oss_file) @@ -1528,6 +1549,7 @@ static int snd_pcm_oss_sync(struct snd_p return err; format = snd_pcm_oss_format_from(runtime->oss.format); width = snd_pcm_format_physical_width(format); + mutex_lock(&runtime->oss.params_lock); if (runtime->oss.buffer_used > 0) { #ifdef OSS_DEBUG printk("sync: buffer_used\n"); @@ -1537,8 +1559,10 @@ static int snd_pcm_oss_sync(struct snd_p runtime->oss.buffer + runtime->oss.buffer_used, size); err = snd_pcm_oss_sync1(substream, runtime->oss.period_bytes); - if (err < 0) + if (err < 0) { + mutex_unlock(&runtime->oss.params_lock); return err; + } } else if (runtime->oss.period_ptr > 0) { #ifdef OSS_DEBUG printk("sync: period_ptr\n"); @@ -1548,8 +1572,10 @@ static int snd_pcm_oss_sync(struct snd_p runtime->oss.buffer, size * 8 / width); err = snd_pcm_oss_sync1(substream, size); - if (err < 0) + if (err < 0) { + mutex_unlock(&runtime->oss.params_lock); return err; + } } /* * The ALSA's period might be a bit large than OSS one. @@ -1579,6 +1605,7 @@ static int snd_pcm_oss_sync(struct snd_p snd_pcm_lib_writev(substream, buffers, size); } } + mutex_unlock(&runtime->oss.params_lock); /* * finish sync: drain the buffer */ @@ -2172,6 +2199,7 @@ static void snd_pcm_oss_init_substream(s runtime->oss.params = 1; runtime->oss.trigger = 1; runtime->oss.rate = 8000; + mutex_init(&runtime->oss.params_lock); switch (SNDRV_MINOR_OSS_DEVICE(minor)) { case SNDRV_MINOR_OSS_PCM_8: runtime->oss.format = AFMT_U8; diff -r 7cc57ec28195 include/pcm_oss.h --- a/include/pcm_oss.h Tue Sep 26 15:32:35 2006 +0200 +++ b/include/pcm_oss.h Wed Sep 27 16:28:28 2006 +0200 @@ -56,6 +56,7 @@ struct snd_pcm_oss_runtime { size_t mmap_bytes; char *buffer; /* vmallocated period */ size_t buffer_used; /* used length from period buffer */ + struct mutex params_lock; #ifdef CONFIG_SND_PCM_OSS_PLUGINS struct snd_pcm_plugin *plugin_first; struct snd_pcm_plugin *plugin_last; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/