some missing spin_unlocks

some missing spin_unlocks

[Ted Unangst]

I think these are all real bugs.

sound/synth/emux/emux_synth.c snd_emux_note_on, line 101
snd_assert will return without unlocking emu->voice_lock (line 89)

sound/pci/au88x0/au88x0_core.c vortex_adb_allocroute, search for EBUSY
returns without unlocking vortex->lock

net/rose/rose_route.c rose_route_frame, line 998
returns without unlocking rose_node_list_lock, rose_neigh_list_lock, or 
rose_route_list_lock

net/rose/rose_timer.c rose_heartbeat_expiry, line 141
rose_destroy_socket does not unlock sk as far as i can see

drivers/net/irda/donauboe.c toshoboe_net_ioctl, search for EPERM
returns without unlocking self->lock


-- 
Ted Unangst             www.coverity.com             Coverity, Inc.

Re: some missing spin_unlocks

[David S. Miller]

From: Ted Unangst <tedu@coverity.com>
Date: Mon, 22 Aug 2005 15:26:47 -0700

> net/rose/rose_route.c rose_route_frame, line 998
> returns without unlocking rose_node_list_lock, rose_neigh_list_lock, or 
> rose_route_list_lock

I fixed this one with the patch below.

> net/rose/rose_timer.c rose_heartbeat_expiry, line 141
> rose_destroy_socket does not unlock sk as far as i can see

This one needs more care.  We can't drop the lock, because
the destroy actions need to be protected by that lock, but
we can't release the lock after rose_destroy_socket() because
the object may not even exist any longer.

The problem there, at the core, is that the timer doesn't
grab a reference to the socket, which would make the solution
to this bug very straight forward.

Someone should work on that :-)

diff-tree 61ef36aa6cf356649863a24a850c2183cb762c61 (from daf53344fadaa8c47c6b0864e7f34efcbb66e391)
Author: David S. Miller <davem@davemloft.net>
Date:   Tue Aug 23 09:42:38 2005 -0700

    [ROSE]: Fix missing unlocks in rose_route_frame()
    
    Noticed by Coverity checker.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -994,8 +994,10 @@ int rose_route_frame(struct sk_buff *skb
 	 *	1. The frame isn't for us,
 	 *	2. It isn't "owned" by any existing route.
 	 */
-	if (frametype != ROSE_CALL_REQUEST)	/* XXX */
-		return 0;
+	if (frametype != ROSE_CALL_REQUEST) {	/* XXX */
+		ret = 0;
+		goto out;
+	}
 
 	len  = (((skb->data[3] >> 4) & 0x0F) + 1) / 2;
 	len += (((skb->data[3] >> 0) & 0x0F) + 1) / 2;

Re: some missing spin_unlocks

[Arjan van de Ven]

> This one needs more care.  We can't drop the lock, because
> the destroy actions need to be protected by that lock, but
> we can't release the lock after rose_destroy_socket() because
> the object may not even exist any longer.


does it matter? can ANYTHING be spinning on the lock? if not .. can we
just let the lock go poof and not unlock it... 

Re: some missing spin_unlocks

[David S. Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Tue, 23 Aug 2005 18:54:03 +0200

> does it matter? can ANYTHING be spinning on the lock? if not .. can we
> just let the lock go poof and not unlock it... 

I believe socket lookup can, otherwise the code is OK as-is.

Re: some missing spin_unlocks

[Arjan van de Ven]

On Tue, 2005-08-23 at 10:30 -0700, David S. Miller wrote:
> From: Arjan van de Ven <arjan@infradead.org>
> Date: Tue, 23 Aug 2005 18:54:03 +0200
> 
> > does it matter? can ANYTHING be spinning on the lock? if not .. can we
> > just let the lock go poof and not unlock it... 
> 
> I believe socket lookup can, otherwise the code is OK as-is.

lookup while the object is in progress of being destroyed sounds really
bad though....

Re: some missing spin_unlocks

[David S. Miller]

From: Arjan van de Ven <arjan@infradead.org>
Subject: Re: some missing spin_unlocks
Date: Tue, 23 Aug 2005 19:40:06 +0200

> On Tue, 2005-08-23 at 10:30 -0700, David S. Miller wrote:
> > From: Arjan van de Ven <arjan@infradead.org>
> > Date: Tue, 23 Aug 2005 18:54:03 +0200
> > 
> > > does it matter? can ANYTHING be spinning on the lock? if not .. can we
> > > just let the lock go poof and not unlock it... 
> > 
> > I believe socket lookup can, otherwise the code is OK as-is.
> 
> lookup while the object is in progress of being destroyed sounds really
> bad though....

This happens all the time with TCP sockets, for example.
When we're trying to kill off a socket which is in time
wait state, the receive path can find it, grab a reference,
and process a packet against it right as we're trying to
kill it off.

This is completely normal.

Re: some missing spin_unlocks

[Lee Revell]

[added alsa-devel to cc:]

On Mon, 2005-08-22 at 15:26 -0700, Ted Unangst wrote:
> I think these are all real bugs.
> 
> sound/synth/emux/emux_synth.c snd_emux_note_on, line 101
> snd_assert will return without unlocking emu->voice_lock (line 89)

This one is probably a real bug.

Lee

Re: some missing spin_unlocks

[Takashi Iwai]

At Wed, 24 Aug 2005 11:56:58 -0400,
Lee Revell wrote:
> 
> [added alsa-devel to cc:]
> 
> On Mon, 2005-08-22 at 15:26 -0700, Ted Unangst wrote:
> > I think these are all real bugs.
> > 
> > sound/synth/emux/emux_synth.c snd_emux_note_on, line 101
> > snd_assert will return without unlocking emu->voice_lock (line 89)
> 
> This one is probably a real bug.

The patch below fixes them (already applied to ALSA CVS tree).

================================================================

[PATCH] Fix missing spin_unlock

Fixed missing spin_unlock.

Signed-off-by: Takashi Iwai <tiwai@suse.de>


--- linux/sound/pci/au88x0/au88x0_pcm.c:1.9	Thu Aug 18 05:43:12 2005
+++ linux/sound/pci/au88x0/au88x0_pcm.c	Wed Aug 24 09:57:25 2005
@@ -220,8 +220,10 @@
 		    vortex_adb_allocroute(chip, -1,
 					  params_channels(hw_params),
 					  substream->stream, type);
-		if (dma < 0)
+		if (dma < 0) {
+			spin_unlock_irq(&chip->lock);
 			return dma;
+		}
 		stream = substream->runtime->private_data = &chip->dma_adb[dma];
 		stream->substream = substream;
 		/* Setup Buffers. */
--- linux/sound/synth/emux/emux_synth.c:1.11	Tue Dec  7 07:36:07 2004
+++ linux/sound/synth/emux/emux_synth.c	Wed Aug 24 09:57:25 2005
@@ -98,7 +98,6 @@
 		vp = emu->ops.get_voice(emu, port);
 		if (vp == NULL || vp->ch < 0)
 			continue;
-		snd_assert(vp->emu != NULL && vp->hw != NULL, return);
 		if (STATE_IS_PLAYING(vp->state))
 			emu->ops.terminate(vp);