[PATCH 1/2] cpufreq: acpi_cpufreq: prevent crash on reading freqdomain_cpus

[PATCH 1/2] cpufreq: acpi_cpufreq: prevent crash on reading freqdomain_cpus

[Srinivas Pandruvada]

When freqdomain_cpus attribute is read from an offlined cpu, it will
cause crash. This change prevents calling cpufreq_show_cpus when
mask is NULL. Alternatively we can add this NULL check in
cpufreq_show_cpus function.

Crash info:

[  170.814949] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
[  170.814990] IP: [<ffffffff813b2490>] _find_next_bit.part.0+0x10/0x70
[  170.815021] PGD 227d30067 PUD 229e56067 PMD 0
[  170.815043] Oops: 0000 [#2] SMP
[  170.816022] CPU: 3 PID: 3121 Comm: cat Tainted: G      D    OE   4.3.0-rc3+ #33
...
...
[  170.816657] Call Trace:
[  170.816672]  [<ffffffff813b2505>] ? find_next_bit+0x15/0x20
[  170.816696]  [<ffffffff8160e47c>] cpufreq_show_cpus+0x5c/0xd0
[  170.816722]  [<ffffffffa031a409>] show_freqdomain_cpus+0x19/0x20 [acpi_cpufreq]
[  170.816749]  [<ffffffff8160e65b>] show+0x3b/0x60
[  170.816769]  [<ffffffff8129b31c>] sysfs_kf_seq_show+0xbc/0x130
[  170.816793]  [<ffffffff81299be3>] kernfs_seq_show+0x23/0x30
[  170.816816]  [<ffffffff81240f2c>] seq_read+0xec/0x390
[  170.816837]  [<ffffffff8129a64a>] kernfs_fop_read+0x10a/0x160
[  170.816861]  [<ffffffff8121d9b7>] __vfs_read+0x37/0x100
[  170.816883]  [<ffffffff813217c0>] ? security_file_permission+0xa0/0xc0
[  170.816909]  [<ffffffff8121e2e3>] vfs_read+0x83/0x130
[  170.816930]  [<ffffffff8121f035>] SyS_read+0x55/0xc0
...
...
[  170.817185] ---[ end trace bc6eadf82b2b965a ]---

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 7982772..cec1ee2 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -149,6 +149,9 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
 {
 	struct acpi_cpufreq_data *data = policy->driver_data;
 
+	if (unlikely(!data))
+		return -ENODEV;
+
 	return cpufreq_show_cpus(data->freqdomain_cpus, buf);
 }
 
-- 
2.4.3

[PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Srinivas Pandruvada]

When scaling_available_frequencies is read on an offlined cpu, then
either lockup or junk values are displayed. This is caused by
freed freq_table, which policy is using.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/cpufreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ef5ed94..25c4c15 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu)
 	 * since this is a core component, and is essential for the
 	 * subsequent light-weight ->init() to succeed.
 	 */
-	if (cpufreq_driver->exit)
+	if (cpufreq_driver->exit) {
 		cpufreq_driver->exit(policy);
+		policy->freq_table = NULL;
+	}
 }
 
 /**
-- 
2.4.3

Re: [PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Viresh Kumar]

On 01-10-15, 15:23, Srinivas Pandruvada wrote:
> When scaling_available_frequencies is read on an offlined cpu, then
> either lockup or junk values are displayed. This is caused by
> freed freq_table, which policy is using.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/cpufreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index ef5ed94..25c4c15 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu)
>  	 * since this is a core component, and is essential for the
>  	 * subsequent light-weight ->init() to succeed.
>  	 */
> -	if (cpufreq_driver->exit)
> +	if (cpufreq_driver->exit) {
>  		cpufreq_driver->exit(policy);
> +		policy->freq_table = NULL;
> +	}
>  }

freq_table was set from the ->init() callbacks and only they should
set it to NULL, isn't it?

-- 
viresh

Re: [PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Srinivas Pandruvada]

On Wed, 2015-10-07 at 18:15 +0530, Viresh Kumar wrote:
> On 01-10-15, 15:23, Srinivas Pandruvada wrote:
> > When scaling_available_frequencies is read on an offlined cpu, then
> > either lockup or junk values are displayed. This is caused by
> > freed freq_table, which policy is using.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index ef5ed94..25c4c15 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu)
> >  	 * since this is a core component, and is essential for the
> >  	 * subsequent light-weight ->init() to succeed.
> >  	 */
> > -	if (cpufreq_driver->exit)
> > +	if (cpufreq_driver->exit) {
> >  		cpufreq_driver->exit(policy);
> > +		policy->freq_table = NULL;
> > +	}
> >  }
> 
> freq_table was set from the ->init() callbacks and only they should
> set it to NULL, isn't it?
freq_table is allocated in init() callbacks and freed on exit()
callback. But I don't see any driver changing policy->freq_table to NULL
or setting value to policy->freq_table to their allocated freq_table .

I see policy->freq_table is only assigned value in function
cpufreq_table_validate_and_show, as part of the core API. So I think we
should set to NULL after clients freed the freq_table as part of core
function.

Otherwise we need to modify every client cpufreq driver and assign
policy->freq_table=NULL on .exit().  Since they don't assign value to
this policy->freq_table in first place, they shouldn't change this.

Also I don't know why we need to keep the sysfs entry cpufreq after
offline. There are other values, which we can read without crash, but
may not be valid. We may delete this sysfs entry. But may be some
cpufreq driver cares about this even after offline. If we can delete
this entry, it can be part of cleanup of cpufreq subsystem.

Thanks,
Srinivas

> 

Re: [PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Viresh Kumar]

On 07-10-15, 09:18, Srinivas Pandruvada wrote:
> freq_table is allocated in init() callbacks and freed on exit()
> callback. But I don't see any driver changing policy->freq_table to NULL
> or setting value to policy->freq_table to their allocated freq_table .
 
That's what I am asking for.

> I see policy->freq_table is only assigned value in function
> cpufreq_table_validate_and_show, as part of the core API.

Its just an helper written to reduce code redundancy..

> So I think we
> should set to NULL after clients freed the freq_table as part of core
> function.

Not really. If we want to do the opposite, then we should initiate
policy->freq_table = NULL, right from exit(), as its done as part of
init() first.

> Otherwise we need to modify every client cpufreq driver and assign
> policy->freq_table=NULL on .exit().  Since they don't assign value to
> this policy->freq_table in first place, they shouldn't change this.

They did set it in the first place, with help of a helper routine
though.

But I do understand the code redundancy we are about to create. So,
maybe we can do this in the core as a special case.

But then you missed another location where exit() was called.

> Also I don't know why we need to keep the sysfs entry cpufreq after
> offline. There are other values, which we can read without crash, but
> may not be valid. We may delete this sysfs entry. But may be some
> cpufreq driver cares about this even after offline. If we can delete
> this entry, it can be part of cleanup of cpufreq subsystem.

Go thought the mail threads where that support is added, it was indeed
useful.

-- 
viresh

Re: [PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Srinivas Pandruvada]

On Wed, 2015-10-07 at 22:33 +0530, Viresh Kumar wrote:
> On 07-10-15, 09:18, Srinivas Pandruvada wrote:
> > freq_table is allocated in init() callbacks and freed on exit()
> > callback. But I don't see any driver changing policy->freq_table to NULL
> > or setting value to policy->freq_table to their allocated freq_table .
>  
> That's what I am asking for.
> 
> > I see policy->freq_table is only assigned value in function
> > cpufreq_table_validate_and_show, as part of the core API.
> 
> Its just an helper written to reduce code redundancy..
> 
> > So I think we
> > should set to NULL after clients freed the freq_table as part of core
> > function.
> 
> Not really. If we want to do the opposite, then we should initiate
> policy->freq_table = NULL, right from exit(), as its done as part of
> init() first.
> 
> > Otherwise we need to modify every client cpufreq driver and assign
> > policy->freq_table=NULL on .exit().  Since they don't assign value to
> > this policy->freq_table in first place, they shouldn't change this.
> 
> They did set it in the first place, with help of a helper routine
> though.
> 
> But I do understand the code redundancy we are about to create. So,
> maybe we can do this in the core as a special case.
> 
> But then you missed another location where exit() was called.
In cpufreq_online() function? In this case whole policy is freed. Do we
need to assign here also?

> 
> > Also I don't know why we need to keep the sysfs entry cpufreq after
> > offline. There are other values, which we can read without crash, but
> > may not be valid. We may delete this sysfs entry. But may be some
> > cpufreq driver cares about this even after offline. If we can delete
> > this entry, it can be part of cleanup of cpufreq subsystem.
> 
> Go thought the mail threads where that support is added, it was indeed
> useful.
> 

Re: [PATCH 2/2] cpufreq: prevent lockup on reading scaling_available_frequencies

[Viresh Kumar]

On 01-10-15, 15:23, Srinivas Pandruvada wrote:
> When scaling_available_frequencies is read on an offlined cpu, then
> either lockup or junk values are displayed. This is caused by
> freed freq_table, which policy is using.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/cpufreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index ef5ed94..25c4c15 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu)
>  	 * since this is a core component, and is essential for the
>  	 * subsequent light-weight ->init() to succeed.
>  	 */
> -	if (cpufreq_driver->exit)
> +	if (cpufreq_driver->exit) {
>  		cpufreq_driver->exit(policy);
> +		policy->freq_table = NULL;
> +	}
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 1/2] cpufreq: acpi_cpufreq: prevent crash on reading freqdomain_cpus

[Viresh Kumar]

On 01-10-15, 15:23, Srinivas Pandruvada wrote:
> When freqdomain_cpus attribute is read from an offlined cpu, it will
> cause crash. This change prevents calling cpufreq_show_cpus when
> mask is NULL.

Not exactly, you prevent it when the driver_data is NULL and not the
mask.

> Alternatively we can add this NULL check in
> cpufreq_show_cpus function.

That will be a different change then, as we aren't comparing the mask
now. I will just remove the above line from the commit log, to say
what we are doing, not what we can also do.

> Crash info:
> 
> [  170.814949] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> [  170.814990] IP: [<ffffffff813b2490>] _find_next_bit.part.0+0x10/0x70
> [  170.815021] PGD 227d30067 PUD 229e56067 PMD 0
> [  170.815043] Oops: 0000 [#2] SMP
> [  170.816022] CPU: 3 PID: 3121 Comm: cat Tainted: G      D    OE   4.3.0-rc3+ #33
> ...
> ...
> [  170.816657] Call Trace:
> [  170.816672]  [<ffffffff813b2505>] ? find_next_bit+0x15/0x20
> [  170.816696]  [<ffffffff8160e47c>] cpufreq_show_cpus+0x5c/0xd0
> [  170.816722]  [<ffffffffa031a409>] show_freqdomain_cpus+0x19/0x20 [acpi_cpufreq]
> [  170.816749]  [<ffffffff8160e65b>] show+0x3b/0x60
> [  170.816769]  [<ffffffff8129b31c>] sysfs_kf_seq_show+0xbc/0x130
> [  170.816793]  [<ffffffff81299be3>] kernfs_seq_show+0x23/0x30
> [  170.816816]  [<ffffffff81240f2c>] seq_read+0xec/0x390
> [  170.816837]  [<ffffffff8129a64a>] kernfs_fop_read+0x10a/0x160
> [  170.816861]  [<ffffffff8121d9b7>] __vfs_read+0x37/0x100
> [  170.816883]  [<ffffffff813217c0>] ? security_file_permission+0xa0/0xc0
> [  170.816909]  [<ffffffff8121e2e3>] vfs_read+0x83/0x130
> [  170.816930]  [<ffffffff8121f035>] SyS_read+0x55/0xc0
> ...
> ...
> [  170.817185] ---[ end trace bc6eadf82b2b965a ]---
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 7982772..cec1ee2 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -149,6 +149,9 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>  {
>  	struct acpi_cpufreq_data *data = policy->driver_data;
>  
> +	if (unlikely(!data))
> +		return -ENODEV;
> +
>  	return cpufreq_show_cpus(data->freqdomain_cpus, buf);
>  }

Change looks fine though.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 1/2] cpufreq: acpi_cpufreq: prevent crash on reading freqdomain_cpus

[Srinivas Pandruvada]

On Wed, 2015-10-07 at 17:49 +0530, Viresh Kumar wrote:
> On 01-10-15, 15:23, Srinivas Pandruvada wrote:
> > When freqdomain_cpus attribute is read from an offlined cpu, it will
> > cause crash. This change prevents calling cpufreq_show_cpus when
> > mask is NULL.
> 
> Not exactly, you prevent it when the driver_data is NULL and not the
> mask.
correct, I will change the description.
> 
> > Alternatively we can add this NULL check in
> > cpufreq_show_cpus function.
> 
> That will be a different change then, as we aren't comparing the mask
> now. I will just remove the above line from the commit log, to say
> what we are doing, not what we can also do.
> 
> > Crash info:
> > 
> > [  170.814949] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> > [  170.814990] IP: [<ffffffff813b2490>] _find_next_bit.part.0+0x10/0x70
> > [  170.815021] PGD 227d30067 PUD 229e56067 PMD 0
> > [  170.815043] Oops: 0000 [#2] SMP
> > [  170.816022] CPU: 3 PID: 3121 Comm: cat Tainted: G      D    OE   4.3.0-rc3+ #33
> > ...
> > ...
> > [  170.816657] Call Trace:
> > [  170.816672]  [<ffffffff813b2505>] ? find_next_bit+0x15/0x20
> > [  170.816696]  [<ffffffff8160e47c>] cpufreq_show_cpus+0x5c/0xd0
> > [  170.816722]  [<ffffffffa031a409>] show_freqdomain_cpus+0x19/0x20 [acpi_cpufreq]
> > [  170.816749]  [<ffffffff8160e65b>] show+0x3b/0x60
> > [  170.816769]  [<ffffffff8129b31c>] sysfs_kf_seq_show+0xbc/0x130
> > [  170.816793]  [<ffffffff81299be3>] kernfs_seq_show+0x23/0x30
> > [  170.816816]  [<ffffffff81240f2c>] seq_read+0xec/0x390
> > [  170.816837]  [<ffffffff8129a64a>] kernfs_fop_read+0x10a/0x160
> > [  170.816861]  [<ffffffff8121d9b7>] __vfs_read+0x37/0x100
> > [  170.816883]  [<ffffffff813217c0>] ? security_file_permission+0xa0/0xc0
> > [  170.816909]  [<ffffffff8121e2e3>] vfs_read+0x83/0x130
> > [  170.816930]  [<ffffffff8121f035>] SyS_read+0x55/0xc0
> > ...
> > ...
> > [  170.817185] ---[ end trace bc6eadf82b2b965a ]---
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/cpufreq/acpi-cpufreq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index 7982772..cec1ee2 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -149,6 +149,9 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
> >  {
> >  	struct acpi_cpufreq_data *data = policy->driver_data;
> >  
> > +	if (unlikely(!data))
> > +		return -ENODEV;
> > +
> >  	return cpufreq_show_cpus(data->freqdomain_cpus, buf);
> >  }
> 
> Change looks fine though.
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>