Re: [PATCH v8 2/2] cpufreq: Add boost_freq_req QoS request

[Zhongqiu Han]

On 3/30/2026 3:16 PM, zhenglifeng (A) wrote:
> On 3/30/2026 12:00 PM, Zhongqiu Han wrote:
> > On 3/30/2026 10:10 AM, zhenglifeng (A) wrote:
> > > On 3/29/2026 5:00 PM, Zhongqiu Han wrote:
> > > > > @@ -1377,6 +1386,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > > > >         }
> > > > >           freq_qos_remove_request(policy->min_freq_req);
> > > > > +    freq_qos_remove_request(policy->boost_freq_req);
> > > > >         kfree(policy->min_freq_req);
> > > > >           cpufreq_policy_put_kobj(policy);
> > > > > @@ -1445,26 +1455,38 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
> > > > >         cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
> > > > >           if (new_policy) {
> > > > > +        unsigned int count;
> > > > > +
> > > > >             for_each_cpu(j, policy->related_cpus) {
> > > > >                 per_cpu(cpufreq_cpu_data, j) = policy;
> > > > >                 add_cpu_dev_symlink(policy, j, get_cpu_device(j));
> > > > >             }
> > > > >     -        policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req),
> > > > > +        count = policy->boost_supported ? 3 : 2;
> > > > > +        policy->min_freq_req = kzalloc(count * sizeof(*policy->min_freq_req),
> > > > >                                GFP_KERNEL);
> > > > >             if (!policy->min_freq_req) {
> > > > >                 ret = -ENOMEM;
> > > > >                 goto out_destroy_policy;
> > > > >             }
> > > > >     +        if (policy->boost_supported) {
> > > > > +            policy->boost_freq_req = policy->min_freq_req + 2;
> > > > > +
> > > > > +            ret = freq_qos_add_request(&policy->constraints,
> > > > > +                           policy->boost_freq_req,
> > > > > +                           FREQ_QOS_MAX,
> > > > > +                           policy->cpuinfo.max_freq);
> > > > > +            if (ret < 0) {
> > > > > +                policy->boost_freq_req = NULL;
> > > > > +                goto out_destroy_policy;
> > > > > +            }
> > > > > +        }
> > > > > +
> > > > >             ret = freq_qos_add_request(&policy->constraints,
> > > > >                            policy->min_freq_req, FREQ_QOS_MIN,
> > > > >                            FREQ_QOS_MIN_DEFAULT_VALUE);
> > > > >             if (ret < 0) {
> > > > > -            /*
> > > > > -             * So we don't call freq_qos_remove_request() for an
> > > > > -             * uninitialized request.
> > > > > -             */
> > > > >                 kfree(policy->min_freq_req);
> > > > >                 policy->min_freq_req = NULL;
> > > > >                 goto out_destroy_policy;
> > > >
> > > > Hi Pierre, Viresh,
> > > >
> > > > Sorry for the late follow-up on v8. While re-reading the patch, I
> > > > noticed a potential UAF issue on an error path — I might be missing
> > > > something, so I'd appreciate a double-check.
> > > >
> > > > min_freq_req, max_freq_req and boost_freq_req all point into the same
> > > > contiguous kzalloc'd block:
> > > >
> > > > slot0 (min_freq_req + 0) -> min_freq_req
> > > > slot1 (min_freq_req + 1) -> max_freq_req
> > > > slot2 (min_freq_req + 2) -> boost_freq_req
> > > >
> > > > If boost_freq_req is successfully added to the QoS constraints list, but
> > > > the subsequent freq_qos_add_request() for min_freq_req fails, the error
> > > > path does:
> > > >
> > > > kfree(policy->min_freq_req); /* frees the entire block, including slot2
> > > > */
> > > > policy->min_freq_req = NULL;
> > > > goto out_destroy_policy;
> > > >
> > > > policy->boost_freq_req is not set to NULL here, so it becomes a dangling
> > > > pointer into freed memory.
> > > > cpufreq_policy_free() is then called from cpufreq_online() and does:
> > > >
> > > > freq_qos_remove_request(policy->boost_freq_req); /* UAF */
> > > > or this boost qos req will leak.
> > >
> > > Good catch!
> > >
> > > How about remove the kfree() here and just leave it to
> > > cpufreq_policy_free()?
> >
> > Thanks for the suggestion — this is another fix approach we can
> > explore, but there seems to be a small caveat.
> >
> > Some additional changes would still be needed; otherwise, removing the
> > kfree() here and deferring it to cpufreq_policy_free() can lead to a
> > warning.
> >
> > The reason is that we neither free policy->min_freq_req nor set policy
> > ->min_freq_req = NULL. As a result, when cpufreq_policy_free() later
> > calls freq_qos_remove_request(policy->min_freq_req), it hits the
> > following warning:
> >
> > if (WARN(!freq_qos_request_active(req),
> >      "%s() called for unknown object\n", __func__))
> >      return -EINVAL;
>
> Therefore, it seems the only option is to allocate memory separately for
> boost_freq_req.

Thanks Lifeng. Allocating memory separately could also be a direction we
can explore. I also sketched another small example in a separate mail
thread for discussion.


--
Thx and BRs,
Zhongqiu Han