From: "Mintz, Yuval" <Yuval.Mintz@cavium.com>
To: Colin King <colin.king@canonical.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Elior, Ariel" <Ariel.Elior@cavium.com>,
        "Tayar, Tomer" <Tomer.Tayar@cavium.com>
Subject: RE: [PATCH][V2] qed: fix memory leak of a qed_spq_entry on error
 failure paths
Thread-Topic: [PATCH][V2] qed: fix memory leak of a qed_spq_entry on error
 failure paths
Thread-Index: AQHSWraF2ob/Bz0D3Ei/hjfvfzGeUKESY79A
Date: Wed, 21 Dec 2016 13:29:56 +0000
Message-ID: <BL2PR07MB230615AAE3CF67DEBC6083778D930@BL2PR07MB2306.namprd07.prod.outlook.com>
References: <20161220114424.11244-1-colin.king@canonical.com>
In-Reply-To: <20161220114424.11244-1-colin.king@canonical.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Dec 2016 13:29:56.6190
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO2PR07MB2615
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uBLF3OnS003495
Content-Length: 1267
Lines: 29

> From: Colin Ian King <colin.king@canonical.com>
> 
> A qed_spq_entry entry is allocated by qed_sp_init_request but is not kfree'd
> if an error occurs, causing a memory leak. Fix this by returning the previously
> allocated spq entry and also setting *pp_ent to NULL to be safe.
> 
> Thanks to Yuval Mintz for suggestions on how to improve my original fix.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

We've given it a more thorough look, and apparently this isn't the correct fix.
So I'll start by saying sorry for making you send this V2 needlessly.

It boils down to the fact there are two kinds of SPQ entries -
Those originating from the 'free_pool' and those from the 'unlimited_pending'.
Only those originating from the free_pool should be returned
using the qed_spq_return_entry(), as only those actually point to a valid
dma-mapped memory where FW expects to find the entries;
Returning the other kind would lead to assertions later,
as driver would post a ramrod to FW which actually points to address 0.

Looking at the error flows, it seems possible this isn't the only faulty
error flow in the SPQ. I suggest you'd drop this and we'll take it from
here [although if you really have the urge to continue - please do].

Thanks,
Yuval