| Message ID | 1521899375-24335-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 24-03-18, 10:49, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > devm_kzalloc() may fail, so we should better check for error and > propagate the error in the case of allocation failure. > > This avoids a potential NULL pointer dereference later on. > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > drivers/cpufreq/powernv-cpufreq.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c > index 0591874..4ddfec9 100644 > --- a/drivers/cpufreq/powernv-cpufreq.c > +++ b/drivers/cpufreq/powernv-cpufreq.c > @@ -329,6 +329,8 @@ static int init_powernv_pstates(void) > > revmap_data = (struct pstate_idx_revmap_data *) > kmalloc(sizeof(*revmap_data), GFP_KERNEL); > + if (!revmap_data) > + return -ENOMEM; What about freeing resources on such failures ?
On Saturday, March 24, 2018 2:49:34 PM CEST Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > devm_kzalloc() may fail, so we should better check for error and > propagate the error in the case of allocation failure. > > This avoids a potential NULL pointer dereference later on. > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > drivers/cpufreq/powernv-cpufreq.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c > index 0591874..4ddfec9 100644 > --- a/drivers/cpufreq/powernv-cpufreq.c > +++ b/drivers/cpufreq/powernv-cpufreq.c > @@ -329,6 +329,8 @@ static int init_powernv_pstates(void) > > revmap_data = (struct pstate_idx_revmap_data *) > kmalloc(sizeof(*revmap_data), GFP_KERNEL); > + if (!revmap_data) > + return -ENOMEM; As per the response from Viresh, are there any resources to free in this case? > > revmap_data->pstate_id = id & 0xFF; > revmap_data->cpufreq_table_idx = i; > Overall, if you make powernv changes, please CC the PowerPC maintainers as per the LINUX FOR POWERPC entry in MAINTAINERS. Thanks, Rafel
On Mon, Apr 2, 2018 at 2:03 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> --- a/drivers/cpufreq/powernv-cpufreq.c >> +++ b/drivers/cpufreq/powernv-cpufreq.c >> @@ -329,6 +329,8 @@ static int init_powernv_pstates(void) >> >> revmap_data = (struct pstate_idx_revmap_data *) >> kmalloc(sizeof(*revmap_data), GFP_KERNEL); >> + if (!revmap_data) >> + return -ENOMEM; > > What about freeing resources on such failures ? What are the resources that you are talking about?
On 23-04-18, 15:50, Fabio Estevam wrote: > On Mon, Apr 2, 2018 at 2:03 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > >> --- a/drivers/cpufreq/powernv-cpufreq.c > >> +++ b/drivers/cpufreq/powernv-cpufreq.c > >> @@ -329,6 +329,8 @@ static int init_powernv_pstates(void) > >> > >> revmap_data = (struct pstate_idx_revmap_data *) > >> kmalloc(sizeof(*revmap_data), GFP_KERNEL); > >> + if (!revmap_data) > >> + return -ENOMEM; > > > > What about freeing resources on such failures ? > > What are the resources that you are talking about? You are allocating revmap_data for all states one by one. You need to undo that for all previous states once this loop fails for any one of them. Isn't that right ?
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c index 0591874..4ddfec9 100644 --- a/drivers/cpufreq/powernv-cpufreq.c +++ b/drivers/cpufreq/powernv-cpufreq.c @@ -329,6 +329,8 @@ static int init_powernv_pstates(void) revmap_data = (struct pstate_idx_revmap_data *) kmalloc(sizeof(*revmap_data), GFP_KERNEL); + if (!revmap_data) + return -ENOMEM; revmap_data->pstate_id = id & 0xFF; revmap_data->cpufreq_table_idx = i;