linux-kernel - Re: [PATCH v2 5/7] iio: inkern: copy/release available info from producer

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: <8a5f6f12c2cb6989cef1d09957fc0f6f7a512b26.camel@gmail.com>
Date: Mon, 07 Oct 2024 17:15:13 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Matteo Martelli <matteomartelli3@...il.com>, Jonathan Cameron
 <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Alisa-Dariana Roman
 <alisa.roman@...log.com>,  Christian Eggers <ceggers@...i.de>, Peter Rosin
 <peda@...ntia.se>, Paul Cercueil <paul@...pouillou.net>, Sebastian Reichel
 <sre@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mips@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/7] iio: inkern: copy/release available info from
 producer

On Mon, 2024-10-07 at 10:37 +0200, Matteo Martelli wrote:
> Consumers need to call the read_avail_release_resource after reading the
> available info. To call the release with info_exists locked, copy the
> available info from the producer and immediately call its release
> callback. With this change, users of iio_read_avail_channel_raw() and
> iio_read_avail_channel_attribute() must free the copied avail info after
> calling them.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@...il.com>
> ---
>  drivers/iio/inkern.c         | 64 +++++++++++++++++++++++++++++++++-----------
>  include/linux/iio/consumer.h |  4 +--
>  2 files changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index
> 7f325b3ed08fae6674245312cf8f57bb151006c0..cc65ef79451e5aa2cea447e168007a447ffc0d91
> 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -760,9 +760,25 @@ static int iio_channel_read_avail(struct iio_channel *chan,
>  	if (!iio_channel_has_available(chan->channel, info))
>  		return -EINVAL;
>  
> -	if (iio_info->read_avail)
> -		return iio_info->read_avail(chan->indio_dev, chan->channel,
> -					    vals, type, length, info);
> +	if (iio_info->read_avail) {
> +		const int *vals_tmp;
> +		int ret;
> +
> +		ret = iio_info->read_avail(chan->indio_dev, chan->channel,
> +					   &vals_tmp, type, length, info);
> +		if (ret < 0)
> +			return ret;
> +
> +		*vals = kmemdup_array(vals_tmp, *length, sizeof(int), GFP_KERNEL);
> +		if (!*vals)
> +			return -ENOMEM;
> +

Not a big deal but I would likely prefer to avoid yet another copy. If I'm
understanding things correctly, I would rather create an inkern wrapper API like 
iio_channel_read_avail_release_resource() - maybe something with a smaller name :).
Hence, the lifetime of the data would be only controlled by the producer of it. It
would also produce a smaller diff (I think). I just find it a bit confusing that we
duplicate the data in here and the producer also duplicates it on the ->read_avail()
call. Another advantage I see is that often the available data is indeed const in
which case no kmemdup_array() is needed at all.

- Nuno Sá