From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753028AbdDCRtC (ORCPT ); Mon, 3 Apr 2017 13:49:02 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:33271 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752935AbdDCRtA (ORCPT ); Mon, 3 Apr 2017 13:49:00 -0400 Date: Mon, 3 Apr 2017 10:48:57 -0700 From: Eric Biggers To: David Howells Cc: keyrings@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers Subject: Re: [PATCH] KEYS: put keyring if install_session_keyring_to_cred() fails Message-ID: <20170403174857.GA72831@gmail.com> References: <20170401185416.10225-1-ebiggers3@gmail.com> <2347.1491234125@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2347.1491234125@warthog.procyon.org.uk> 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 Hi David, On Mon, Apr 03, 2017 at 04:42:05PM +0100, David Howells wrote: > Eric Biggers wrote: > > > +error3: > > + key_put(keyring); > > error2: > > mutex_unlock(&key_session_mutex); > > I would prefer the put to be outside of the locked section. How about the > attached instead? [...] > diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c > index 44451af828c0..8e98e5d1b5fb 100644 > --- a/security/keys/process_keys.c > +++ b/security/keys/process_keys.c > @@ -793,21 +793,20 @@ long join_session_keyring(const char *name) > KEY_ALLOC_IN_QUOTA, NULL, NULL); > if (IS_ERR(keyring)) { > ret = PTR_ERR(keyring); > - goto error2; > + goto error_unlock_no_keyring; > } > } else if (IS_ERR(keyring)) { > ret = PTR_ERR(keyring); > - goto error2; > + goto error_unlock_no_keyring; > } else if (keyring == new->session_keyring) { > - key_put(keyring); > ret = 0; > - goto error2; > + goto error_unlock; > } > > /* we've got a keyring - now to install it */ > ret = install_session_keyring_to_cred(new, keyring); > if (ret < 0) > - goto error2; > + goto error_unlock; > > commit_creds(new); > mutex_unlock(&key_session_mutex); > @@ -817,8 +816,11 @@ long join_session_keyring(const char *name) > okay: > return ret; > > -error2: > +error_unlock_no_keyring: > + keyring = NULL; > +error_unlock: > mutex_unlock(&key_session_mutex); > + key_put(keyring); > error: > abort_creds(new); > return ret; I guess I'm okay with either patch, but I think jumping to the error labels in the way I proposed is more straightforward since it keeps things in the logical order. And I doubt there's much advantage in moving key_put() outside of the mutex, given that key_session_mutex is only used for joining session keyrings so shouldn't be contended, and key_put() hardly does any work anyway. - Eric