linux-kernel - Re: [PATCH] kernel: Unlock after locking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 3 Mar 2015 06:41:05 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Tapasweni Pathak <tapaswenipathak@...il.com>
Cc:	akpm@...ux-foundation.org, tglx@...utronix.de,
	ionut.m.alexa@...il.com, paulmcquad@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel: Unlock after locking

On Tue, Mar 03, 2015 at 08:49:10AM +0530, Tapasweni Pathak wrote:
> Release lock before returning.
> 
> Signed-off-by: Tapasweni Pathak <tapaswenipathak@...il.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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/