Re: [PATCH] powercap: intel_rapl: Fix rapl_packages list corruption

On Fri, 2026-09-04 at 15:42 +0200, Rafael J. Wysocki (Intel) wrote:
> Hi Srinivas,
> 
> Can you please have a look at the patch below and let me know what
> you think?
> 
> On Sat, Aug 29, 2026 at 6:43 PM Sumeet Pawnikar
> <[email protected]> wrote:
> > 
> > The rapl_packages list was previously documented as being guarded
> > by the
> > CPU hotplug lock (cpus_read_lock()). However, cpus_read_lock() is a
> > reader-writer semaphore taken for shared read access. This provides
> > zero
> > mutual exclusion between multiple concurrent callers.
> > 

Using these locks together need to be careful as it will be lock
inversion risk.

> > Because auxiliary device probes (such as intel_rapl_tpmi_probe) can
> > execute concurrently on different CPUs, multiple packages can race
> > on
> > list operations (list_add and list_del) inside
> > rapl_add_package_cpuslocked() and rapl_remove_package_cpuslocked(),
> > leading to list corruption.
> > 
> > Introduce a dedicated rapl_packages_lock mutex to protect all
> > additions,
> > removals and iterations of the rapl_packages list.
> > 
> > Signed-off-by: Sumeet Pawnikar <[email protected]>
> > ---
> >  drivers/powercap/intel_rapl_common.c | 29
> > ++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 70cab5f08bc6..e544629dbe30 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -175,7 +175,8 @@ static u64 rapl_unit_xlate(struct rapl_domain
> > *rd,
> >                            enum unit_type type, u64 value, int
> > to_raw);
> >  static void package_power_limit_irq_save(struct rapl_package *rp);
> > 
> > -static LIST_HEAD(rapl_packages);       /* guarded by CPU hotplug
> > lock */
> > +static DEFINE_MUTEX(rapl_packages_lock);
> > +static LIST_HEAD(rapl_packages);       /* guarded by
> > rapl_packages_lock */
> > 
> >  static const char *const rapl_domain_names[] = {
> >         "package",
> > @@ -1360,12 +1361,14 @@ static int rapl_pmu_event_init(struct
> > perf_event *event)
> >                 return -EINVAL;
> > 
> >         /* Find out which Package the event belongs to */
> > +       mutex_lock(&rapl_packages_lock);

Have you tried to exercise this path from perf ?

> >         list_for_each_entry(pos, &rapl_packages, plist) {
> >                 if (is_rp_pmu_cpu(pos, event->cpu)) {
> >                         rp = pos;
> >                         break;
> >                 }
> >         }
> > +       mutex_unlock(&rapl_packages_lock);

Mutex is released while event->pmu_private = rp is assigned while
perf_event_open does not hold hotplug or package reference locks.
So rapl_remove_package_cpuslocked() can free this rp, but events->rp
still holds that pointer.

You may need 
	cpus_read_lock()

before
	mutex_lock(&rapl_packages_lock);
above

and
 and cpus_read_unlock() at the end of function

Please run LOCKDEP, perf RAPL and online offline tests before.

Thanks,
Srinivas


> >         if (!rp)
> >                 return -ENODEV;
> > 
> > @@ -1445,9 +1448,11 @@ static ssize_t cpumask_show(struct device
> > *dev,
> >         cpumask_clear(cpu_mask);
> > 
> >         /* Choose a cpu for each RAPL Package */
> > +       mutex_lock(&rapl_packages_lock);
> >         list_for_each_entry(rp, &rapl_packages, plist) {
> >                 set_pmu_cpumask(rp, cpu_mask);
> >         }
> > +       mutex_unlock(&rapl_packages_lock);
> >         cpus_read_unlock();
> > 
> >         ret = sysfs_emit(buf, "%*pbl\n",
> > cpumask_pr_args(cpu_mask));
> > @@ -1655,11 +1660,15 @@ void rapl_package_remove_pmu_locked(struct
> > rapl_package *rp)
> >         if (!rp->has_pmu)
> >                 return;
> > 
> > +       mutex_lock(&rapl_packages_lock);
> >         list_for_each_entry(pos, &rapl_packages, plist) {
> >                 /* PMU is still needed */
> > -               if (pos->has_pmu && pos != rp)
> > +               if (pos->has_pmu && pos != rp) {
> > +                       mutex_unlock(&rapl_packages_lock);
> >                         return;
> > +               }
> >         }
> > +       mutex_unlock(&rapl_packages_lock);
> > 
> >         if (rapl_pmu.registered)
> >                 perf_pmu_unregister(&rapl_pmu.pmu);
> > @@ -1704,7 +1713,9 @@ void rapl_remove_package_cpuslocked(struct
> > rapl_package *rp)
> >         /* do parent zone last */
> >         powercap_unregister_zone(rp->priv->control_type,
> >                                  &rd_package->power_zone);
> > +       mutex_lock(&rapl_packages_lock);
> >         list_del(&rp->plist);
> > +       mutex_unlock(&rapl_packages_lock);
> >         kfree(rp);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(rapl_remove_package_cpuslocked,
> > "INTEL_RAPL");
> > @@ -1749,11 +1760,15 @@ struct rapl_package
> > *rapl_find_package_domain_cpuslocked(int id, struct rapl_if_
> >         else
> >                 uid = id;
> > 
> > +       mutex_lock(&rapl_packages_lock);
> >         list_for_each_entry(rp, &rapl_packages, plist) {
> > -               if (rp->id == uid
> > -                   && rp->priv->control_type == priv-
> > >control_type)
> > +               if (rp->id == uid &&
> > +                   rp->priv->control_type == priv->control_type) {
> > +                       mutex_unlock(&rapl_packages_lock);
> >                         return rp;
> > +               }
> >         }
> > +       mutex_unlock(&rapl_packages_lock);
> > 
> >         return NULL;
> >  }
> > @@ -1810,7 +1825,9 @@ struct rapl_package
> > *rapl_add_package_cpuslocked(int id, struct rapl_if_priv *pr
> >         ret = rapl_package_register_powercap(rp);
> >         if (!ret) {
> >                 INIT_LIST_HEAD(&rp->plist);
> > +               mutex_lock(&rapl_packages_lock);
> >                 list_add(&rp->plist, &rapl_packages);
> > +               mutex_unlock(&rapl_packages_lock);
> >                 return rp;
> >         }
> > 
> > @@ -1835,6 +1852,7 @@ static void power_limit_state_save(void)
> >         int ret, i;
> > 
> >         cpus_read_lock();
> > +       mutex_lock(&rapl_packages_lock);
> >         list_for_each_entry(rp, &rapl_packages, plist) {
> >                 if (!rp->power_zone)
> >                         continue;
> > @@ -1846,6 +1864,7 @@ static void power_limit_state_save(void)
> >                                 rd->rpl[i].last_power_limit = 0;
> >                 }
> >         }
> > +       mutex_unlock(&rapl_packages_lock);
> >         cpus_read_unlock();
> >  }
> > 
> > @@ -1856,6 +1875,7 @@ static void power_limit_state_restore(void)
> >         int i;
> > 
> >         cpus_read_lock();
> > +       mutex_lock(&rapl_packages_lock);
> >         list_for_each_entry(rp, &rapl_packages, plist) {
> >                 if (!rp->power_zone)
> >                         continue;
> > @@ -1865,6 +1885,7 @@ static void power_limit_state_restore(void)
> >                                 rapl_write_pl_data(rd, i, PL_LIMIT,
> >                                                rd-
> > >rpl[i].last_power_limit);
> >         }
> > +       mutex_unlock(&rapl_packages_lock);
> >         cpus_read_unlock();
> >  }
> > 
> > --
> > 2.43.0
> >