Re: [PATCH 2/2] ubi: free the normal volumes in error paths of ubi_attach_mtd_dev()

[Hou Tao <houtao1@xxxxxxxxxx>]

ping ?

On 2019/11/30 17:48, Hou Tao wrote:
> The allocated normal volumes saved in ubi->volumes are not freed
> in the error paths in ubi_attach_mtd_dev() and its callees (e.g.
> ubi_attach() and ubi_read_volume_table()).
> 
> These normal volumes should be freed through kill_volumes() and
> vol_release(), but ubi_attach_mtd_dev() may fail before
> calling uif_init(), and there will be memory leaks.
> 
> So adding a new helper ubi_free_all_volumes() to free the normal
> and the internal volumes. And in order to prevent double-free
> of volume, reset ubi->volumes[i] to NULL after freeing.
> 
> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> ---
>  drivers/mtd/ubi/attach.c |  2 +-
>  drivers/mtd/ubi/build.c  | 31 ++++++++++++++++++++++++++-----
>  drivers/mtd/ubi/ubi.h    |  1 +
>  drivers/mtd/ubi/vtbl.c   | 10 ++--------
>  4 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
> index 10b2459f8951..ea7440ac913b 100644
> --- a/drivers/mtd/ubi/attach.c
> +++ b/drivers/mtd/ubi/attach.c
> @@ -1640,7 +1640,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
>  out_wl:
>  	ubi_wl_close(ubi);
>  out_vtbl:
> -	ubi_free_internal_volumes(ubi);
> +	ubi_free_all_volumes(ubi);
>  	vfree(ubi->vtbl);
>  out_ai:
>  	destroy_ai(ai);
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index d636bbe214cb..25fb72b2efa0 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -503,21 +503,42 @@ static void uif_close(struct ubi_device *ubi)
>  }
>  
>  /**
> - * ubi_free_internal_volumes - free internal volumes.
> + * ubi_free_volumes_from - free volumes from specific index.
>   * @ubi: UBI device description object
> + * @from: the start index used for volume free.
>   */
> -void ubi_free_internal_volumes(struct ubi_device *ubi)
> +static void ubi_free_volumes_from(struct ubi_device *ubi, int from)
>  {
>  	int i;
>  
> -	for (i = ubi->vtbl_slots;
> -	     i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
> +	for (i = from; i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
> +		if (!ubi->volumes[i])
> +			continue;
>  		ubi_eba_replace_table(ubi->volumes[i], NULL);
>  		ubi_fastmap_destroy_checkmap(ubi->volumes[i]);
>  		kfree(ubi->volumes[i]);
> +		ubi->volumes[i] = NULL;
>  	}
>  }
>  
> +/**
> + * ubi_free_all_volumes - free all volumes.
> + * @ubi: UBI device description object
> + */
> +void ubi_free_all_volumes(struct ubi_device *ubi)
> +{
> +	ubi_free_volumes_from(ubi, 0);
> +}
> +
> +/**
> + * ubi_free_internal_volumes - free internal volumes.
> + * @ubi: UBI device description object
> + */
> +void ubi_free_internal_volumes(struct ubi_device *ubi)
> +{
> +	ubi_free_volumes_from(ubi, ubi->vtbl_slots);
> +}
> +
>  static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
>  {
>  	int limit, device_pebs;
> @@ -1013,7 +1034,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>  out_detach:
>  	ubi_devices[ubi_num] = NULL;
>  	ubi_wl_close(ubi);
> -	ubi_free_internal_volumes(ubi);
> +	ubi_free_all_volumes(ubi);
>  	vfree(ubi->vtbl);
>  out_free:
>  	vfree(ubi->peb_buf);
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index 721b6aa7936c..5ab3affd2fcf 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -948,6 +948,7 @@ int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol,
>  int ubi_notify_all(struct ubi_device *ubi, int ntype,
>  		   struct notifier_block *nb);
>  int ubi_enumerate_volumes(struct notifier_block *nb);
> +void ubi_free_all_volumes(struct ubi_device *ubi);
>  void ubi_free_internal_volumes(struct ubi_device *ubi);
>  
>  /* kapi.c */
> diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> index 8a2a0f091598..f700f0e4f2ec 100644
> --- a/drivers/mtd/ubi/vtbl.c
> +++ b/drivers/mtd/ubi/vtbl.c
> @@ -782,7 +782,7 @@ static int check_attaching_info(const struct ubi_device *ubi,
>   */
>  int ubi_read_volume_table(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  {
> -	int i, err;
> +	int err;
>  	struct ubi_ainf_volume *av;
>  
>  	empty_vtbl_record.crc = cpu_to_be32(0xf116c36b);
> @@ -851,13 +851,7 @@ int ubi_read_volume_table(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  
>  out_free:
>  	vfree(ubi->vtbl);
> -	for (i = 0; i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
> -		if (!ubi->volumes[i])
> -			continue;
> -		ubi_fastmap_destroy_checkmap(ubi->volumes[i]);
> -		kfree(ubi->volumes[i]);
> -		ubi->volumes[i] = NULL;
> -	}
> +	ubi_free_all_volumes(ubi);
>  	return err;
>  }
>  
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/