linux-kernel - Re: [PATCH 1/3] iio: gts: Simplify using _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241130175141.14d3589a@jic23-huawei>
Date: Sat, 30 Nov 2024 17:51:41 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
 <lars@...afoo.de>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] iio: gts: Simplify using __free

On Thu, 28 Nov 2024 18:50:24 +0200
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> The error path in the gain_to_scaletables() uses goto for unwinding an
> allocation on failure. This can be slightly simplified by using the
> automated free when exiting the scope.
> 
> Use __free(kfree) and drop the goto based error handling.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> ---
> This is derived from the:
> https://lore.kernel.org/lkml/5efc30d832275778d1f48d7e2c75b1ecc63511d5.1732105157.git.mazziesaccount@gmail.com/

Hi Matti

A few comments on specific parts of this below

Thanks,

Jonathan

> +static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_bytes)
> +{
> +	int ret, i, j, new_idx, time_idx;
> +	int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes,
> +					       GFP_KERNEL);
> +
>  	if (!all_gains)
>  		return -ENOMEM;
>  
> @@ -232,10 +238,9 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>  
>  	gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int),
>  					      GFP_KERNEL);

I'm not particularly keen in a partial application of __free magic.

Perhaps you can use a local variable for this and a no_free_ptr() to assign it after we know
there can't be an error that requires it to be freed.

> -	if (!gts->avail_all_scales_table) {
> -		ret = -ENOMEM;
> -		goto free_out;
> -	}
> +	if (!gts->avail_all_scales_table)
> +		return -ENOMEM;
> +
>  	gts->num_avail_all_scales = new_idx;
>  
>  	for (i = 0; i < gts->num_avail_all_scales; i++) {
> @@ -246,14 +251,25 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>  		if (ret) {
>  			kfree(gts->avail_all_scales_table);
>  			gts->num_avail_all_scales = 0;
> -			goto free_out;
> +			return ret;
>  		}
>  	}
>  
> -free_out:
> -	kfree(all_gains);
> +	return 0;
> +}
>  
> -	return ret;
> +static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> +{
> +	int ret;
> +	size_t gain_bytes;
> +
> +	ret = fill_and_sort_scaletables(gts, gains, scales);
> +	if (ret)
> +		return ret;
> +
> +	gain_bytes = array_size(gts->num_hwgain, sizeof(int));

array_size is documented as being for 2D arrays, not an array of a multi byte
type.  We should not use it for this purpose.

I'd be tempted to not worry about overflow, but if you do want to be sure then
copy what kcalloc does and use a check_mul_overflow()

https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/slab.h#L919

You don't have to tidy that up in this patch though.

> +
> +	return build_combined_table(gts, gains, gain_bytes);
>  }

>  
> +/**
> + * iio_gts_build_avail_time_table - build table of available integration times
> + * @gts:	Gain time scale descriptor
> + *
> + * Build the table which can represent the available times to be returned
> + * to users using the read_avail-callback.
> + *
> + * NOTE: Space allocated for the tables must be freed using
> + * iio_gts_purge_avail_time_table() when the tables are no longer needed.
> + *
> + * Return: 0 on success.
> + */
> +static int iio_gts_build_avail_time_table(struct iio_gts *gts)
Hmm. I guess this wrapper exists because perhaps you aren't comfortable
yet with the __free() handling mid function.  It does not harm so I'm fine
having this.

> +{
> +	if (!gts->num_itime)
> +		return 0;
> +
> +	return __iio_gts_build_avail_time_table(gts);
> +}
> +
>  /**
>   * iio_gts_purge_avail_time_table - free-up the available integration time table
>   * @gts:	Gain time scale descriptor