diff mbox series

[v7,1/3] devres: provide devm_krealloc()

Message ID 20200817170535.17041-2-brgl@bgdev.pl
State New
Headers show
Series devres: provide and use devm_krealloc() | expand

Commit Message

Bartosz Golaszewski Aug. 17, 2020, 5:05 p.m. UTC
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                         | 93 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 96 insertions(+)

Comments

Andy Shevchenko Aug. 17, 2020, 5:39 p.m. UTC | #1
On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski 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().

Thanks for an update! My comments / questions below.

...

> +static struct devres *to_devres(void *data)
> +{
> +	return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> +						    ARCH_KMALLOC_MINALIGN));

Do you really need both explicit castings?

> +}

...

> +	total_old_size = ksize(to_devres(ptr));

But how you can guarantee this pointer:
 - belongs to devres,
 - hasn't gone while you run a ksize()?

...

> +	new_dr = alloc_dr(devm_kmalloc_release,
> +			  total_new_size, gfp, dev_to_node(dev));

Can you move some parameters to the previous line?

> +	if (!new_dr)
> +		return NULL;

...

> +	spin_lock_irqsave(&dev->devres_lock, flags);
> +
> +	old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> +	if (!old_dr) {
> +		spin_unlock_irqrestore(&dev->devres_lock, flags);
> +		devres_free(new_dr);
> +		WARN(1, "Memory chunk not managed or managed by a different device.");
> +		return NULL;
> +	}
> +
> +	replace_dr(dev, &old_dr->node, &new_dr->node);
> +
> +	spin_unlock_irqrestore(&dev->devres_lock, flags);
> +
> +	memcpy(new_dr->data, old_dr->data, devres_data_size(total_old_size));

But new_dr may concurrently gone at this point, no? It means memcpy() should be
under spin lock.
Bartosz Golaszewski Aug. 17, 2020, 8:02 p.m. UTC | #2
On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski 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().
>
> Thanks for an update! My comments / questions below.
>
> ...
>
> > +static struct devres *to_devres(void *data)
> > +{
> > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > +                                                 ARCH_KMALLOC_MINALIGN));
>
> Do you really need both explicit castings?
>

Yeah, we can probably drop the (struct devres *) here.

> > +}
>
> ...
>
> > +     total_old_size = ksize(to_devres(ptr));
>
> But how you can guarantee this pointer:
>  - belongs to devres,

We can only check if a chunk is dynamically allocated with ksize() -
it will return 0 if it isn't and I'll add a check for that in the next
iteration. We check whether it's a managed chunk later after taking
the lock.

>  - hasn't gone while you run a ksize()?
>

At some point you need to draw a line. In the end: how do you
guarantee a devres buffer hasn't been freed when you're using it? In
my comment to the previous version of this patch I clarified that we
need to protect all modifications of the devres linked list - we must
not realloc a chunk that contains the links without taking the
spinlock but also we must not call alloc() funcs with GFP_KERNEL with
spinlock taken. The issue we could run into is: someone modifies the
linked list by adding/removing other managed resources, not modifying
this one.

The way this function works now guarantees it but other than that:
it's up to the users to not free memory they're actively using.

> ...
>
> > +     new_dr = alloc_dr(devm_kmalloc_release,
> > +                       total_new_size, gfp, dev_to_node(dev));
>
> Can you move some parameters to the previous line?
>

Why though? It's fine this way.

> > +     if (!new_dr)
> > +             return NULL;
>
> ...
>
> > +     spin_lock_irqsave(&dev->devres_lock, flags);
> > +
> > +     old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> > +     if (!old_dr) {
> > +             spin_unlock_irqrestore(&dev->devres_lock, flags);
> > +             devres_free(new_dr);
> > +             WARN(1, "Memory chunk not managed or managed by a different device.");
> > +             return NULL;
> > +     }
> > +
> > +     replace_dr(dev, &old_dr->node, &new_dr->node);
> > +
> > +     spin_unlock_irqrestore(&dev->devres_lock, flags);
> > +
> > +     memcpy(new_dr->data, old_dr->data, devres_data_size(total_old_size));
>
> But new_dr may concurrently gone at this point, no? It means memcpy() should be
> under spin lock.
>

Just as I explained above: we're protecting the linked list, not the
resource itself.

Bartosz
Andy Shevchenko Aug. 18, 2020, 8:25 a.m. UTC | #3
On Mon, Aug 17, 2020 at 10:02:05PM +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

...

> > > +static struct devres *to_devres(void *data)
> > > +{
> > > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > > +                                                 ARCH_KMALLOC_MINALIGN));
> >
> > Do you really need both explicit castings?
> >
> 
> Yeah, we can probably drop the (struct devres *) here.

void * -> u8 * here is also not needed, it is considered byte access IIRC.

> > > +}

...

> >  - hasn't gone while you run a ksize()?

> At some point you need to draw a line. In the end: how do you
> guarantee a devres buffer hasn't been freed when you're using it? In
> my comment to the previous version of this patch I clarified that we
> need to protect all modifications of the devres linked list - we must
> not realloc a chunk that contains the links without taking the
> spinlock but also we must not call alloc() funcs with GFP_KERNEL with
> spinlock taken. The issue we could run into is: someone modifies the
> linked list by adding/removing other managed resources, not modifying
> this one.
> 
> The way this function works now guarantees it but other than that:
> it's up to the users to not free memory they're actively using.

Thanks for clarification. I agree.
Bartosz Golaszewski Aug. 18, 2020, 4:27 p.m. UTC | #4
On Tue, Aug 18, 2020 at 10:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 17, 2020 at 10:02:05PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> ...
>
> > > > +static struct devres *to_devres(void *data)
> > > > +{
> > > > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > > > +                                                 ARCH_KMALLOC_MINALIGN));
> > >
> > > Do you really need both explicit castings?
> > >
> >
> > Yeah, we can probably drop the (struct devres *) here.
>
> void * -> u8 * here is also not needed, it is considered byte access IIRC.
>

Actually it turns out that while we don't need the (void *) -> (u8 *)
casting, we must cast to (struct devres *) or the following error is
produced:

drivers/base/devres.c: In function ‘to_devres’:
drivers/base/devres.c:41:21: error: returning ‘u8 *’ {aka ‘unsigned
char *’} from a function with incompatible return type ‘struct devres
*’ [-Werror=incompatible-pointer-types]
  return ((u8 *)data - ALIGN(sizeof(struct devres),
         ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        ARCH_KMALLOC_MINALIGN));
        ~~~~~~~~~~~~~~~~~~~~~~~

Bart
Andy Shevchenko Aug. 18, 2020, 5:10 p.m. UTC | #5
On Tue, Aug 18, 2020 at 06:27:12PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 18, 2020 at 10:40 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Aug 17, 2020 at 10:02:05PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > ...
> >
> > > > > +static struct devres *to_devres(void *data)
> > > > > +{
> > > > > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > > > > +                                                 ARCH_KMALLOC_MINALIGN));
> > > >
> > > > Do you really need both explicit castings?
> > > >
> > >
> > > Yeah, we can probably drop the (struct devres *) here.
> >
> > void * -> u8 * here is also not needed, it is considered byte access IIRC.
> >
> 
> Actually it turns out that while we don't need the (void *) -> (u8 *)
> casting, we must cast to (struct devres *) or the following error is
> produced:
> 
> drivers/base/devres.c: In function ‘to_devres’:
> drivers/base/devres.c:41:21: error: returning ‘u8 *’ {aka ‘unsigned
> char *’} from a function with incompatible return type ‘struct devres
> *’ [-Werror=incompatible-pointer-types]
>   return ((u8 *)data - ALIGN(sizeof(struct devres),
>          ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         ARCH_KMALLOC_MINALIGN));
>         ~~~~~~~~~~~~~~~~~~~~~~~

Of course, you have to drop u8 * casting as well.
Bartosz Golaszewski Aug. 18, 2020, 6:13 p.m. UTC | #6
On Tue, Aug 18, 2020 at 7:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 18, 2020 at 06:27:12PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Aug 18, 2020 at 10:40 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Aug 17, 2020 at 10:02:05PM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > ...
> > >
> > > > > > +static struct devres *to_devres(void *data)
> > > > > > +{
> > > > > > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > > > > > +                                                 ARCH_KMALLOC_MINALIGN));
> > > > >
> > > > > Do you really need both explicit castings?
> > > > >
> > > >
> > > > Yeah, we can probably drop the (struct devres *) here.
> > >
> > > void * -> u8 * here is also not needed, it is considered byte access IIRC.
> > >
> >
> > Actually it turns out that while we don't need the (void *) -> (u8 *)
> > casting, we must cast to (struct devres *) or the following error is
> > produced:
> >
> > drivers/base/devres.c: In function ‘to_devres’:
> > drivers/base/devres.c:41:21: error: returning ‘u8 *’ {aka ‘unsigned
> > char *’} from a function with incompatible return type ‘struct devres
> > *’ [-Werror=incompatible-pointer-types]
> >   return ((u8 *)data - ALIGN(sizeof(struct devres),
> >          ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >         ARCH_KMALLOC_MINALIGN));
> >         ~~~~~~~~~~~~~~~~~~~~~~~
>
> Of course, you have to drop u8 * casting as well.
>

Yes, of course. Duh

Bart
Andy Shevchenko Aug. 19, 2020, 9:14 a.m. UTC | #7
On Tue, Aug 18, 2020 at 08:13:10PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 18, 2020 at 7:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 18, 2020 at 06:27:12PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Aug 18, 2020 at 10:40 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 17, 2020 at 10:02:05PM +0200, Bartosz Golaszewski wrote:
> > > > > On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote:
> > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

...

> > > > > > > +static struct devres *to_devres(void *data)
> > > > > > > +{
> > > > > > > +     return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
> > > > > > > +                                                 ARCH_KMALLOC_MINALIGN));
> > > > > >
> > > > > > Do you really need both explicit castings?
> > > > > >
> > > > >
> > > > > Yeah, we can probably drop the (struct devres *) here.
> > > >
> > > > void * -> u8 * here is also not needed, it is considered byte access IIRC.
> > > >
> > >
> > > Actually it turns out that while we don't need the (void *) -> (u8 *)
> > > casting, we must cast to (struct devres *) or the following error is
> > > produced:
> > >
> > > drivers/base/devres.c: In function ‘to_devres’:
> > > drivers/base/devres.c:41:21: error: returning ‘u8 *’ {aka ‘unsigned
> > > char *’} from a function with incompatible return type ‘struct devres
> > > *’ [-Werror=incompatible-pointer-types]
> > >   return ((u8 *)data - ALIGN(sizeof(struct devres),
> > >          ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >         ARCH_KMALLOC_MINALIGN));
> > >         ~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Of course, you have to drop u8 * casting as well.
> >
> 
> Yes, of course. Duh

With this addressed (and don't forget to remove also unneeded parentheses),
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thanks!
diff mbox series

Patch

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..bfe46e83147e 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -36,6 +36,17 @@  struct devres {
 	u8 __aligned(ARCH_KMALLOC_MINALIGN) data[];
 };
 
+static struct devres *to_devres(void *data)
+{
+	return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres),
+						    ARCH_KMALLOC_MINALIGN));
+}
+
+static size_t devres_data_size(size_t total_size)
+{
+	return total_size - ALIGN(sizeof(struct devres), ARCH_KMALLOC_MINALIGN);
+}
+
 struct devres_group {
 	struct devres_node		node[2];
 	void				*id;
@@ -126,6 +137,14 @@  static void add_dr(struct device *dev, struct devres_node *node)
 	list_add_tail(&node->entry, &dev->devres_head);
 }
 
+static void replace_dr(struct device *dev,
+		       struct devres_node *old, struct devres_node *new)
+{
+	devres_log(dev, old, "REPLACE");
+	BUG_ON(!list_empty(&new->entry));
+	list_replace(&old->entry, &new->entry);
+}
+
 #ifdef CONFIG_DEBUG_DEVRES
 void * __devres_alloc_node(dr_release_t release, size_t size, gfp_t gfp, int nid,
 		      const char *name)
@@ -837,6 +856,80 @@  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)
+{
+	size_t total_new_size, total_old_size;
+	struct devres *old_dr, *new_dr;
+	unsigned long flags;
+
+	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_new_size))
+		return NULL;
+
+	total_old_size = ksize(to_devres(ptr));
+
+	/*
+	 * If new size is smaller or equal to the actual number of bytes
+	 * allocated previously - just return the same pointer.
+	 */
+	if (total_new_size <= total_old_size)
+		return ptr;
+
+	new_dr = alloc_dr(devm_kmalloc_release,
+			  total_new_size, gfp, dev_to_node(dev));
+	if (!new_dr)
+		return NULL;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+
+	old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+	if (!old_dr) {
+		spin_unlock_irqrestore(&dev->devres_lock, flags);
+		devres_free(new_dr);
+		WARN(1, "Memory chunk not managed or managed by a different device.");
+		return NULL;
+	}
+
+	replace_dr(dev, &old_dr->node, &new_dr->node);
+
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+
+	memcpy(new_dr->data, old_dr->data, devres_data_size(total_old_size));
+	kfree(old_dr);
+	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 ca18da4768e3..5da7d5f0a7ff 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,