On Wed, Apr 12, 2023 at 8:39 AM Nikita Zhandarovich <n.zhandarov...@fintech.ru> wrote: > > > > On 4/11/23 14:11, Deucher, Alexander wrote: > > [Public] > > > >> -----Original Message----- > >> From: Nikita Zhandarovich <n.zhandarov...@fintech.ru> > >> Sent: Monday, April 3, 2023 2:28 PM > >> To: Deucher, Alexander <alexander.deuc...@amd.com> > >> Cc: Nikita Zhandarovich <n.zhandarov...@fintech.ru>; Koenig, Christian > >> <christian.koe...@amd.com>; Pan, Xinhui <xinhui....@amd.com>; David > >> Airlie <airl...@gmail.com>; Daniel Vetter <dan...@ffwll.ch>; amd- > >> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > >> ker...@vger.kernel.org; lvc-proj...@linuxtesting.org > >> Subject: [PATCH] radeon: avoid double free in ci_dpm_init() > >> > >> There are several calls to ci_dpm_fini() in ci_dpm_init() when there occur > >> errors in functions like r600_parse_extended_power_table(). > >> This is harmful as it can lead to double free situations: for instance, > >> r600_parse_extended_power_table() will call for > >> r600_free_extended_power_table() as will ci_dpm_fini(), both of which will > >> try to free resources. > >> Other drivers do not call *_dpm_fini functions from their respective > >> *_dpm_init calls - neither should cpm_dpm_init(). > >> > >> Fix this by removing extra calls to ci_dpm_fini(). > > > > You can't just drop the calls to fini(). You'll need to properly unwind to > > avoid leaking memory. > > > > Alex > >>> > >> Found by Linux Verification Center (linuxtesting.org) with static analysis > >> tool > >> SVACE. > >> > >> Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)") > >> Cc: sta...@vger.kernel.org > >> Co-developed-by: Natalia Petrova <n.petr...@fintech.ru> > >> Signed-off-by: Nikita Zhandarovich <n.zhandarov...@fintech.ru> > >> > >> --- > >> drivers/gpu/drm/radeon/ci_dpm.c | 20 +++++--------------- > >> 1 file changed, 5 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/radeon/ci_dpm.c > >> b/drivers/gpu/drm/radeon/ci_dpm.c index 8ef25ab305ae..7b77d4c93f1d > >> 100644 > >> --- a/drivers/gpu/drm/radeon/ci_dpm.c > >> +++ b/drivers/gpu/drm/radeon/ci_dpm.c > >> @@ -5677,28 +5677,20 @@ int ci_dpm_init(struct radeon_device *rdev) > >> pi->pcie_lane_powersaving.min = 16; > >> > >> ret = ci_get_vbios_boot_values(rdev, &pi->vbios_boot_state); > >> - if (ret) { > >> - ci_dpm_fini(rdev); > >> + if (ret) > >> return ret; > >> - } > >> > >> ret = r600_get_platform_caps(rdev); > >> - if (ret) { > >> - ci_dpm_fini(rdev); > >> + if (ret) > >> return ret; > >> - } > >> > >> ret = r600_parse_extended_power_table(rdev); > >> - if (ret) { > >> - ci_dpm_fini(rdev); > >> + if (ret) > >> return ret; > >> - } > >> > >> ret = ci_parse_power_table(rdev); > >> - if (ret) { > >> - ci_dpm_fini(rdev); > >> + if (ret) > >> return ret; > >> - } > >> > >> pi->dll_default_on = false; > >> pi->sram_end = SMC_RAM_END; > >> @@ -5749,10 +5741,8 @@ int ci_dpm_init(struct radeon_device *rdev) > >> kcalloc(4, > >> sizeof(struct > >> radeon_clock_voltage_dependency_entry), > >> GFP_KERNEL); > >> - if (!rdev- > >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) { > >> - ci_dpm_fini(rdev); > >> + if (!rdev- > >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) > >> return -ENOMEM; > >> - } > >> rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.count = 4; > >> rdev- > >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].clk = 0; > >> rdev- > >>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].v = 0; > > > I think you are correct when it comes to ensuring we deal with memory > issues in ci_dpm_init(). > > However, I could use some direction on how to deal with the problem of > freeing only previously allocated resources. For instance, once > ci_parse_power_table() fails, it is not clear what we should and should > not free.
You'll want to free any memory allocated in ci_dpm_init(). Any of the functions called from that function should clean themselves up if they allocate any memory, but if not, they should be fixed. Alex > > I wanna point out that in this case I would like to fix both double and > uninitialized free issues as it can also lead to undefined behavior. > > Thanks for your patience, > Nikita