[linux-dvb] [PATCH] dvb-core: Fix several locking related problems.

[Oliver Endriss]

Johannes Stezenbach wrote:
> On Mon, Mar 05, 2007 at 06:19:01PM +0100, Oliver Endriss wrote:
> > 
> > From 'Linux Device Drivers' (replace 'down' by 'mutex_lock'):
> > | ...
> > | down decrements the value of the semaphore and waits as long as need
> > | be. down_ interruptible does the same, but the operation is
> > | interruptible. The interruptible version is almost always the one you
> > | will want; it allows a user-space process that is waiting on a
> > | semaphore to be interrupted by the user. You do not, as a general
> >                                            ^^^^^^^^^^^^^^^^^^^^^^^^
> > | rule, want to use noninterruptible operations unless there truly is no
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > | alternative. Noninterruptible operations are a good way to create
> >   ^^^^^^^^^^^
> > | unkillable processes (the dreaded  D state  seen in ps), and annoy
> > | your users. Using down_interruptible requires some extra care,
> > | however, if the operation is interrupted, the function returns a
> > | nonzero value, and the caller does not hold the semaphore. Proper use
> > | of down_interruptible requires always checking the return value and
> > | responding accordingly.            
> > | ...
> 
> I don't think this advice applies to mutexes (at least if the
> mutexes are used in the usual way to protect some common data).
> 
> For event semaphores, you block waiting for an event, and if
> the event doesn't happen (maybe you wait for an irq), you need
> a way out or you're screwed. So using down_interruptible()
> is what you want.
> 
> Mutexes, however, are supposed to be held only while manipulating
> some shared data. So the code looks *always* like this:
> 
> 	mutex_lock(&mtx);
> 	do_something_with_shared_data();
> 	mutex_unlock(&mtx);
> 
> Now if some process sleeps uninterruptibly in mutex_lock(),
> some other process holds the mutex and sleeps in do_something().
> If it wedges up, you either have some locking bug
> (someone forgot a mutex_unlock()), or it wedged up in
> do_something(), and you got to fix *that*.

Well, your example is simple.
Locking is easy if there is only one mutex involved.

But there are a lot of mutexes/spinlocks/event semaphores in various
layers of the subsystem. I simply cannot tell whether a deadlock may
happen under some rare conditions or not...

> mutex_unlock_interruptible() would only help to paper over
> locking bugs, and its use in dvb-core comes from a straight
> conversion from down_interruptible(), which itself was used
> because it was once useful during development. How that the
> signal handling was found to be buggy I think it's much better
> to use mutex_lock() instead of fixing the mutex_unlock_interruptible()
> usage.
> 
> BTW, some statistics:
> 
> linux-2.6.20$ grep -r '\<mutex_lock\>' . | wc -l
> 3080
> linux-2.6.20$ grep -r '\<mutex_lock_interruptible\>' . | wc -l
> 236

Ok, to make it short:
Please proceed and apply this patch, or convert all occurrences of
mutex_lock_interruptible() to mutex_lock() if you prefer.
dvb_core is not my working area. ;-)

Oliver

-- 
--------------------------------------------------------
VDR Remote Plugin 0.3.9 available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------