[PATCH] devres: zero the memory in devm_krealloc() if needed

[PATCH] devres: zero the memory in devm_krealloc() if needed

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

If we're returning the same pointer (when new size is smaller or equal
to the old size) we need to check if the user wants the memory zeroed
and memset() it manually if so.

Fixes: f82485722e5d devres: provide devm_krealloc()
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/base/devres.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 586e9a75c840..e522ad5f8342 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -895,8 +895,12 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
 	 * 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)
+	if (total_new_size <= total_old_size) {
+		if (gfp & __GFP_ZERO)
+			memset(ptr, 0, new_size);
+
 		return ptr;
+	}
 
 	/*
 	 * Otherwise: allocate new, larger chunk. We need to allocate before
-- 
2.29.1

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 01:27:28PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> If we're returning the same pointer (when new size is smaller or equal
> to the old size) we need to check if the user wants the memory zeroed
> and memset() it manually if so.

Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Bartosz Golaszewski]

On Thu, Oct 29, 2020 at 9:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Oct 26, 2020 at 01:27:28PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > If we're returning the same pointer (when new size is smaller or equal
> > to the old size) we need to check if the user wants the memory zeroed
> > and memset() it manually if so.
>
> Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().
>

This is kind of a gray area in original krealloc() too and I want to
submit a patch for mm too. Right now krealloc ignores the __GFP_ZERO
flag if new_size <= old_size but zeroes the memory if new_size >
old_size. This should be consistent - either ignore __GFP_ZERO or
don't ignore it in both cases. I think that not ignoring it is better
- if user passes it then it's for a reason.

Bartosz

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Andy Shevchenko]

On Fri, Oct 30, 2020 at 10:03:50AM +0100, Bartosz Golaszewski wrote:
> On Thu, Oct 29, 2020 at 9:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Oct 26, 2020 at 01:27:28PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > If we're returning the same pointer (when new size is smaller or equal
> > > to the old size) we need to check if the user wants the memory zeroed
> > > and memset() it manually if so.
> >
> > Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().
> 
> This is kind of a gray area in original krealloc() too and I want to
> submit a patch for mm too. Right now krealloc ignores the __GFP_ZERO
> flag if new_size <= old_size but zeroes the memory if new_size >
> old_size.

> This should be consistent - either ignore __GFP_ZERO or
> don't ignore it in both cases. I think that not ignoring it is better
> - if user passes it then it's for a reason.

Sorry, but I consider in these two choices the best is the former one, i.e.
ignoring, because non-ignoring for sizes less than current is counter the
REalloc() by definition.

Reading realloc(3):

"If the new size is larger than the old size, the added memory will not be
initialized."

So, supports my choice over yours.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Andy Shevchenko]

On Fri, Oct 30, 2020 at 12:57:06PM +0200, Andy Shevchenko wrote:
> On Fri, Oct 30, 2020 at 10:03:50AM +0100, Bartosz Golaszewski wrote:
> > On Thu, Oct 29, 2020 at 9:05 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Oct 26, 2020 at 01:27:28PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > If we're returning the same pointer (when new size is smaller or equal
> > > > to the old size) we need to check if the user wants the memory zeroed
> > > > and memset() it manually if so.
> > >
> > > Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().
> > 
> > This is kind of a gray area in original krealloc() too and I want to
> > submit a patch for mm too. Right now krealloc ignores the __GFP_ZERO
> > flag if new_size <= old_size but zeroes the memory if new_size >
> > old_size.
> 
> > This should be consistent - either ignore __GFP_ZERO or
> > don't ignore it in both cases. I think that not ignoring it is better
> > - if user passes it then it's for a reason.
> 
> Sorry, but I consider in these two choices the best is the former one, i.e.
> ignoring, because non-ignoring for sizes less than current is counter the
> REalloc() by definition.
> 
> Reading realloc(3):
> 
> "If the new size is larger than the old size, the added memory will not be
> initialized."
> 
> So, supports my choice over yours.

Two notes:
 - perhaps kzrealloc() for what you want
 - there is a library call reallocarray() which supports your idea about
   krealloc_array() API in kernel.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Bartosz Golaszewski]

On Fri, Oct 30, 2020 at 11:56 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

[snip]

> > >
> > > Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().
> >
> > This is kind of a gray area in original krealloc() too and I want to
> > submit a patch for mm too. Right now krealloc ignores the __GFP_ZERO
> > flag if new_size <= old_size but zeroes the memory if new_size >
> > old_size.
>
> > This should be consistent - either ignore __GFP_ZERO or
> > don't ignore it in both cases. I think that not ignoring it is better
> > - if user passes it then it's for a reason.
>
> Sorry, but I consider in these two choices the best is the former one, i.e.
> ignoring, because non-ignoring for sizes less than current is counter the
> REalloc() by definition.
>
> Reading realloc(3):
>
> "If the new size is larger than the old size, the added memory will not be
> initialized."
>
> So, supports my choice over yours.

Kernel memory management API is not really orthogonal to the one in
user-space. For example: kmalloc() takes the gfp parameter and if you
pass __GFP_ZERO to it, it zeroes the memory even if user-space
malloc() never does.

On Fri, Oct 30, 2020 at 11:57 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

[snip]

>
> Two notes:
>  - perhaps kzrealloc() for what you want

It could be used as a helper wrapper around krealloc() but kzalloc()
is already a simple kmalloc() with additional __GFP_ZERO flag passed.
This is why I think krealloc() should honor __GFP_ZERO.

>  - there is a library call reallocarray() which supports your idea about
>    krealloc_array() API in kernel.
>

reallocarray() is a bsd extension. I'd stick to krealloc_array()
naming in the kernel.

Bartosz

Re: [PATCH] devres: zero the memory in devm_krealloc() if needed

[Bartosz Golaszewski]

On Fri, Oct 30, 2020 at 12:03 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Fri, Oct 30, 2020 at 11:56 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
>
> [snip]
>
> > > >
> > > > Any use case? Because to me it sounds contradictory to the whole idea of [k]realloc().
> > >
> > > This is kind of a gray area in original krealloc() too and I want to
> > > submit a patch for mm too. Right now krealloc ignores the __GFP_ZERO
> > > flag if new_size <= old_size but zeroes the memory if new_size >
> > > old_size.
> >
> > > This should be consistent - either ignore __GFP_ZERO or
> > > don't ignore it in both cases. I think that not ignoring it is better
> > > - if user passes it then it's for a reason.
> >
> > Sorry, but I consider in these two choices the best is the former one, i.e.
> > ignoring, because non-ignoring for sizes less than current is counter the
> > REalloc() by definition.
> >
> > Reading realloc(3):
> >
> > "If the new size is larger than the old size, the added memory will not be
> > initialized."
> >
> > So, supports my choice over yours.
>
> Kernel memory management API is not really orthogonal to the one in
> user-space. For example: kmalloc() takes the gfp parameter and if you
> pass __GFP_ZERO to it, it zeroes the memory even if user-space
> malloc() never does.
>

Ok so I was wrong - it turns out that krealloc() is consistent in
ignoring __GFP_ZERO after all. In that case this patch can be dropped.

Bartosz