public inbox for linux-pm@vger.kernel.org
 help / color / mirror / Atom feed
From: Chris Hyser <chris.hyser@oracle.com>
To: chris.hyser@oracle.com, linux-pm@vger.kernel.org,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Viresh Kumar <viresh.kumar@linaro.org>
Subject: cpufreq: Scaling driver sysfs attribute issues.
Date: Fri, 19 Aug 2022 15:03:56 -0400	[thread overview]
Message-ID: <1660935837-7481-1-git-send-email-chris.hyser@oracle.com> (raw)

We found a NULL pointer dereference in cppc_cpufreq scaling driver, traced it
to root cause, and looked at the other scaling drivers to see what else may be
affected.

The simplest fix for cppc_cpufreq is basically identical to commit e25303676e18
("cpufreq: acpi_cpufreq: prevent crash on reading freqdomain_cpus") and is
included.

The problem is a scaling driver which provides a struct freq_attr sysfs
attributes array passed via struct cpufreq_driver during driver registration,
has .init() and .exit() functions and does _not_ provide .online()/.offline()
routines.  The freq_attr attributes are often backed by allocated memory from
.init() and freed on .exit(). The issue is that the cpufreq core does not remove
those driver freq_attr sysfs files in this scenario though the backing memory
has been freed and while some instances just show garbage values, others contain
pointers. Note: registering empty .online()/.offline() driver functions can also
solve this problem as .exit() is not called and the memory has not been freed.

Looking through the other scaling drivers, most that provide a .attr use
'cpufreq_generic_attr' for which the 'show' functions do check for a NULL data
pointer. Two others are worth mentioning: brcm_avs_cpufreq driver has it's own
attributes backed by it's own functions. These do not check to see if the data
pointer is NULL in the show() functions, however as it uses devm_kzalloc(), it
has no .exit() and the sysfs files remain backed by memory until driver unload.

The other is the 'amd_pstate' driver. I don't have a means of testing, but I
suspect that this driver does have a similar problem, ie if a cpu is removed
and the various driver sysfs attrs are accessed, it will crash.

Additionally, I spent some time looking at an alternative that removed the
driver .attr set of sysfs attributes prior to calling .exit() in
__cpufreq_offline() and puts them back following an .init() in cpufreq_online().
On the AMD and ARM systems I tested with, no policies are shared and thus
.init()/.exit() are called on every cpu online/offline operation.  A simple test
consisting of two tight loops one onlining/offlining a few cpus and the other
reading the sysfs cpumask file, ultimately results in deadlock between sysfs and
the cpu hotplug machinery.  This seems consistent with various commit comments
about the locking here and a good time to send an email.

-chrish


             reply	other threads:[~2022-08-19 19:04 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-19 19:03 Chris Hyser [this message]
2022-08-19 19:03 ` [PATCH] cpufreq: cppc_cpufreq: prevent crash on reading freqdomain_cpus Chris Hyser
2022-08-22  5:39   ` Viresh Kumar
2022-08-22 13:19     ` Chris Hyser
2022-08-22 15:54       ` Chris Hyser
2022-08-22 20:14         ` Chris Hyser
2022-08-23  4:13           ` Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1660935837-7481-1-git-send-email-chris.hyser@oracle.com \
    --to=chris.hyser@oracle.com \
    --cc=linux-pm@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox