From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933536AbbJISsv (ORCPT ); Fri, 9 Oct 2015 14:48:51 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:54415 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759164AbbJISsu (ORCPT ); Fri, 9 Oct 2015 14:48:50 -0400 X-Sasl-enc: 1fkBpIkw7b7a3JmzK0P+hP3+4MynIAUmnAJMuqutACeb 1444416519 Date: Fri, 9 Oct 2015 21:48:38 +0300 From: Sergei Zviagintsev To: David Herrmann Cc: Greg Kroah-Hartman , Daniel Mack , David Herrmann , Djalal Harouni , linux-kernel Subject: Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup() Message-ID: <20151009184838.GM2189@localhost.localdomain> References: <1cbeb0e49e6a51048b4f5c71e8d34fc82ce0a7f3.1444302968.git.sergei@s15v.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, On Thu, Oct 08, 2015 at 05:06:57PM +0200, David Herrmann wrote: > Hi > > On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev wrote: > > If idr_alloc() fails, we shouldn't call idr_remove() as latter produces > > warning when called on non-allocated ids. Split cleanup code into three > > parts for differrent cleanup scenarios before and after idr_alloc(). > > > > Signed-off-by: Sergei Zviagintsev > > --- > > ipc/kdbus/domain.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c > > index ac9f760c150d..31cd09fb572f 100644 > > --- a/ipc/kdbus/domain.c > > +++ b/ipc/kdbus/domain.c > > @@ -208,7 +208,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid) > > u = kzalloc(sizeof(*u), GFP_KERNEL); > > if (!u) { > > ret = -ENOMEM; > > - goto exit; > > + goto exit_unlock; > > } > > > > kref_init(&u->kref); > > @@ -225,7 +225,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid) > > ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid), > > __kuid_val(uid) + 1, GFP_KERNEL); > > if (ret < 0) > > - goto exit; > > + goto exit_free; > > } > > } > > > > @@ -235,19 +235,19 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid) > > */ > > ret = ida_simple_get(&domain->user_ida, 1, 0, GFP_KERNEL); > > if (ret < 0) > > - goto exit; > > + goto exit_idr; > > > > Why not simply assign "u->uid = uid;" _after_ doing the idr operations? If I understand it right, in this case we have to firstly assign INVALID_UID to u->uid (to check it with uid_valid() in the error path) and then do 'u->uid = uid'. But from the first sight it would be not so obvious and may require adding some comment on it. Isn't it better to stay explicit here by maintaining several goto labels? > > Thanks > David > > > u->id = ret; > > mutex_unlock(&domain->lock); > > return u; > > > > -exit: > > - if (u) { > > - if (uid_valid(u->uid)) > > - idr_remove(&domain->user_idr, __kuid_val(u->uid)); > > - kdbus_domain_unref(u->domain); > > - kfree(u); > > - } > > +exit_idr: > > + if (uid_valid(u->uid)) > > + idr_remove(&domain->user_idr, __kuid_val(u->uid)); > > +exit_free: > > + kdbus_domain_unref(u->domain); > > + kfree(u); > > +exit_unlock: > > mutex_unlock(&domain->lock); > > return ERR_PTR(ret); > > } > > -- > > 1.8.3.1 > >