linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/3] devres: provide and use devm_krealloc()
@ 2020-08-02  8:34 Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2020-08-02  8:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Michal Simek, Greg Kroah-Hartman,
	Guenter Roeck, Andy Shevchenko
  Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hi Greg, Andy,

this is a reworked version of devm_krealloc(). This time without the risk
of sleeping with spinlock taken. I noticed that devres_lock is only there
to protect the integrity of the devres list - we can assume users are sane
enough to not devm_kfree() chunks that they devm_krealloc() at the same time
so we only take the lock when manipulating the list now.

I dropped Andy's review tag due to this change.

===

Regular krealloc() obviously can't work with managed memory. This series
implements devm_krealloc() and adds the first users with hope that this
helper will be adopted by other drivers currently using non-managed
krealloc().

Some additional changes to the code modified by main patches are included.

v1 -> v2:
- remove leftover call to hwmon_device_unregister() from pmbus_core.c
- add a patch extending devm_kmalloc() to handle zero size case
- use WARN_ON() instead of WARN_ONCE() in devm_krealloc() when passed
  a pointer to non-managed memory
- correctly handle the case when devm_krealloc() is passed a pointer to
  memory in .rodata (potentially returned by devm_kstrdup_const())
- correctly handle ZERO_SIZE_PTR passed as the ptr argument in devm_krealloc()

v2 -> v3:
- drop already applied patches
- collect Acks
- add an additional user in iio

v3 -> v4:
- add the kerneldoc for devm_krealloc()
- WARN() outside of spinlock
- rename local variable

v4 -> v5:
- tweak the kerneldoc

v5 -> v6:
- tweak the devres_lock handling in devm_krealloc()

Bartosz Golaszewski (3):
  devres: provide devm_krealloc()
  hwmon: pmbus: use more devres helpers
  iio: adc: xilinx-xadc: use devm_krealloc()

 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 65 +++++++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c              | 28 +++-----
 drivers/iio/adc/xilinx-xadc-core.c            | 16 ++---
 include/linux/device.h                        |  2 +
 5 files changed, 85 insertions(+), 27 deletions(-)

-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 1/3] devres: provide devm_krealloc()
  2020-08-02  8:34 [PATCH v6 0/3] devres: provide and use devm_krealloc() Bartosz Golaszewski
@ 2020-08-02  8:34 ` Bartosz Golaszewski
  2020-08-02 10:42   ` Andy Shevchenko
  2020-08-02  8:34 ` [PATCH v6 2/3] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 3/3] iio: adc: xilinx-xadc: use devm_krealloc() Bartosz Golaszewski
  2 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2020-08-02  8:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Michal Simek, Greg Kroah-Hartman,
	Guenter Roeck, Andy Shevchenko
  Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Implement the managed variant of krealloc(). This function works with
all memory allocated by devm_kmalloc() (or devres functions using it
implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 65 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 68 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index eaaaafc21134..f318a5c0033c 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -354,6 +354,7 @@ MEM
   devm_kmalloc()
   devm_kmalloc_array()
   devm_kmemdup()
+  devm_krealloc()
   devm_kstrdup()
   devm_kvasprintf()
   devm_kzalloc()
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index ed615d3b9cf1..68a5a7201065 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -837,6 +837,71 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(devm_kmalloc);
 
+/**
+ * devm_krealloc - Resource-managed krealloc()
+ * @dev: Device to re-allocate memory for
+ * @ptr: Pointer to the memory chunk to re-allocate
+ * @new_size: New allocation size
+ * @gfp: Allocation gfp flags
+ *
+ * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc().
+ * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR,
+ * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the
+ * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't
+ * change the order in which the release callback for the re-alloc'ed devres
+ * will be called (except when falling back to devm_kmalloc() or when freeing
+ * resources when new_size is zero). The contents of the memory are preserved
+ * up to the lesser of new and old sizes.
+ */
+void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
+{
+	struct devres *old_dr, *new_dr;
+	struct list_head old_head;
+	unsigned long flags;
+	size_t total_size;
+
+	if (unlikely(!new_size)) {
+		devm_kfree(dev, ptr);
+		return ZERO_SIZE_PTR;
+	}
+
+	if (unlikely(ZERO_OR_NULL_PTR(ptr)))
+		return devm_kmalloc(dev, new_size, gfp);
+
+	if (WARN_ON(is_kernel_rodata((unsigned long)ptr)))
+		/*
+		 * We cannot reliably realloc a const string returned by
+		 * devm_kstrdup_const().
+		 */
+		return NULL;
+
+	if (!check_dr_size(new_size, &total_size))
+		return NULL;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+	old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	if (!old_dr) {
+		WARN(1, "Memory chunk not managed or managed by a different device.");
+		return NULL;
+	}
+
+	old_head = old_dr->node.entry;
+
+	new_dr = krealloc(old_dr, total_size, gfp);
+	if (!new_dr)
+		return NULL;
+
+	if (new_dr != old_dr) {
+		spin_lock_irqsave(&dev->devres_lock, flags);
+		list_replace(&old_head, &new_dr->node.entry);
+		spin_unlock_irqrestore(&dev->devres_lock, flags);
+	}
+
+	return new_dr->data;
+}
+EXPORT_SYMBOL_GPL(devm_krealloc);
+
 /**
  * devm_kstrdup - Allocate resource managed space and
  *                copy an existing string into that.
diff --git a/include/linux/device.h b/include/linux/device.h
index 79ce404619e6..51fb33400d7a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -206,6 +206,8 @@ int devres_release_group(struct device *dev, void *id);
 
 /* managed devm_k.alloc/kfree for device drivers */
 void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+void *devm_krealloc(struct device *dev, void *ptr, size_t size,
+		    gfp_t gfp) __must_check;
 __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
 				     const char *fmt, va_list ap) __malloc;
 __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 2/3] hwmon: pmbus: use more devres helpers
  2020-08-02  8:34 [PATCH v6 0/3] devres: provide and use devm_krealloc() Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
@ 2020-08-02  8:34 ` Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 3/3] iio: adc: xilinx-xadc: use devm_krealloc() Bartosz Golaszewski
  2 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2020-08-02  8:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Michal Simek, Greg Kroah-Hartman,
	Guenter Roeck, Andy Shevchenko
  Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Shrink pmbus code by using devm_hwmon_device_register_with_groups()
and devm_krealloc() instead of their non-managed variants.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 44535add3a4a..91839979cf6c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1018,9 +1018,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
 		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-		void *new_attrs = krealloc(data->group.attrs,
-					   new_max_attrs * sizeof(void *),
-					   GFP_KERNEL);
+		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
+						new_max_attrs * sizeof(void *),
+						GFP_KERNEL);
 		if (!new_attrs)
 			return -ENOMEM;
 		data->group.attrs = new_attrs;
@@ -2534,7 +2534,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 
 	ret = pmbus_find_attributes(client, data);
 	if (ret)
-		goto out_kfree;
+		return ret;
 
 	/*
 	 * If there are no attributes, something is wrong.
@@ -2542,35 +2542,27 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	 */
 	if (!data->num_attributes) {
 		dev_err(dev, "No attributes found\n");
-		ret = -ENODEV;
-		goto out_kfree;
+		return -ENODEV;
 	}
 
 	data->groups[0] = &data->group;
 	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
-	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-							    data, data->groups);
+	data->hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+					client->name, data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
 		dev_err(dev, "Failed to register hwmon device\n");
-		goto out_kfree;
+		return PTR_ERR(data->hwmon_dev);
 	}
 
 	ret = pmbus_regulator_register(data);
 	if (ret)
-		goto out_unregister;
+		return ret;
 
 	ret = pmbus_init_debugfs(client, data);
 	if (ret)
 		dev_warn(dev, "Failed to register debugfs\n");
 
 	return 0;
-
-out_unregister:
-	hwmon_device_unregister(data->hwmon_dev);
-out_kfree:
-	kfree(data->group.attrs);
-	return ret;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_probe);
 
@@ -2580,8 +2572,6 @@ int pmbus_do_remove(struct i2c_client *client)
 
 	debugfs_remove_recursive(data->debugfs);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	kfree(data->group.attrs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 3/3] iio: adc: xilinx-xadc: use devm_krealloc()
  2020-08-02  8:34 [PATCH v6 0/3] devres: provide and use devm_krealloc() Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
  2020-08-02  8:34 ` [PATCH v6 2/3] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
@ 2020-08-02  8:34 ` Bartosz Golaszewski
  2 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2020-08-02  8:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Michal Simek, Greg Kroah-Hartman,
	Guenter Roeck, Andy Shevchenko
  Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	Jonathan Cameron

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Use the managed variant of krealloc() and shrink the code a bit.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index d0b7ef296afb..f93c34fe5873 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1092,6 +1092,7 @@ MODULE_DEVICE_TABLE(of, xadc_of_match_table);
 static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	unsigned int *conf)
 {
+	struct device *dev = indio_dev->dev.parent;
 	struct xadc *xadc = iio_priv(indio_dev);
 	struct iio_chan_spec *channels, *chan;
 	struct device_node *chan_node, *child;
@@ -1136,7 +1137,8 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 		*conf |= XADC_CONF0_MUX | XADC_CONF0_CHAN(ext_mux_chan);
 	}
 
-	channels = kmemdup(xadc_channels, sizeof(xadc_channels), GFP_KERNEL);
+	channels = devm_kmemdup(dev, xadc_channels,
+				sizeof(xadc_channels), GFP_KERNEL);
 	if (!channels)
 		return -ENOMEM;
 
@@ -1172,8 +1174,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	of_node_put(chan_node);
 
 	indio_dev->num_channels = num_channels;
-	indio_dev->channels = krealloc(channels, sizeof(*channels) *
-					num_channels, GFP_KERNEL);
+	indio_dev->channels = devm_krealloc(dev, channels,
+					    sizeof(*channels) * num_channels,
+					    GFP_KERNEL);
 	/* If we can't resize the channels array, just use the original */
 	if (!indio_dev->channels)
 		indio_dev->channels = channels;
@@ -1225,14 +1228,14 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = xadc_parse_dt(indio_dev, pdev->dev.of_node, &conf0);
 	if (ret)
-		goto err_device_free;
+		return ret;
 
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
 		ret = iio_triggered_buffer_setup(indio_dev,
 			&iio_pollfunc_store_time, &xadc_trigger_handler,
 			&xadc_buffer_ops);
 		if (ret)
-			goto err_device_free;
+			return ret;
 
 		xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
 		if (IS_ERR(xadc->convst_trigger)) {
@@ -1350,8 +1353,6 @@ static int xadc_probe(struct platform_device *pdev)
 err_triggered_buffer_cleanup:
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
 		iio_triggered_buffer_cleanup(indio_dev);
-err_device_free:
-	kfree(indio_dev->channels);
 
 	return ret;
 }
@@ -1371,7 +1372,6 @@ static int xadc_remove(struct platform_device *pdev)
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
 	clk_disable_unprepare(xadc->clk);
 	kfree(xadc->data);
-	kfree(indio_dev->channels);
 
 	return 0;
 }
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] devres: provide devm_krealloc()
  2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
@ 2020-08-02 10:42   ` Andy Shevchenko
  2020-08-03 19:31     ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-08-02 10:42 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-arm Mailing List, Lars-Peter Clausen, linux-iio,
	Greg Kroah-Hartman, Michal Simek, Linux Kernel Mailing List,
	Bartosz Golaszewski, Guenter Roeck, Peter Meerwald-Stadler,
	Hartmut Knaack, Andy Shevchenko, Jonathan Cameron

On Sun, Aug 2, 2020 at 11:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Implement the managed variant of krealloc(). This function works with
> all memory allocated by devm_kmalloc() (or devres functions using it
> implicitly like devm_kmemdup(), devm_kstrdup() etc.).
>
> Managed realloc'ed chunks can be manually released with devm_kfree().

Some thoughts below. You may ignore nit-picks, of course :-)

...

> + * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc().
> + * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR,

> + * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the

equivalent for

> + * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't
> + * change the order in which the release callback for the re-alloc'ed devres
> + * will be called (except when falling back to devm_kmalloc() or when freeing
> + * resources when new_size is zero). The contents of the memory are preserved
> + * up to the lesser of new and old sizes.

Might deserve to say about pointers to RO, but see below.

...

> +       if (WARN_ON(is_kernel_rodata((unsigned long)ptr)))
> +               /*
> +                * We cannot reliably realloc a const string returned by
> +                * devm_kstrdup_const().
> +                */
> +               return NULL;

I was thinking about this bit... Shouldn't we rather issue a simple
dev_warn() and return the existing pointer?
For example in some cases we might want to have resources coming
either from heap or from constant. Then, if at some circumstances we
would like to extend that memory (only for non-constant cases) we
would need to manage this ourselves. Otherwise we may simply call
krealloc().
It seems that devm_kstrdup_const returns an initial pointer. Getting
NULL is kinda inconvenient (and actually dev_warn() might also be
quite a noise, however I would give a message to the user, because
it's something worth checking).

...

> +       spin_lock_irqsave(&dev->devres_lock, flags);
> +       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> +       spin_unlock_irqrestore(&dev->devres_lock, flags);

> +       if (!old_dr) {

I would have this under spin lock b/c of below.

> +               WARN(1, "Memory chunk not managed or managed by a different device.");
> +               return NULL;
> +       }

> +       old_head = old_dr->node.entry;

This would be still better to be under spin lock.

> +       new_dr = krealloc(old_dr, total_size, gfp);
> +       if (!new_dr)
> +               return NULL;

And perhaps spin lock taken already here.

> +       if (new_dr != old_dr) {
> +               spin_lock_irqsave(&dev->devres_lock, flags);
> +               list_replace(&old_head, &new_dr->node.entry);
> +               spin_unlock_irqrestore(&dev->devres_lock, flags);
> +       }

Yes, I understand that covering more code under spin lock does not fix
any potential race, but at least it minimizes scope of the code that
is not under it to see exactly what is problematic.

I probably will think more about a better approach to avoid potential races.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] devres: provide devm_krealloc()
  2020-08-02 10:42   ` Andy Shevchenko
@ 2020-08-03 19:31     ` Bartosz Golaszewski
  0 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2020-08-03 19:31 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-arm Mailing List, Lars-Peter Clausen, linux-iio,
	Greg Kroah-Hartman, Bartosz Golaszewski, Michal Simek,
	Linux Kernel Mailing List, Guenter Roeck, Peter Meerwald-Stadler,
	Hartmut Knaack, Andy Shevchenko, Jonathan Cameron

On Sun, Aug 2, 2020 at 12:42 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>

[snip]

>
> I was thinking about this bit... Shouldn't we rather issue a simple
> dev_warn() and return the existing pointer?
> For example in some cases we might want to have resources coming
> either from heap or from constant. Then, if at some circumstances we
> would like to extend that memory (only for non-constant cases) we
> would need to manage this ourselves. Otherwise we may simply call
> krealloc().
> It seems that devm_kstrdup_const returns an initial pointer. Getting
> NULL is kinda inconvenient (and actually dev_warn() might also be
> quite a noise, however I would give a message to the user, because
> it's something worth checking).
>

But this is inconsistent behavior: if you pass a pointer to ro memory
to devm_krealloc() it will not resize it but by returning a valid
pointer it will make you think it did -> you end up writing to ro
memory in good faith.

> ...
>
> > +       spin_lock_irqsave(&dev->devres_lock, flags);
> > +       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> > +       spin_unlock_irqrestore(&dev->devres_lock, flags);
>
> > +       if (!old_dr) {
>
> I would have this under spin lock b/c of below.
>
> > +               WARN(1, "Memory chunk not managed or managed by a different device.");
> > +               return NULL;
> > +       }
>
> > +       old_head = old_dr->node.entry;
>
> This would be still better to be under spin lock.
>
> > +       new_dr = krealloc(old_dr, total_size, gfp);
> > +       if (!new_dr)
> > +               return NULL;
>
> And perhaps spin lock taken already here.
>
> > +       if (new_dr != old_dr) {
> > +               spin_lock_irqsave(&dev->devres_lock, flags);
> > +               list_replace(&old_head, &new_dr->node.entry);
> > +               spin_unlock_irqrestore(&dev->devres_lock, flags);
> > +       }
>
> Yes, I understand that covering more code under spin lock does not fix
> any potential race, but at least it minimizes scope of the code that
> is not under it to see exactly what is problematic.
>
> I probably will think more about a better approach to avoid potential races.

My thinking behind this was this: we already have users who call
devres_find() and do something with the retrieved resources without
holding the devres_lock - so it's assumed that users are sane enough
to not be getting in each other's way. Now I see that the difference
is that here we're accessing the list node and it can change if
another thread is adding a different devres to the same device. So
this should definitely be protected somehow.

I think that we may have to give up using real krealloc() and instead
just reimplement its behavior in the following way:

Still outside of spinlock check if the new total size is smaller or
equal to the previous one. If so: return the same pointer. If not:
allocate a new devres as if it were for devm_kmalloc() but don't add
it to the list yet. Take the spinlock - check if we can find the
devres - if not: kfree() the new and old chunk and return NULL. If
yes: copy the contents of the devres node into the new chunk as well
as the memory contents. Replace the old one on the list and free it.
Release spinlock and return.

Does that work?

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-08-03 19:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-02  8:34 [PATCH v6 0/3] devres: provide and use devm_krealloc() Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
2020-08-02 10:42   ` Andy Shevchenko
2020-08-03 19:31     ` Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 2/3] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 3/3] iio: adc: xilinx-xadc: use devm_krealloc() Bartosz Golaszewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).