* Re: [PATCH] kernel: Unlock after locking
2015-03-03 3:19 [PATCH] kernel: Unlock after locking Tapasweni Pathak
@ 2015-03-03 4:15 ` David Rientjes
2015-03-03 6:41 ` Al Viro
1 sibling, 0 replies; 3+ messages in thread
From: David Rientjes @ 2015-03-03 4:15 UTC (permalink / raw)
To: Tapasweni Pathak
Cc: viro, akpm, tglx, ionut.m.alexa, paulmcquad, linux-kernel
On Tue, 3 Mar 2015, Tapasweni Pathak wrote:
> diff --git a/kernel/acct.c b/kernel/acct.c
> index e6c10d1a..f592218 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -161,6 +161,7 @@ again:
> acct_put(res);
> goto again;
> }
> + mutex_unlock(&res->lock);
> return res;
> }
>
This function returns with the mutex held, the caller releases it.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] kernel: Unlock after locking
2015-03-03 3:19 [PATCH] kernel: Unlock after locking Tapasweni Pathak
2015-03-03 4:15 ` David Rientjes
@ 2015-03-03 6:41 ` Al Viro
1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2015-03-03 6:41 UTC (permalink / raw)
To: Tapasweni Pathak; +Cc: akpm, tglx, ionut.m.alexa, paulmcquad, linux-kernel
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...
^ permalink raw reply [flat|nested] 3+ messages in thread