diff mbox series

[v4,2/5] iio: consumers: copy/release available info from producer to fix race

Message ID 20241018-iio-read-avail-release-v4-2-53c8ac618585@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: fix possible race condition during access of available info lists | expand

Commit Message

Matteo Martelli Oct. 18, 2024, 10:16 a.m. UTC
Consumers need to call the producer's read_avail_release_resource()
callback after reading producer's available info. To avoid a race
condition with the producer unregistration, change inkern
iio_channel_read_avail() so that it copies the available info from the
producer and immediately calls its release callback with info_exists
locked.

Also, modify the users of iio_read_avail_channel_raw() and
iio_read_avail_channel_attribute() to free the copied available buffers
after calling these functions.

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 drivers/iio/afe/iio-rescale.c          |  8 ++++++++
 drivers/iio/dac/dpot-dac.c             |  8 ++++++++
 drivers/iio/inkern.c                   | 34 ++++++++++++++++++++++++++++------
 drivers/iio/multiplexer/iio-mux.c      |  8 ++++++++
 drivers/power/supply/ingenic-battery.c |  4 +++-
 include/linux/iio/consumer.h           |  4 ++--
 6 files changed, 57 insertions(+), 9 deletions(-)

Comments

Jonathan Cameron Oct. 19, 2024, 12:04 p.m. UTC | #1
On Fri, 18 Oct 2024 12:16:41 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Consumers need to call the producer's read_avail_release_resource()
> callback after reading producer's available info. To avoid a race
> condition with the producer unregistration, change inkern
> iio_channel_read_avail() so that it copies the available info from the
> producer and immediately calls its release callback with info_exists
> locked.
> 
> Also, modify the users of iio_read_avail_channel_raw() and
> iio_read_avail_channel_attribute() to free the copied available buffers
> after calling these functions.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
Hi Matteo,

The cleanup.h stuff is new and comes with footguns. As such the
'rules' applied are perhaps stricter than then will be in the long term.
https://lore.kernel.org/all/172294149613.2215.3274492813920223809.tip-bot2@tip-bot2/
is what we have today. Particularly the last few paragraphs on usage.

> @@ -857,7 +879,7 @@ static int iio_channel_read_min(struct iio_channel *chan,
>  				int *val, int *val2, int *type,
>  				enum iio_chan_info_enum info)
>  {
> -	const int *vals;
> +	const int *vals __free(kfree) = NULL;

Unlike below this one is 'almost' ok because there isn't much below. However,
still not good because of the risk of future code putting something in between.
At very minimum move it down to just before the place it's allocated.

It's a bit messy but maybe what we need is:

int *iio_read_avail_channel_attribute_retvals(struct iio_channel *chan,
				     	      int *type, int *length,
				              enum iio_chan_info_enum attr)
{
	int *vals;
	int ret;

	ret = iio_read_avail_channel_attribute(chan, &vals, type, len, attr;
	if (ret)
		return ERR_PTR(ret);

	return vals;
}

Then you can do
	const int *vals __free(kfree) =
		iio_channel_read_avail_retvals(chan, type, &length, info);

	if (IS_ERR(vals))
		...

and obey the suggested style for cleanup.h usage.

Would need some clear comments on why it exists though!
(+ maybe a better name)


>  	int length;
>  	int ret;
>  

> diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..6b7856e69f5fb7b8b73166b9b6825f4af7b19129 100644
> --- a/drivers/power/supply/ingenic-battery.c
> +++ b/drivers/power/supply/ingenic-battery.c
> @@ -6,12 +6,14 @@
>   * based on drivers/power/supply/jz4740-battery.c
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/property.h>
> +#include <linux/slab.h>
>  
>  struct ingenic_battery {
>  	struct device *dev;
> @@ -62,7 +64,7 @@ static int ingenic_battery_get_property(struct power_supply *psy,
>   */
>  static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>  {
> -	const int *scale_raw;
> +	const int *scale_raw __free(kfree) = NULL;
This isn't a good pattern as per the docs I just replied to v3 with.
Whilst the code is functionally correct today, it is fragile for the reasons
in those docs.

>  	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
>  	u64 max_mV;
>
diff mbox series

Patch

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 56e5913ab82d1c045c9ca27012008a4495502cbf..78bb86c291706748b4072a484532ad20c415ff9f 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -249,9 +249,17 @@  static int rescale_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static void rescale_read_avail_release_res(struct iio_dev *indio_dev,
+					   struct iio_chan_spec const *chan,
+					   const int *vals, long mask)
+{
+	kfree(vals);
+}
+
 static const struct iio_info rescale_info = {
 	.read_raw = rescale_read_raw,
 	.read_avail = rescale_read_avail,
+	.read_avail_release_resource = rescale_read_avail_release_res,
 };
 
 static ssize_t rescale_read_ext_info(struct iio_dev *indio_dev,
diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
index f36f10bfb6be7863a56b911b5f58671ef530c977..43d68e17fc3a5fca59fad6ccf818eeadfecdb8c1 100644
--- a/drivers/iio/dac/dpot-dac.c
+++ b/drivers/iio/dac/dpot-dac.c
@@ -108,6 +108,13 @@  static int dpot_dac_read_avail(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static void dpot_dac_read_avail_release_res(struct iio_dev *indio_dev,
+					    struct iio_chan_spec const *chan,
+					    const int *vals, long mask)
+{
+	kfree(vals);
+}
+
 static int dpot_dac_write_raw(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      int val, int val2, long mask)
@@ -125,6 +132,7 @@  static int dpot_dac_write_raw(struct iio_dev *indio_dev,
 static const struct iio_info dpot_dac_info = {
 	.read_raw = dpot_dac_read_raw,
 	.read_avail = dpot_dac_read_avail,
+	.read_avail_release_resource = dpot_dac_read_avail_release_res,
 	.write_raw = dpot_dac_write_raw,
 };
 
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 7f325b3ed08fae6674245312cf8f57bb151006c0..9af6b33eb8d178ee7b3f5d417ccfa4d0f87835bc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -760,9 +760,29 @@  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;
+
+		/*
+		 * Copy the producer's avail buffer with lock_exists locked to
+		 * avoid possible race with producer unregistration.
+		 */
+		*vals = kmemdup_array(vals_tmp, *length, sizeof(int), GFP_KERNEL);
+		if (!*vals)
+			return -ENOMEM;
+
+		if (iio_info->read_avail_release_resource)
+			iio_info->read_avail_release_resource(
+				chan->indio_dev, chan->channel, vals_tmp, info);
+
+		return ret;
+	}
 	return -EINVAL;
 }
 
@@ -789,9 +809,11 @@  int iio_read_avail_channel_raw(struct iio_channel *chan,
 	ret = iio_read_avail_channel_attribute(chan, vals, &type, length,
 					       IIO_CHAN_INFO_RAW);
 
-	if (ret >= 0 && type != IIO_VAL_INT)
+	if (ret >= 0 && type != IIO_VAL_INT) {
 		/* raw values are assumed to be IIO_VAL_INT */
+		kfree(*vals);
 		ret = -EINVAL;
+	}
 
 	return ret;
 }
@@ -801,7 +823,7 @@  static int iio_channel_read_max(struct iio_channel *chan,
 				int *val, int *val2, int *type,
 				enum iio_chan_info_enum info)
 {
-	const int *vals;
+	const int *vals __free(kfree) = NULL;
 	int length;
 	int ret;
 
@@ -857,7 +879,7 @@  static int iio_channel_read_min(struct iio_channel *chan,
 				int *val, int *val2, int *type,
 				enum iio_chan_info_enum info)
 {
-	const int *vals;
+	const int *vals __free(kfree) = NULL;
 	int length;
 	int ret;
 
diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..31345437784b01c5d6f8ea70263f4c2574388e7a 100644
--- a/drivers/iio/multiplexer/iio-mux.c
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -142,6 +142,13 @@  static int mux_read_avail(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static void mux_read_avail_release_res(struct iio_dev *indio_dev,
+				       struct iio_chan_spec const *chan,
+				       const int *vals, long mask)
+{
+	kfree(vals);
+}
+
 static int mux_write_raw(struct iio_dev *indio_dev,
 			 struct iio_chan_spec const *chan,
 			 int val, int val2, long mask)
@@ -171,6 +178,7 @@  static int mux_write_raw(struct iio_dev *indio_dev,
 static const struct iio_info mux_info = {
 	.read_raw = mux_read_raw,
 	.read_avail = mux_read_avail,
+	.read_avail_release_resource = mux_read_avail_release_res,
 	.write_raw = mux_write_raw,
 };
 
diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..6b7856e69f5fb7b8b73166b9b6825f4af7b19129 100644
--- a/drivers/power/supply/ingenic-battery.c
+++ b/drivers/power/supply/ingenic-battery.c
@@ -6,12 +6,14 @@ 
  * based on drivers/power/supply/jz4740-battery.c
  */
 
+#include <linux/cleanup.h>
 #include <linux/iio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/property.h>
+#include <linux/slab.h>
 
 struct ingenic_battery {
 	struct device *dev;
@@ -62,7 +64,7 @@  static int ingenic_battery_get_property(struct power_supply *psy,
  */
 static int ingenic_battery_set_scale(struct ingenic_battery *bat)
 {
-	const int *scale_raw;
+	const int *scale_raw __free(kfree) = NULL;
 	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
 	u64 max_mV;
 
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..e3e268d2574b3e01c9412449d90d627de7efcd84 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -316,7 +316,7 @@  int iio_read_min_channel_raw(struct iio_channel *chan, int *val);
 /**
  * iio_read_avail_channel_raw() - read available raw values from a given channel
  * @chan:		The channel being queried.
- * @vals:		Available values read back.
+ * @vals:		Available values read back. Must be freed after use.
  * @length:		Number of entries in vals.
  *
  * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
@@ -334,7 +334,7 @@  int iio_read_avail_channel_raw(struct iio_channel *chan,
 /**
  * iio_read_avail_channel_attribute() - read available channel attribute values
  * @chan:		The channel being queried.
- * @vals:		Available values read back.
+ * @vals:		Available values read back. Must be freed after use.
  * @type:		Type of values read back.
  * @length:		Number of entries in vals.
  * @attribute:		info attribute to be read back.