From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751511Ab3KRQYf (ORCPT ); Mon, 18 Nov 2013 11:24:35 -0500 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:57118 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751243Ab3KRQY1 (ORCPT ); Mon, 18 Nov 2013 11:24:27 -0500 Message-ID: <1384791864.2001.19.camel@dabdike.int.hansenpartnership.com> Subject: Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code From: James Bottomley To: Geyslan =?ISO-8859-1?Q?Greg=F3rio?= Bem Cc: Jayamohan Kallickal , "open list:SERVER ENGINES 10..." , open list Date: Mon, 18 Nov 2013 08:24:24 -0800 In-Reply-To: References: <1384714284-13712-1-git-send-email-geyslan@gmail.com> <1384717371.2050.2.camel@dabdike.int.hansenpartnership.com> <1384724339.2050.6.camel@dabdike.int.hansenpartnership.com> <1384786707.2001.6.camel@dabdike.int.hansenpartnership.com> Content-Type: text/plain; charset="ISO-8859-15" X-Mailer: Evolution 3.8.5 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote: > 2013/11/18 James Bottomley : > > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote: > >> 2013/11/17 James Bottomley : > >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote: > >> >> 2013/11/17 James Bottomley : > >> >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: > >> >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and > >> >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when > >> >> >> necessary. > >> >> > > >> >> > You pointlessly renamed a variable, which makes the diff hard to read. > >> >> > Please don't do that. > >> >> > >> >> Ok, I can agree. 'len' means length? What is returned in case of non > >> >> error? > >> > > >> > it returns the length of buf written to or negative error. > >> > > >> >> > You missed the fact that the passed in pointer is unmodified if > >> >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and would > >> >> > oops. > >> >> > > >> >> > There's no need for a goto; len = -EINVAL; does everything that's > >> >> > needed. > >> >> > >> >> Well, that is a coverity catch. CID 1128954. Check it. > >> > > >> > I didn't say coverity was wrong, I said your patch was (well not wrong, > >> > just over complex and incomplete). This is the way to fix both > >> > problems. > >> > > >> > James > >> > > >> > --- > >> > > >> > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c > >> > index ffadbee..9dcbdfa 100644 > >> > --- a/drivers/scsi/be2iscsi/be_iscsi.c > >> > +++ b/drivers/scsi/be2iscsi/be_iscsi.c > >> > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba, > >> > ip_type = BE2_IPV6; > >> > >> James, this approach will not prevent the leakage. > > > > I don't see why not. The -EINVAL case goes through the kfree() now too, > > no? > > I'm refering to the removal of kfree in your suggestion. That's the second bug I pointed out via code inspection. If the function returns an error (any non zero return) then the pointer isn't altered, so we return without the free. It's a standard error pattern. > > > >> We can initialize the if_info with NULL and always kfree it without > >> to care about junk. > > > > Why? Error return means no allocation. > Setting if_info to NULL allow to kfree without concerns. > > Eg.: > > - struct be_cmd_get_if_info_resp *if_info; > + struct be_cmd_get_if_info_resp *if_info = NULL; > > ... > > + if (len) > + goto out; > > ... > > - if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) > - return -EINVAL; > + if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) { > + len = -EINVAL; > + goto out; > + } What's the point of that? Just removing the goto out; has the code going to the same place because of the break below. James