Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753491AbeAQODr (ORCPT + 1 other); Wed, 17 Jan 2018 09:03:47 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:44506 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752659AbeAQODp (ORCPT ); Wed, 17 Jan 2018 09:03:45 -0500 Date: Wed, 17 Jan 2018 15:02:16 +0100 From: Greg Kroah-Hartman To: "Dilger, Andreas" Cc: "Eremin, Dmitry" , "devel@driverdev.osuosl.org" , "Drokin, Oleg" , James Simmons , Dan Carpenter , Linux Kernel Mailing List , Lustre Development List Subject: Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch Message-ID: <20180117140216.GA5449@kroah.com> References: <1516114161-27679-1-git-send-email-Dmitry.Eremin@intel.com> <9FC73D3DBECE0941BD2ED069D26863425CE75F26@irsmsx110.ger.corp.intel.com> <20180116165628.GB7066@kroah.com> <6D19AB36-CDDF-4743-AF46-ADF31E26B4F4@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6D19AB36-CDDF-4743-AF46-ADF31E26B4F4@intel.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, Jan 17, 2018 at 12:36:19AM +0000, Dilger, Andreas wrote: > > > On Jan 16, 2018, at 09:56, Greg Kroah-Hartman wrote: > > > > On Tue, Jan 16, 2018 at 03:01:49PM +0000, Eremin, Dmitry wrote: > >> In the original commit 4d99b2581effe115376402e710fbcb1c3c073769 > > > > Please use the documented way to write this: > > 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd") > > > > >> was missed one hunk. Added it now to avoid issue with use after free. > > > > And I do not understand this commit message at all. > > > >> Signed-off-by: Dmitry Eremin > >> --- > >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c > >> index 2ebc484..a15a625 100644 > >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c > >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c > >> @@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn) > >> atomic_dec(&net->ibn_nconns); > >> } > >> > >> - kfree(conn); > >> + if (free_conn) > >> + kfree(conn); > > > > This looks really odd, don't you think? > > I'm not sure what the objection is here? There is an argument to this > this function named "free_conn" which determines if the structure should > be freed, or if the network connection is just being torn down and > reconnected. At first glance it really looks like the normal pattern of: if (foo_ptr) kfree(foo_ptr); right? If you don't want to free the variable, set it to NULL. Even then, this is a horrible function, you should have 2 different ones: kiblnd_destroy_conn(...) kiblnd_free_conn() and then just free the variable in the free_conn() function if you were going to set the free_conn variable, right? That way no odd code paths are taken, and it's obvious what you are doing just by reading the code at the callsite. thanks, greg k-h