svn commit: r282971 - in head/sys: kern sys

[Mateusz Guzik]

On Thu, May 21, 2015 at 11:33:02AM -0400, John Baldwin wrote:
> On Wednesday, May 20, 2015 12:00:46 PM Alexander Kabaev wrote:
> > On Fri, 15 May 2015 13:50:38 +0000 (UTC)
> > John Baldwin <jhb at FreeBSD.org> wrote:
> > 
> > > Author: jhb
> > > Date: Fri May 15 13:50:37 2015
> > > New Revision: 282971
> > > URL: https://svnweb.freebsd.org/changeset/base/282971
> > > 
> > > Log:
> > >   Previously, cv_waiters was only updated by cv_signal or cv_wait. If
> > > a thread awakened due to a time out, then cv_waiters was not
> > > decremented. If INT_MAX threads timed out on a cv without an
> > > intervening cv_broadcast, then cv_waiters could overflow. To fix
> > > this, have each sleeping thread decrement cv_waiters when it resumes.
> > >   
> > >   Note that previously cv_waiters was protected by the sleepq chain
> > > lock. However, that lock is not held when threads resume from sleep.
> > > In addition, the interlock is also not always reacquired after
> > > resuming (cv_wait_unlock), nor is it always held by callers of
> > > cv_signal() or cv_broadcast(). Instead, use atomic ops to update
> > > cv_waiters. Since the sleepq chain lock is still held on every
> > > increment, it should still be safe to compare cv_waiters against zero
> > > while holding the lock in the wakeup routines as the only way the
> > > race should be lost would result in extra calls to sleepq_signal() or
> > > sleepq_broadcast(). 
> > >   Differential Revision:	https://reviews.freebsd.org/D2427
> > >   Reviewed by:	benno
> > >   Reported by:	benno (wrap of cv_waiters in the field)
> > >   MFC after:	2 weeks
> > > 
> > > Modified:
> > >   head/sys/kern/kern_condvar.c
> > >   head/sys/sys/condvar.h
> > > 
> > 
> > This breaks ZFS range locking code, which expects to be able to wakeup
> > everyone on the condition variable and then free the structure that
> > contains it. Having woken up threads modify cv_waiters results in a
> > race that leads to already freed memory to be accessed.
> > 
> > It is debatable just how correct ZFS code in its expectations, but I
> > think this commit should probably be reverted until either ZFS is
> > changed not to expect cv modifiable by waking threads or until
> > alternative solution is found to the cv_waiters overflow issue fixed by
> > this commit.
> 
> Yes, this is fine to revert for now.  I'll have to think about which way
> to fix the bug in question.  The simplest route is probably to remove
> cv_waiters entirely, though hacking up sleepq_timeout to recognize cv's
> and decrement the count is another option (but hackier).
> 

The code was buggy even prior to this.

_cv_wait does:
#ifdef KTRACE
        if (KTRPOINT(td, KTR_CSW))
                ktrcsw(0, 0, cv_wmesg(cvp));
#endif

So with ktrace enabled cvp is possibly accessed after it gets freed.

I checked "solaris compatiblity layer" from "zfs on linux" project and it looks
like they are screwed by this as well.

In other words, I would argue modifying native privmitives to accomodate for
zfs use may not be the best course of action.

Hacking up zfs or cv_ primitives seems better and opens up posibilities for
minor optimisations.

I'm not up to it though.
-- 
Mateusz Guzik <mjguzik gmail.com>