[1/1] clk: tegra: tegra124-emc: Fix possible memory leak
diff mbox series

Message ID 20210617082759.1008-1-thunder.leizhen@huawei.com
State New, archived
Headers show
Series
  • [1/1] clk: tegra: tegra124-emc: Fix possible memory leak
Related show

Commit Message

Zhen Lei June 17, 2021, 8:27 a.m. UTC
When krealloc() fails to expand the memory and returns NULL, the original
memory is not released. In this case, the original "timings" scale should
be maintained.

Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/clk/tegra/clk-tegra124-emc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Stephen Boyd June 25, 2021, 11:31 p.m. UTC | #1
Quoting Zhen Lei (2021-06-17 01:27:59)
> When krealloc() fails to expand the memory and returns NULL, the original
> memory is not released. In this case, the original "timings" scale should
> be maintained.
> 
> Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---

Looks correct, but when does krealloc() return NULL? My read of the
kerneldoc is that it would return the original memory if the new
allocation "failed".

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Zhen Lei June 26, 2021, 1:32 a.m. UTC | #2
On 2021/6/26 7:31, Stephen Boyd wrote:
> Quoting Zhen Lei (2021-06-17 01:27:59)
>> When krealloc() fails to expand the memory and returns NULL, the original
>> memory is not released. In this case, the original "timings" scale should
>> be maintained.
>>
>> Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
> 
> Looks correct, but when does krealloc() return NULL? My read of the
> kerneldoc is that it would return the original memory if the new
> allocation "failed".

That must be the wrong description in the document. For example, the original
100-byte memory needs to be expanded to 200 bytes. If the memory allocation fails,
a non-null pointer is returned. People must think they've applied for it and
continue to use it, the end result is memory crashed.

I don't think the kernel needs to be different from libc's realloc().

The implementation of __do_krealloc() illustrates this as well:
        /* If the object still fits, repoison it precisely. */
        if (ks >= new_size) {
                p = kasan_krealloc((void *)p, new_size, flags);
                return (void *)p;
        }

        ret = kmalloc_track_caller(new_size, flags);			//enlarge, allocate new memory
	if (ret && p) {
                memcpy(ret, kasan_reset_tag(p), ks);			//copy the old content from 'p' to new memory 'ret'
        }

	return ret;							//ret may be NULL, if kmalloc_track_caller() failed



> 
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> 
> .
>
Stephen Boyd June 27, 2021, 11:44 p.m. UTC | #3
Quoting Leizhen (ThunderTown) (2021-06-25 18:32:46)
> 
> 
> On 2021/6/26 7:31, Stephen Boyd wrote:
> > Quoting Zhen Lei (2021-06-17 01:27:59)
> >> When krealloc() fails to expand the memory and returns NULL, the original
> >> memory is not released. In this case, the original "timings" scale should
> >> be maintained.
> >>
> >> Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >> ---
> > 
> > Looks correct, but when does krealloc() return NULL? My read of the
> > kerneldoc is that it would return the original memory if the new
> > allocation "failed".
> 
> That must be the wrong description in the document. For example, the original

Can you fix the kernel doc then?
Dmitry Osipenko June 28, 2021, 12:29 a.m. UTC | #4
28.06.2021 02:44, Stephen Boyd пишет:
> Quoting Leizhen (ThunderTown) (2021-06-25 18:32:46)
>>
>>
>> On 2021/6/26 7:31, Stephen Boyd wrote:
>>> Quoting Zhen Lei (2021-06-17 01:27:59)
>>>> When krealloc() fails to expand the memory and returns NULL, the original
>>>> memory is not released. In this case, the original "timings" scale should
>>>> be maintained.
>>>>
>>>> Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")

The memory is still not released on error and this is not the only one
place in that code which doesn't release memory on error.

All this code is executed only once during early kernel boot, perhaps
not really worthwhile fixing it or at least this should be done properly.

>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>
>>> Looks correct, but when does krealloc() return NULL? My read of the
>>> kerneldoc is that it would return the original memory if the new
>>> allocation "failed".
>>
>> That must be the wrong description in the document. For example, the original
> 
> Can you fix the kernel doc then?
> 

The doc is clearly saying that it returns NULL, am I missing something?

* Return: pointer to the allocated memory or %NULL in case of error
Stephen Boyd June 28, 2021, 1:52 a.m. UTC | #5
Quoting Dmitry Osipenko (2021-06-27 17:29:20)
> 28.06.2021 02:44, Stephen Boyd пишет:
> > Quoting Leizhen (ThunderTown) (2021-06-25 18:32:46)
> >>
> >>
> >> On 2021/6/26 7:31, Stephen Boyd wrote:
> >>> Quoting Zhen Lei (2021-06-17 01:27:59)
> >>>> When krealloc() fails to expand the memory and returns NULL, the original
> >>>> memory is not released. In this case, the original "timings" scale should
> >>>> be maintained.
> >>>>
> >>>> Fixes: 888ca40e2843 ("clk: tegra: emc: Support multiple RAM codes")
> 
> The memory is still not released on error and this is not the only one
> place in that code which doesn't release memory on error.
> 
> All this code is executed only once during early kernel boot, perhaps
> not really worthwhile fixing it or at least this should be done properly.
> 
> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>> ---
> >>>
> >>> Looks correct, but when does krealloc() return NULL? My read of the
> >>> kerneldoc is that it would return the original memory if the new
> >>> allocation "failed".
> >>
> >> That must be the wrong description in the document. For example, the original
> > 
> > Can you fix the kernel doc then?
> > 
> 
> The doc is clearly saying that it returns NULL, am I missing something?
> 
> * Return: pointer to the allocated memory or %NULL in case of error

See the paragraph

 * The contents of the object pointed to are preserved up to the
 * lesser of the new and old sizes (__GFP_ZERO flag is effectively ignored). 

which confused me into thinking the memory that is being reallocated is
guaranteed to be preserved so it would be returned if the larger size
failed. I suppose there's nothing to fix in the kernel-doc, just my
understanding of that paragraph. Maybe it should say

	The __GFP_ZERO flag is effectively ignored as the contents of
	the objected pointed to @p are preserved up to the lesser of the
	@new_size and old size.

Patch
diff mbox series

diff --git a/drivers/clk/tegra/clk-tegra124-emc.c b/drivers/clk/tegra/clk-tegra124-emc.c
index 74c1d894cca8..1a329202b543 100644
--- a/drivers/clk/tegra/clk-tegra124-emc.c
+++ b/drivers/clk/tegra/clk-tegra124-emc.c
@@ -450,11 +450,12 @@  static int load_timings_from_dt(struct tegra_clk_emc *tegra,
 
 	size = (tegra->num_timings + child_count) * sizeof(struct emc_timing);
 
-	tegra->timings = krealloc(tegra->timings, size, GFP_KERNEL);
-	if (!tegra->timings)
+	timings_ptr = krealloc(tegra->timings, size, GFP_KERNEL);
+	if (!timings_ptr)
 		return -ENOMEM;
 
-	timings_ptr = tegra->timings + tegra->num_timings;
+	tegra->timings = timings_ptr;
+	timings_ptr += tegra->num_timings;
 	tegra->num_timings += child_count;
 
 	for_each_child_of_node(node, child) {