From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941608AbcIHLsV (ORCPT ); Thu, 8 Sep 2016 07:48:21 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:43828 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932165AbcIHLsT (ORCPT ); Thu, 8 Sep 2016 07:48:19 -0400 From: Sebastian Andrzej Siewior To: Andrew Morton Cc: linux-kernel@vger.kernel.org, Sebastian Andrzej Siewior , Anton Vorontsov , Colin Cross , Kees Cook , Tony Luck , Namhyung Kim Subject: [REPOST PATCH 1/2] pstore/ramoops: fixup driver removal Date: Thu, 8 Sep 2016 13:48:05 +0200 Message-Id: <20160908114806.10686-1-bigeasy@linutronix.de> X-Mailer: git-send-email 2.9.3 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org A basic rmmod ramoops segfaults. Let's see why. Since commit 34f0ec82e0a9 ("pstore: Correct the max_dump_cnt clearing of ramoops") sets ->max_dump_cnt to zero before looping over ->przs but we didn't use it before that either. And since commit ee1d267423a1 ("pstore: add pstore unregister") we free that memory on rmmod. But even then, we looped until a NULL pointer or ERR. I don't see where it is ensured that the last member is NULL. Let's try this instead: simply error recovery and free. Clean up in error case where ressouces were allocated. And then, in the free path, rely on ->max_dump_cnt in the free path. Cc: Anton Vorontsov Cc: Colin Cross Cc: Kees Cook Cc: Tony Luck Cc: Namhyung Kim Acked-by: Namhyung Kim Signed-off-by: Sebastian Andrzej Siewior --- fs/pstore/ram.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 7a034d62cf8c..2340262a7e97 100644 --- a/fs/pstore/ram.c +++ b/fs/pstore/ram.c @@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context = *cxt) { int i; =20 - cxt->max_dump_cnt =3D 0; if (!cxt->przs) return; =20 - for (i =3D 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++) + for (i =3D 0; i < cxt->max_dump_cnt; i++) persistent_ram_free(cxt->przs[i]); + kfree(cxt->przs); + cxt->max_dump_cnt =3D 0; } =20 static int ramoops_init_przs(struct device *dev, struct ramoops_context *c= xt, @@ -408,7 +409,7 @@ static int ramoops_init_przs(struct device *dev, struct= ramoops_context *cxt, GFP_KERNEL); if (!cxt->przs) { dev_err(dev, "failed to initialize a prz array for dumps\n"); - goto fail_prz; + goto fail_mem; } =20 for (i =3D 0; i < cxt->max_dump_cnt; i++) { @@ -419,6 +420,11 @@ static int ramoops_init_przs(struct device *dev, struc= t ramoops_context *cxt, err =3D PTR_ERR(cxt->przs[i]); dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n", cxt->record_size, (unsigned long long)*paddr, err); + + while (i > 0) { + i--; + persistent_ram_free(cxt->przs[i]); + } goto fail_prz; } *paddr +=3D cxt->record_size; @@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct= ramoops_context *cxt, =20 return 0; fail_prz: - ramoops_free_przs(cxt); + kfree(cxt->przs); +fail_mem: + cxt->max_dump_cnt =3D 0; return err; } =20 @@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev) struct ramoops_context *cxt =3D &oops_cxt; =20 pstore_unregister(&cxt->pstore); - cxt->max_dump_cnt =3D 0; =20 kfree(cxt->pstore.buf); cxt->pstore.bufsize =3D 0; --=20 2.9.3