Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: LEm0IEe3ItJ7CmA1O2Z4PSpoWx2wtpnq8jntfJqd/Uw0cczla5zelcACOSBeVTOU2Mg+iSsP6F
 ovi7lLzTYcaw==
IronPort-SDR: Xyr4BmkhoH5allwCx1J2v5ZZj/pn9A6anrpHZfhvC5mWpsgUNVQEa2eTLRmFdndIgK62uHknbN
 xhXdPQQG9BOA==
Date:   Tue, 11 Aug 2020 09:45:04 -0700
From:   Jesse Brandeburg <jesse.brandeburg@intel.com>
To:     Pavel Machek <pavel@denx.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>,
        Martyna Szapar <martyna.szapar@intel.com>,
        Andrew Bowers <andrewx.bowers@intel.com>,
        "Jeff Kirsher" <jeffrey.t.kirsher@intel.com>
Subject: Re: [PATCH 4.19 47/48] i40e: Memory leak in
 i40e_config_iwarp_qvlist
Message-ID: <20200811094504.000068bb@intel.com>
In-Reply-To: <20200811124614.2myealhkhnla6v3a@duo.ucw.cz>
References: <20200810151804.199494191@linuxfoundation.org>
        <20200810151806.541597863@linuxfoundation.org>
        <20200811124614.2myealhkhnla6v3a@duo.ucw.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 11 Aug 2020 14:46:14 +0200
Pavel Machek <pavel@denx.de> wrote:

> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -449,16 +450,19 @@ static int i40e_config_iwarp_qvlist(stru
> >  			 "Incorrect number of iwarp vectors %u.
> > Maximum %u allowed.\n", qvlist_info->num_vectors,
> >  			 msix_vf);
> > -		goto err;
> > +		ret = -EINVAL;
> > +		goto err_out;
> >  	}
> 
> And it is no longer freeing data qvlist_info() in this path. Is that
> correct? Should it goto err_free instead? 

Hi Pavel, thanks for the review.

I believe it is still correct, the logic is a bit convoluted, but
tracing back, I see that the caller in i40e_main.c allocates a buffer,
calls this function (eventually) with that memory cast to the *input*
variable qvlist_info, and then the top caller frees the original buffer.

One thing that I'll admit is confusing here is that the *input* struct
qvlist_info is different than the vf->qvlist_info struct managed by
this function. Maybe that they have the same name was confusing to you?
It confused me for a moment while I investigated, but I believe there
is no actual problem.

The reason for the function working this way is that the input data is
from the VF message, and the driver data structures in the PF (i40e)
driver representing state of the VF are managed separately. 

Jesse