[PATCH] mtd: nand: free vendor-specific resources in init failure paths
Boris Brezillon
boris.brezillon at free-electrons.com
Tue May 2 09:15:39 PDT 2017
On Tue, 2 May 2017 09:52:30 +0200
Boris Brezillon <boris.brezillon at free-electrons.com> wrote:
> On Mon, 1 May 2017 17:04:53 -0700
> Brian Norris <computersforpeace at gmail.com> wrote:
>
> > If we fail any time after calling nand_detect(), then we don't call the
> > vendor-specific ->cleanup() callback, and we'll leak any resources the
> > vendor-specific code might have allocated.
> >
> > Mark the "fix" against the first commit that started allocating anything
> > in ->init().
>
> Yep, I noticed this leak while reviewing/applying Masahiro's series
> [1], and forgot to send a fix for this bug.
>
> Actually, I'm not sure we should keep nand_manufacturer_init() in
> nand_scan_ident(), especially if we consider that nand_scan_ident() is
> not supposed to allocate resources and does not require a
> nand_cleanup() when something fails between nand_scan_ident() and
> nand_scan_tail().
>
Just for the record, this is the kind of fix I had in mind, but it can
still be applied on top of your patch.
--->8---
From fd918ec7ed960792f020227b675838fee6f710d9 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon at free-electrons.com>
Date: Tue, 2 May 2017 18:10:18 +0200
Subject: [PATCH] mtd: nand: Fix various memory leaks in core
The nand_scan_ident() function is not expected to allocate resources,
and people are usually not calling nand_cleanup() if something fails
between nand_scan_ident() and nand_scan_tail().
Move all functions that may allocate resource to the nand_scan_tail()
path to prevent such resource leaks.
Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
---
Only compile tested.
---
drivers/mtd/nand/nand_base.c | 79 ++++++++++++++++++++++++--------------------
1 file changed, 44 insertions(+), 35 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ed49a1d634b0..6ba03921eddb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3939,7 +3939,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
const struct nand_manufacturer *manufacturer;
struct mtd_info *mtd = nand_to_mtd(chip);
int busw;
- int i, ret;
+ int i;
u8 *id_data = chip->id.data;
u8 maf_id, dev_id;
@@ -4080,10 +4080,6 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
chip->cmdfunc = nand_command_lp;
- ret = nand_manufacturer_init(chip);
- if (ret)
- return ret;
-
pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
maf_id, dev_id);
@@ -4290,23 +4286,6 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
return ret;
}
- /* Initialize the ->data_interface field. */
- ret = nand_init_data_interface(chip);
- if (ret)
- return ret;
-
- /*
- * Setup the data interface correctly on the chip and controller side.
- * This explicit call to nand_setup_data_interface() is only required
- * for the first die, because nand_reset() has been called before
- * ->data_interface and ->default_onfi_timing_mode were set.
- * For the other dies, nand_reset() will automatically switch to the
- * best mode for us.
- */
- ret = nand_setup_data_interface(chip);
- if (ret)
- return ret;
-
nand_maf_id = chip->id.data[0];
nand_dev_id = chip->id.data[1];
@@ -4502,7 +4481,7 @@ int nand_scan_tail(struct mtd_info *mtd)
struct nand_chip *chip = mtd_to_nand(mtd);
struct nand_ecc_ctrl *ecc = &chip->ecc;
struct nand_buffers *nbuf = NULL;
- int ret;
+ int ret, i;
/* New bad blocks should be marked in OOB, flash-based BBT, or both */
if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
@@ -4522,20 +4501,20 @@ int nand_scan_tail(struct mtd_info *mtd)
nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
if (!nbuf->ecccalc) {
ret = -ENOMEM;
- goto err_free;
+ goto err_free_nbuf;
}
nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
if (!nbuf->ecccode) {
ret = -ENOMEM;
- goto err_free;
+ goto err_free_nbuf;
}
nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
GFP_KERNEL);
if (!nbuf->databuf) {
ret = -ENOMEM;
- goto err_free;
+ goto err_free_nbuf;
}
chip->buffers = nbuf;
@@ -4544,6 +4523,10 @@ int nand_scan_tail(struct mtd_info *mtd)
return -ENOMEM;
}
+ ret = nand_manufacturer_init(chip);
+ if (ret)
+ goto err_free_nbuf;
+
/* Set the internal oob buffer location, just after the page data */
chip->oob_poi = chip->buffers->databuf + mtd->writesize;
@@ -4565,7 +4548,7 @@ int nand_scan_tail(struct mtd_info *mtd)
WARN(1, "No oob scheme defined for oobsize %d\n",
mtd->oobsize);
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
}
@@ -4580,7 +4563,7 @@ int nand_scan_tail(struct mtd_info *mtd)
if (!ecc->calculate || !ecc->correct || !ecc->hwctl) {
WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
if (!ecc->read_page)
ecc->read_page = nand_read_page_hwecc_oob_first;
@@ -4612,7 +4595,7 @@ int nand_scan_tail(struct mtd_info *mtd)
ecc->write_page == nand_write_page_hwecc)) {
WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
/* Use standard syndrome read/write page function? */
if (!ecc->read_page)
@@ -4632,7 +4615,7 @@ int nand_scan_tail(struct mtd_info *mtd)
if (!ecc->strength) {
WARN(1, "Driver must set ecc.strength when using hardware ECC\n");
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
break;
}
@@ -4645,7 +4628,7 @@ int nand_scan_tail(struct mtd_info *mtd)
ret = nand_set_ecc_soft_ops(mtd);
if (ret) {
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
break;
@@ -4665,7 +4648,7 @@ int nand_scan_tail(struct mtd_info *mtd)
default:
WARN(1, "Invalid NAND_ECC_MODE %d\n", ecc->mode);
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
/* For many systems, the standard OOB write also works for raw */
@@ -4686,7 +4669,7 @@ int nand_scan_tail(struct mtd_info *mtd)
if (ecc->steps * ecc->size != mtd->writesize) {
WARN(1, "Invalid ECC parameters\n");
ret = -EINVAL;
- goto err_free;
+ goto err_nand_manuf_cleanup;
}
ecc->total = ecc->steps * ecc->bytes;
@@ -4769,13 +4752,39 @@ int nand_scan_tail(struct mtd_info *mtd)
if (!mtd->bitflip_threshold)
mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3, 4);
+ /* Initialize the ->data_interface field. */
+ ret = nand_init_data_interface(chip);
+ if (ret)
+ goto err_nand_manuf_cleanup;
+
+ /* Enter fastest possible mode on all dies. */
+ for (i = 0; i < chip->numchips; i++) {
+ chip->select_chip(mtd, i);
+ ret = nand_setup_data_interface(chip);
+ chip->select_chip(mtd, -1);
+
+ if (ret)
+ goto err_nand_data_iface_cleanup;
+ }
+
/* Check, if we should skip the bad block table scan */
if (chip->options & NAND_SKIP_BBTSCAN)
return 0;
/* Build bad block table */
- return chip->scan_bbt(mtd);
-err_free:
+ ret = chip->scan_bbt(mtd);
+ if (ret)
+ goto err_nand_data_iface_cleanup;
+
+ return 0;
+
+err_nand_data_iface_cleanup:
+ nand_release_data_interface(chip);
+
+err_nand_manuf_cleanup:
+ nand_manufacturer_cleanup(chip);
+
+err_free_nbuf:
if (nbuf) {
kfree(nbuf->databuf);
kfree(nbuf->ecccode);
More information about the linux-mtd
mailing list