linux-kernel - Re: [PATCH v4 2/4] PM / devfreq: Fix kernel warning with cpufreq passive register fail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Jun 2022 11:20:09 +0200
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Chanwoo Choi <cwchoi00@...il.com>
Cc:     MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Sibi Sankar <sibis@...eaurora.org>,
        Saravana Kannan <skannan@...eaurora.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/4] PM / devfreq: Fix kernel warning with cpufreq
 passive register fail

On Wed, Jun 15, 2022 at 04:11:13PM +0900, Chanwoo Choi wrote:
> On 22. 6. 15. 08:09, Christian 'Ansuel' Marangi wrote:
> > When the cpufreq passive register path from the passive governor fails,
> > the cpufreq_passive_unregister is called and a kernel WARNING is always
> > reported.
> > This is caused by the fact that the devfreq driver already call the
> > governor unregister with the GOV_STOP, for this reason the second
> > cpufreq_passive_unregister always return error and a WARN is printed
> > from the WARN_ON function.
> > Remove the unregister call from the error handling of the cpufreq register
> > notifier as it's fundamentally wrong and already handled by the devfreq
> > core code.
> > 
> > Fixes: a03dacb0316f ("PM / devfreq: Add cpu based scaling support to passive governor")
> > Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@...il.com>
> > ---
> >  drivers/devfreq/governor_passive.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> > index 95de336f20d5..dcc9dd518197 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -331,7 +331,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
> >  err_put_policy:
> >  	cpufreq_cpu_put(policy);
> >  err:
> > -	WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
> >  
> >  	return ret;
> >  }
> 
> I think that it is necessary to free the resource when error happen.

Thing is that it should not be done in the register. Following the flow
of the devfreq core code, if a gov fails to START, the gov STOP is
called and we correctly free our resources. In the current
implementation we call the free 2 times and the second time will always
print error as the notifier is already unregistered.

> Also, after merging the your patch1, I think that cpufreq_passive_unregister_notifier(devfreq)
> will not return error. Instead, just 0 for success.

With path1 we removed the error with the parent_cpu_data deletion but
the unregister error is still there.

> 
> Instead, 'err_free_cpu_data' and 'err_put_policy' goto statement are wrong exception
> handling. If fix the exception handling code in cpufreq_passive_register_notifier
> as following and with your patch1, I'll handle the resource for free/un-registration
> when error happen during cpufreq_passive_register_notifier.
> 

Don't know the main problem here is calling unregister 2 times.

> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index a35b39ac656c..0246e0731fc0 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -289,22 +289,23 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>                 parent_cpu_data = kzalloc(sizeof(*parent_cpu_data),
>                                                 GFP_KERNEL);
>                 if (!parent_cpu_data) {
> +                       cpufreq_cpu_put(policy);
>                         ret = -ENOMEM;
> -                       goto err_put_policy;
> +                       goto err;
>                 }
>  
>                 cpu_dev = get_cpu_device(cpu);
>                 if (!cpu_dev) {
>                         dev_err(dev, "failed to get cpu device\n");
>                         ret = -ENODEV;
> -                       goto err_free_cpu_data;
> +                       goto err;
>                 }
>  
>                 opp_table = dev_pm_opp_get_opp_table(cpu_dev);
>                 if (IS_ERR(opp_table)) {
>                         dev_err(dev, "failed to get opp_table of cpu%d\n", cpu);
>                         ret = PTR_ERR(opp_table);
> -                       goto err_free_cpu_data;
> +                       goto err;
>                 }
>  
>                 parent_cpu_data->dev = cpu_dev;
> @@ -326,10 +327,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  
>         return ret;
>  
> -err_free_cpu_data:
> -       kfree(parent_cpu_data);
> -err_put_policy:
> -       cpufreq_cpu_put(policy);
>  err:
>         WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
>  
> 
> 
> 
> -- 
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

-- 
	Ansuel