linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: manafm@codeaurora.org
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Zhang Rui <rui.zhang@intel.com>,
	Eduardo Valentin <edubezval@gmail.com>,
	Amit Kucheria <amit.kucheria@verdurent.com>,
	Ram Chandrasekar <rkumbako@codeaurora.org>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers: thermal: Add NULL pointer check before using cooling device stats
Date: Tue, 08 Dec 2020 00:22:12 +0530	[thread overview]
Message-ID: <6cbc296beb4dd28214ab009cfc10e7f9@codeaurora.org> (raw)
In-Reply-To: <55cd5e44-f34e-26a3-fad7-20100d429d1d@linaro.org>

On 2020-12-05 16:42, Daniel Lezcano wrote:
> On 04/12/2020 20:09, Manaf Meethalavalappu Pallikunhi wrote:
>> There is a possible chance that some cooling device stats buffer
>> allocation fails due to very high cooling device max state value.
>> Later cooling device update or cooling stats sysfs will try to
>> access stats data for the same cooling device. It will lead to
>> NULL pointer dereference issue.
>> 
>> Add a NULL pointer check before accessing thermal cooling device
>> stats data. It fixes the following bug
>> 
>> [ 26.812833] Unable to handle kernel NULL pointer dereference at 
>> virtual address 0000000000000004
>> [ 27.122960] Call trace:
>> [ 27.122963] do_raw_spin_lock+0x18/0xe8
>> [ 27.122966] _raw_spin_lock+0x24/0x30
>> [ 27.128157] thermal_cooling_device_stats_update+0x24/0x98
>> [ 27.128162] cur_state_store+0x88/0xb8
>> [ 27.128166] dev_attr_store+0x40/0x58
>> [ 27.128169] sysfs_kf_write+0x50/0x68
>> [ 27.133358] kernfs_fop_write+0x12c/0x1c8
>> [ 27.133362] __vfs_write+0x54/0x160
>> [ 27.152297] vfs_write+0xcc/0x188
>> [ 27.157132] ksys_write+0x78/0x108
>> [ 27.162050] ksys_write+0xf8/0x108
>> [ 27.166968] __arm_smccc_hvc+0x158/0x4b0
>> [ 27.166973] __arm_smccc_hvc+0x9c/0x4b0
>> [ 27.186005] el0_svc+0x8/0xc
>> 
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi 
>> <manafm@codeaurora.org>
> 
> The only place where it can crash is when the
> thermal_cooling_device_stats_update() function is called.
> 
> The other places in show*/store* in the stats directory are 
> inaccessible
> as the sysfs entry is not showed up due to the
> thermal_cooling_device_stats_setup() failing.
> 
> It would have been nice if the thermal_cooling_device_stats_update() 
> was
> not called at all but I don't see how we can do that without static 
> keys
> which is overkill for a degraded mode.
> 
> I guess having the kzallocation warning in the console output is enough
> to warn the user the system is working without the stats for the 
> cooling
> device. That should not prevent the system functioning.
> 
> Can you resend with the check in thermal_cooling_device_stats_update() 
> only?
> 
> Thanks
> 
>   -- Daniel
Thanks for your review, I double checked it, yes, no need to add NULL 
check in stats sysfs show*/store* functions.
I will update this in V2

Thanks
Manaf
> 
> 
>> ---
>>  drivers/thermal/thermal_sysfs.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/thermal/thermal_sysfs.c 
>> b/drivers/thermal/thermal_sysfs.c
>> index 473449b..a5e4855 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -827,6 +827,9 @@ void thermal_cooling_device_stats_update(struct 
>> thermal_cooling_device *cdev,
>>  {
>>  	struct cooling_dev_stats *stats = cdev->stats;
>> 
>> +	if (!stats)
>> +		return;
>> +
>>  	spin_lock(&stats->lock);
>> 
>>  	if (stats->state == new_state)
>> @@ -848,6 +851,9 @@ static ssize_t total_trans_show(struct device 
>> *dev,
>>  	struct cooling_dev_stats *stats = cdev->stats;
>>  	int ret;
>> 
>> +	if (!stats)
>> +		return -ENODEV;
>> +
>>  	spin_lock(&stats->lock);
>>  	ret = sprintf(buf, "%u\n", stats->total_trans);
>>  	spin_unlock(&stats->lock);
>> @@ -864,6 +870,9 @@ time_in_state_ms_show(struct device *dev, struct 
>> device_attribute *attr,
>>  	ssize_t len = 0;
>>  	int i;
>> 
>> +	if (!stats)
>> +		return -ENODEV;
>> +
>>  	spin_lock(&stats->lock);
>>  	update_time_in_state(stats);
>> 
>> @@ -882,8 +891,12 @@ reset_store(struct device *dev, struct 
>> device_attribute *attr, const char *buf,
>>  {
>>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
>>  	struct cooling_dev_stats *stats = cdev->stats;
>> -	int i, states = stats->max_states;
>> +	int i, states;
>> +
>> +	if (!stats)
>> +		return -ENODEV;
>> 
>> +	states = stats->max_states;
>>  	spin_lock(&stats->lock);
>> 
>>  	stats->total_trans = 0;
>> @@ -907,6 +920,9 @@ static ssize_t trans_table_show(struct device 
>> *dev,
>>  	ssize_t len = 0;
>>  	int i, j;
>> 
>> +	if (!stats)
>> +		return -ENODEV;
>> +
>>  	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
>>  	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
>>  	for (i = 0; i < stats->max_states; i++) {
>> 

      reply	other threads:[~2020-12-07 18:53 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-04 19:09 Manaf Meethalavalappu Pallikunhi
2020-12-04 19:21 ` Daniel Lezcano
2020-12-05 11:12 ` Daniel Lezcano
2020-12-07 18:52   ` manafm [this message]

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=6cbc296beb4dd28214ab009cfc10e7f9@codeaurora.org \
    --to=manafm@codeaurora.org \
    --cc=amit.kucheria@verdurent.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=edubezval@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rkumbako@codeaurora.org \
    --cc=rui.zhang@intel.com \
    --subject='Re: [PATCH] drivers: thermal: Add NULL pointer check before using cooling device stats' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).