Re: [PATCH] mtd: nand: free vendor-specific resources in init failure paths

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon,  1 May 2017 17:04:53 -0700
Brian Norris <computersforpeace@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().

Note that nand_scan_ident() is already allocating resources through
nand_init_data_interface() which are also leaked if nand_cleanup() is
not called. Again, we could solve the problem by moving data-iface
initialization steps in nand_scan_tail() (which shouldn't be a problem
AFAICS). Alternatively, we could could consider that nand_cleanup() is
smart enough to know what to not release (which seems to be the case
already), and force drivers to call nand_cleanup() as soon as
nand_scan_ident() has returned 0.

Brian, any opinion?

Anyway, this is a bit off-topic, so for this patch

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Fixes: 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 2adcc8cdedf1..978242b1213f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4300,7 +4300,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	/* Initialize the ->data_interface field. */
>  	ret = nand_init_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	/*
>  	 * Setup the data interface correctly on the chip and controller side.
> @@ -4312,7 +4312,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	 */
>  	ret = nand_setup_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	nand_maf_id = chip->id.data[0];
>  	nand_dev_id = chip->id.data[1];
> @@ -4343,6 +4343,12 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	mtd->size = i * chip->chipsize;
>  
>  	return 0;
> +
> +err_nand_init:
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_ident);
>  
> @@ -4513,18 +4519,23 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
>  	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> -		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
> -		return -EINVAL;
> +		   !(chip->bbt_options & NAND_BBT_USE_FLASH))) {
> +		ret = -EINVAL;
> +		goto err_ident;
> +	}
>  
>  	if (invalid_ecc_page_accessors(chip)) {
>  		pr_err("Invalid ECC page accessors setup\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_ident;
>  	}
>  
>  	if (!(chip->options & NAND_OWN_BUFFERS)) {
>  		nbuf = kzalloc(sizeof(*nbuf), GFP_KERNEL);
> -		if (!nbuf)
> -			return -ENOMEM;
> +		if (!nbuf) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  
>  		nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
>  		if (!nbuf->ecccalc) {
> @@ -4547,8 +4558,10 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  		chip->buffers = nbuf;
>  	} else {
> -		if (!chip->buffers)
> -			return -ENOMEM;
> +		if (!chip->buffers) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  	}
>  
>  	/* Set the internal oob buffer location, just after the page data */
> @@ -4789,6 +4802,13 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		kfree(nbuf->ecccalc);
>  		kfree(nbuf);
>  	}
> +
> +err_ident:
> +	/* Clean up nand_scan_ident(). */
> +
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_tail);