Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 2 Sep 2008 10:19:38 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Tor Egge <Tor.Egge@cvsup.no.freebsd.org>
Cc:        attilio@freebsd.org, kevinxlinuz@163.com, freebsd-current@freebsd.org, Benjamin.Close@clearchain.com, kib@freebsd.org
Subject:   Re: [BUG] I think sleepqueue need to be protected in sleepq_broadcast
Message-ID:  <200809021019.39467.jhb@freebsd.org>
In-Reply-To: <20080901.013117.74700691.Tor.Egge@cvsup.no.freebsd.org>
References:  <200808230003.44081.jhb@freebsd.org> <48B6BC81.5060300@clearchain.com> <20080901.013117.74700691.Tor.Egge@cvsup.no.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday 31 August 2008 09:31:17 pm Tor Egge wrote:
> 
> sleepq_resume_thread() contains an ownership handover of sq if the resumed
> thread is the last one blocked on the wait channel.  After the handover, sq 
is
> no longer protected by the sleep queue chain lock and should no longer be
> accessed by sleepq_broadcast().
> 
> Normally, when sleepq_broadcast() incorrectly accesses sq after the 
handover,
> it will find the sq->sq_blocked queue to be empty, and the code appears to
> work.
> 
> If the last correctly woken thread manages to go to sleep again very quickly 
on
> another wait channel, sleepq_broadcast() might incorrectly determine that 
the
> sq->sq_blocked queue isn't empty, and start doing the wrong thing.
> 
> A similar (but probably much more difficult to trigger) issue is present 
with
> regards to thread_lock() and turnstiles.
> 
> The caller of thread_lock() might have performed sufficient locking to 
ensure
> that the thread to be locked doesn't go away, but any turnstile spin lock
> pointed to by td->td_lock isn't protected.  Making turnstiles type stable
> (setting UMA_ZONE_NOFREE flag for turnstile_zone) should fix that issue.

Good find!  I think the better fix is to not rely on type stability, but to 
have sleepq_resume_thread() indicate to the caller that it has claimed the 
sleepqueue instead.  I think this race is only possible with thread_lock() 
btw.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200809021019.39467.jhb>