| Message ID | CAPgLHd_d5DHn==OFdBsLi_Kkekd5h44Cx6m9Hj5EM_yOGJXKPQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
On Tue, 2013-06-18 at 21:09 +0800, Wei Yongjun wrote: > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > If krealloc() returns NULL, it doesn't free the original. So any code > of the form 'foo = krealloc(foo, ...);' is almost certainly a bug. > you mean this would probably cause a memory leak, right? > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > --- > drivers/thermal/x86_pkg_temp_thermal.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c > index 5de56f6..b90e84b 100644 > --- a/drivers/thermal/x86_pkg_temp_thermal.c > +++ b/drivers/thermal/x86_pkg_temp_thermal.c > @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) > char buffer[30]; > int thres_count; > u32 eax, ebx, ecx, edx; > + u8 *temp; > > cpuid(6, &eax, &ebx, &ecx, &edx); > thres_count = ebx & 0x07; > @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) > spin_lock(&pkg_work_lock); > if (topology_physical_package_id(cpu) > max_phy_id) > max_phy_id = topology_physical_package_id(cpu); > - pkg_work_scheduled = krealloc(pkg_work_scheduled, > - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); > - if (!pkg_work_scheduled) { > + temp = krealloc(pkg_work_scheduled, > + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); > + if (!temp) { without this patch, this function will quite if krealloc returns NULL, but with the previous pkg_work_scheduled unfreed, right? thanks, rui > spin_unlock(&pkg_work_lock); > err = -ENOMEM; > goto err_ret_free; > } > + pkg_work_scheduled = temp; > pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; > spin_unlock(&pkg_work_lock); > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2013 04:07 PM, Zhang Rui wrote: > On Tue, 2013-06-18 at 21:09 +0800, Wei Yongjun wrote: >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> If krealloc() returns NULL, it doesn't free the original. So any code >> of the form 'foo = krealloc(foo, ...);' is almost certainly a bug. >> > you mean this would probably cause a memory leak, right? Yes. > >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> --- >> drivers/thermal/x86_pkg_temp_thermal.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c >> index 5de56f6..b90e84b 100644 >> --- a/drivers/thermal/x86_pkg_temp_thermal.c >> +++ b/drivers/thermal/x86_pkg_temp_thermal.c >> @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >> char buffer[30]; >> int thres_count; >> u32 eax, ebx, ecx, edx; >> + u8 *temp; >> >> cpuid(6, &eax, &ebx, &ecx, &edx); >> thres_count = ebx & 0x07; >> @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >> spin_lock(&pkg_work_lock); >> if (topology_physical_package_id(cpu) > max_phy_id) >> max_phy_id = topology_physical_package_id(cpu); >> - pkg_work_scheduled = krealloc(pkg_work_scheduled, >> - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >> - if (!pkg_work_scheduled) { >> + temp = krealloc(pkg_work_scheduled, >> + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >> + if (!temp) { > without this patch, this function will quite if krealloc returns NULL, > but with the previous pkg_work_scheduled unfreed, right? Yes, without patch, previous pkg_work_scheduled will unfreed if krealloc return NULL. > > thanks, > rui >> spin_unlock(&pkg_work_lock); >> err = -ENOMEM; >> goto err_ret_free; >> } >> + pkg_work_scheduled = temp; >> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; >> spin_unlock(&pkg_work_lock); >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2013 01:49 AM, Wei Yongjun wrote: > On 06/21/2013 04:07 PM, Zhang Rui wrote: >> On Tue, 2013-06-18 at 21:09 +0800, Wei Yongjun wrote: >>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>> >>> If krealloc() returns NULL, it doesn't free the original. So any code >>> of the form 'foo = krealloc(foo, ...);' is almost certainly a bug. >>> >> you mean this would probably cause a memory leak, right? Agree. But whether system will really function it can't allocate few bytes (=number CPU packages). It will crash somewhere anyway. > Yes. > >>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>> --- >>> drivers/thermal/x86_pkg_temp_thermal.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c >>> index 5de56f6..b90e84b 100644 >>> --- a/drivers/thermal/x86_pkg_temp_thermal.c >>> +++ b/drivers/thermal/x86_pkg_temp_thermal.c >>> @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >>> char buffer[30]; >>> int thres_count; >>> u32 eax, ebx, ecx, edx; >>> + u8 *temp; >>> >>> cpuid(6, &eax, &ebx, &ecx, &edx); >>> thres_count = ebx & 0x07; >>> @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >>> spin_lock(&pkg_work_lock); >>> if (topology_physical_package_id(cpu) > max_phy_id) >>> max_phy_id = topology_physical_package_id(cpu); >>> - pkg_work_scheduled = krealloc(pkg_work_scheduled, >>> - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >>> - if (!pkg_work_scheduled) { >>> + temp = krealloc(pkg_work_scheduled, >>> + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >>> + if (!temp) { >> without this patch, this function will quite if krealloc returns NULL, >> but with the previous pkg_work_scheduled unfreed, right? > Yes, without patch, previous pkg_work_scheduled will unfreed if krealloc > return NULL. > >> thanks, >> rui >>> spin_unlock(&pkg_work_lock); >>> err = -ENOMEM; >>> goto err_ret_free; >>> } >>> + pkg_work_scheduled = temp; >>> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; >>> spin_unlock(&pkg_work_lock); >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tested this patch and it works fine. Thanks, Srinivas On 06/21/2013 01:49 AM, Wei Yongjun wrote: > On 06/21/2013 04:07 PM, Zhang Rui wrote: >> On Tue, 2013-06-18 at 21:09 +0800, Wei Yongjun wrote: >>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>> >>> If krealloc() returns NULL, it doesn't free the original. So any code >>> of the form 'foo = krealloc(foo, ...);' is almost certainly a bug. >>> >> you mean this would probably cause a memory leak, right? > Yes. > >>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>> --- >>> drivers/thermal/x86_pkg_temp_thermal.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c >>> index 5de56f6..b90e84b 100644 >>> --- a/drivers/thermal/x86_pkg_temp_thermal.c >>> +++ b/drivers/thermal/x86_pkg_temp_thermal.c >>> @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >>> char buffer[30]; >>> int thres_count; >>> u32 eax, ebx, ecx, edx; >>> + u8 *temp; >>> >>> cpuid(6, &eax, &ebx, &ecx, &edx); >>> thres_count = ebx & 0x07; >>> @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) >>> spin_lock(&pkg_work_lock); >>> if (topology_physical_package_id(cpu) > max_phy_id) >>> max_phy_id = topology_physical_package_id(cpu); >>> - pkg_work_scheduled = krealloc(pkg_work_scheduled, >>> - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >>> - if (!pkg_work_scheduled) { >>> + temp = krealloc(pkg_work_scheduled, >>> + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); >>> + if (!temp) { >> without this patch, this function will quite if krealloc returns NULL, >> but with the previous pkg_work_scheduled unfreed, right? > Yes, without patch, previous pkg_work_scheduled will unfreed if krealloc > return NULL. > >> thanks, >> rui >>> spin_unlock(&pkg_work_lock); >>> err = -ENOMEM; >>> goto err_ret_free; >>> } >>> + pkg_work_scheduled = temp; >>> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; >>> spin_unlock(&pkg_work_lock); >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-07-11 at 17:36 -0700, Srinivas Pandruvada wrote: > Tested this patch and it works fine. > > Thanks, > Srinivas > > On 06/21/2013 01:49 AM, Wei Yongjun wrote: > > On 06/21/2013 04:07 PM, Zhang Rui wrote: > >> On Tue, 2013-06-18 at 21:09 +0800, Wei Yongjun wrote: > >>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >>> > >>> If krealloc() returns NULL, it doesn't free the original. So any code > >>> of the form 'foo = krealloc(foo, ...);' is almost certainly a bug. > >>> > >> you mean this would probably cause a memory leak, right? > > Yes. > > > >>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> applied to thermal-next. thanks, rui > >>> --- > >>> drivers/thermal/x86_pkg_temp_thermal.c | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c > >>> index 5de56f6..b90e84b 100644 > >>> --- a/drivers/thermal/x86_pkg_temp_thermal.c > >>> +++ b/drivers/thermal/x86_pkg_temp_thermal.c > >>> @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) > >>> char buffer[30]; > >>> int thres_count; > >>> u32 eax, ebx, ecx, edx; > >>> + u8 *temp; > >>> > >>> cpuid(6, &eax, &ebx, &ecx, &edx); > >>> thres_count = ebx & 0x07; > >>> @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) > >>> spin_lock(&pkg_work_lock); > >>> if (topology_physical_package_id(cpu) > max_phy_id) > >>> max_phy_id = topology_physical_package_id(cpu); > >>> - pkg_work_scheduled = krealloc(pkg_work_scheduled, > >>> - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); > >>> - if (!pkg_work_scheduled) { > >>> + temp = krealloc(pkg_work_scheduled, > >>> + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); > >>> + if (!temp) { > >> without this patch, this function will quite if krealloc returns NULL, > >> but with the previous pkg_work_scheduled unfreed, right? > > Yes, without patch, previous pkg_work_scheduled will unfreed if krealloc > > return NULL. > > > >> thanks, > >> rui > >>> spin_unlock(&pkg_work_lock); > >>> err = -ENOMEM; > >>> goto err_ret_free; > >>> } > >>> + pkg_work_scheduled = temp; > >>> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; > >>> spin_unlock(&pkg_work_lock); > >>> > >>> > >> > >> > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c index 5de56f6..b90e84b 100644 --- a/drivers/thermal/x86_pkg_temp_thermal.c +++ b/drivers/thermal/x86_pkg_temp_thermal.c @@ -394,6 +394,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) char buffer[30]; int thres_count; u32 eax, ebx, ecx, edx; + u8 *temp; cpuid(6, &eax, &ebx, &ecx, &edx); thres_count = ebx & 0x07; @@ -417,13 +418,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu) spin_lock(&pkg_work_lock); if (topology_physical_package_id(cpu) > max_phy_id) max_phy_id = topology_physical_package_id(cpu); - pkg_work_scheduled = krealloc(pkg_work_scheduled, - (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); - if (!pkg_work_scheduled) { + temp = krealloc(pkg_work_scheduled, + (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); + if (!temp) { spin_unlock(&pkg_work_lock); err = -ENOMEM; goto err_ret_free; } + pkg_work_scheduled = temp; pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; spin_unlock(&pkg_work_lock);