From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754081AbbCCGlM (ORCPT ); Tue, 3 Mar 2015 01:41:12 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:48381 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752921AbbCCGlK (ORCPT ); Tue, 3 Mar 2015 01:41:10 -0500 Date: Tue, 3 Mar 2015 06:41:05 +0000 From: Al Viro To: Tapasweni Pathak Cc: akpm@linux-foundation.org, tglx@linutronix.de, ionut.m.alexa@gmail.com, paulmcquad@gmail.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel: Unlock after locking Message-ID: <20150303064105.GQ29656@ZenIV.linux.org.uk> References: <20150303031910.GA25670@kt-Inspiron-3542> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150303031910.GA25670@kt-Inspiron-3542> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 03, 2015 at 08:49:10AM +0530, Tapasweni Pathak wrote: > Release lock before returning. > > Signed-off-by: Tapasweni Pathak > --- > 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...