Re: [PATCH] kernel: Unlock after locking

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Mar 03, 2015 at 08:49:10AM +0530, Tapasweni Pathak wrote:
> Release lock before returning.
> 
> Signed-off-by: Tapasweni Pathak <tapaswenipathak@gmail.com>
> ---
> I'm not sure if this is a bug, it seems like it is intentional, but
> there is no comment or anything like that which confirms this.

Er...  How about looking at the callers?  That's in acct_get() and the
only caller is nearby - it's
static void slow_acct_process(struct pid_namespace *ns)
{
        for ( ; ns; ns = ns->parent) {
                struct bsd_acct_struct *acct = acct_get(ns);
                if (acct) {
                        do_acct_process(acct);
                        mutex_unlock(&acct->lock);
                        acct_put(acct);
                }
        }
}

which obviously expects that acct_get() returns either NULL or
a pointer to an instance of struct bsd_acct_struct *and* expects
.lock of that instance to be locked in the latter case...

IOW, NAK.  Out of curiosity, what's the point of that patch, seeing that
you suspected that current behaviour was intentional, in which case the
patch would obviously break things?  As it does, in fact...  What's more, 
either we are leaking a lock every time we hit that codepath (i.e. every
time we get around to call of do_acct_process()), or you are introducing
double unlocks - if this lock is not leaked, it has to be dropped somewhere.

I'm not saying that you'll never run into really dumb bugs, but it's
generally useful to reason a bit about the observable consequences such
a bug might have - if nothing else, that might yield a test you could
use to verify that the bug was, indeed, fixed by your patch.  In this
case it would be "deadlock on the second exit() after having the
process accounting enabled", which would be very easy to observe if it
happened.  What's more, trying to do that _after_ applying your patch
would have lockdep yelling at you about mutex_unlock() on a mutex that
is not locked, which would indicate that something has gone wrong...