linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: X86: Fix dereference null cpufreq policy
@ 2020-03-02  7:15 Wanpeng Li
  2020-03-02  7:55 ` Paolo Bonzini
  2020-03-02  8:58 ` Naresh Kamboju
  0 siblings, 2 replies; 7+ messages in thread
From: Wanpeng Li @ 2020-03-02  7:15 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Naresh Kamboju

From: Wanpeng Li <wanpengli@tencent.com>

Naresh Kamboju reported:

   Linux version 5.6.0-rc4 (oe-user@oe-host) (gcc version
  (GCC)) #1 SMP Sun Mar 1 22:59:08 UTC 2020
   kvm: no hardware support
   BUG: kernel NULL pointer dereference, address: 000000000000028c
   #PF: supervisor read access in kernel mode
   #PF: error_code(0x0000) - not-present page
   PGD 0 P4D 0
   Oops: 0000 [#1] SMP NOPTI
   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc4 #1
   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
  04/01/2014
   RIP: 0010:kobject_put+0x12/0x1c0
   Call Trace:
    cpufreq_cpu_put+0x15/0x20
    kvm_arch_init+0x1f6/0x2b0
    kvm_init+0x31/0x290
    ? svm_check_processor_compat+0xd/0xd
    ? svm_check_processor_compat+0xd/0xd
    svm_init+0x21/0x23
    do_one_initcall+0x61/0x2f0
    ? rdinit_setup+0x30/0x30
    ? rcu_read_lock_sched_held+0x4f/0x80
    kernel_init_freeable+0x219/0x279
    ? rest_init+0x250/0x250
    kernel_init+0xe/0x110
    ret_from_fork+0x27/0x50
   Modules linked in:
   CR2: 000000000000028c
   ---[ end trace 239abf40c55c409b ]---
   RIP: 0010:kobject_put+0x12/0x1c0

cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
this patch takes care of it.

Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5de2006..3156e25 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7195,10 +7195,12 @@ static void kvm_timer_init(void)
 
 		cpu = get_cpu();
 		policy = cpufreq_cpu_get(cpu);
-		if (policy && policy->cpuinfo.max_freq)
-			max_tsc_khz = policy->cpuinfo.max_freq;
+		if (policy) {
+			if (policy->cpuinfo.max_freq)
+				max_tsc_khz = policy->cpuinfo.max_freq;
+			cpufreq_cpu_put(policy);
+		}
 		put_cpu();
-		cpufreq_cpu_put(policy);
 #endif
 		cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block,
 					  CPUFREQ_TRANSITION_NOTIFIER);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  7:15 [PATCH] KVM: X86: Fix dereference null cpufreq policy Wanpeng Li
@ 2020-03-02  7:55 ` Paolo Bonzini
  2020-03-02  8:12   ` Viresh Kumar
  2020-03-02  8:58 ` Naresh Kamboju
  1 sibling, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2020-03-02  7:55 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Naresh Kamboju, Rafael J. Wysocki, Viresh Kumar

On 02/03/20 08:15, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
> this patch takes care of it.
> 
> Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>

My bad, I checked kobject_put but didn't check that kobj is first in
struct cpufreq_policy.

I think we should do this in cpufreq_cpu_put or, even better, move the
kobject struct first in struct cpufreq_policy.  Rafael, Viresh, any
objection?

Paolo

>  		policy = cpufreq_cpu_get(cpu);
> -		if (policy && policy->cpuinfo.max_freq)
> -			max_tsc_khz = policy->cpuinfo.max_freq;
> +		if (policy) {
> +			if (policy->cpuinfo.max_freq)
> +				max_tsc_khz = policy->cpuinfo.max_freq;
> +			cpufreq_cpu_put(policy);
> +		}


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  7:55 ` Paolo Bonzini
@ 2020-03-02  8:12   ` Viresh Kumar
  2020-03-02  8:39     ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2020-03-02  8:12 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Wanpeng Li, linux-kernel, kvm, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Naresh Kamboju, Rafael J. Wysocki

On 02-03-20, 08:55, Paolo Bonzini wrote:
> On 02/03/20 08:15, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> > 
> > cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
> > this patch takes care of it.
> > 
> > Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
> > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> 
> My bad, I checked kobject_put but didn't check that kobj is first in
> struct cpufreq_policy.
> 
> I think we should do this in cpufreq_cpu_put or, even better, move the
> kobject struct first in struct cpufreq_policy.  Rafael, Viresh, any
> objection?
> 
> Paolo
> 
> >  		policy = cpufreq_cpu_get(cpu);
> > -		if (policy && policy->cpuinfo.max_freq)
> > -			max_tsc_khz = policy->cpuinfo.max_freq;
> > +		if (policy) {
> > +			if (policy->cpuinfo.max_freq)
> > +				max_tsc_khz = policy->cpuinfo.max_freq;
> > +			cpufreq_cpu_put(policy);
> > +		}

I think this change makes sense and I am not sure why should we even
try to support cpufreq_cpu_put(NULL).

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  8:12   ` Viresh Kumar
@ 2020-03-02  8:39     ` Paolo Bonzini
  2020-03-02  9:14       ` Viresh Kumar
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2020-03-02  8:39 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Wanpeng Li, linux-kernel, kvm, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Naresh Kamboju, Rafael J. Wysocki

On 02/03/20 09:12, Viresh Kumar wrote:
> On 02-03-20, 08:55, Paolo Bonzini wrote:
>> On 02/03/20 08:15, Wanpeng Li wrote:
>>> From: Wanpeng Li <wanpengli@tencent.com>
>>>
>>> cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
>>> this patch takes care of it.
>>>
>>> Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
>>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>>> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
>>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>>
>> My bad, I checked kobject_put but didn't check that kobj is first in
>> struct cpufreq_policy.
>>
>> I think we should do this in cpufreq_cpu_put or, even better, move the
>> kobject struct first in struct cpufreq_policy.  Rafael, Viresh, any
>> objection?
>>
>> Paolo
>>
>>>  		policy = cpufreq_cpu_get(cpu);
>>> -		if (policy && policy->cpuinfo.max_freq)
>>> -			max_tsc_khz = policy->cpuinfo.max_freq;
>>> +		if (policy) {
>>> +			if (policy->cpuinfo.max_freq)
>>> +				max_tsc_khz = policy->cpuinfo.max_freq;
>>> +			cpufreq_cpu_put(policy);
>>> +		}
> 
> I think this change makes sense and I am not sure why should we even
> try to support cpufreq_cpu_put(NULL).

For the same reason why we support kfree(NULL) and kobject_put(NULL)?

Paolo


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  7:15 [PATCH] KVM: X86: Fix dereference null cpufreq policy Wanpeng Li
  2020-03-02  7:55 ` Paolo Bonzini
@ 2020-03-02  8:58 ` Naresh Kamboju
  1 sibling, 0 replies; 7+ messages in thread
From: Naresh Kamboju @ 2020-03-02  8:58 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: open list, kvm list, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel

On Mon, 2 Mar 2020 at 12:48, Wanpeng Li <kernellwp@gmail.com> wrote:
>
> From: Wanpeng Li <wanpengli@tencent.com>
>
> Naresh Kamboju reported:
>
>    Linux version 5.6.0-rc4 (oe-user@oe-host) (gcc version
>   (GCC)) #1 SMP Sun Mar 1 22:59:08 UTC 2020
>    kvm: no hardware support
>    BUG: kernel NULL pointer dereference, address: 000000000000028c
>    #PF: supervisor read access in kernel mode
>    #PF: error_code(0x0000) - not-present page
>    PGD 0 P4D 0
>    Oops: 0000 [#1] SMP NOPTI
>    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc4 #1
>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>   04/01/2014
>    RIP: 0010:kobject_put+0x12/0x1c0
>    Call Trace:
>     cpufreq_cpu_put+0x15/0x20
>     kvm_arch_init+0x1f6/0x2b0
>     kvm_init+0x31/0x290
>     ? svm_check_processor_compat+0xd/0xd
>     ? svm_check_processor_compat+0xd/0xd
>     svm_init+0x21/0x23
>     do_one_initcall+0x61/0x2f0
>     ? rdinit_setup+0x30/0x30
>     ? rcu_read_lock_sched_held+0x4f/0x80
>     kernel_init_freeable+0x219/0x279
>     ? rest_init+0x250/0x250
>     kernel_init+0xe/0x110
>     ret_from_fork+0x27/0x50
>    Modules linked in:
>    CR2: 000000000000028c
>    ---[ end trace 239abf40c55c409b ]---
>    RIP: 0010:kobject_put+0x12/0x1c0
>
> cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
> this patch takes care of it.
>
> Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

Applied this patch and test boot pass.

- Naresh

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  8:39     ` Paolo Bonzini
@ 2020-03-02  9:14       ` Viresh Kumar
  2020-03-02 10:23         ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2020-03-02  9:14 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Wanpeng Li, linux-kernel, kvm, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Naresh Kamboju, Rafael J. Wysocki

On 02-03-20, 09:39, Paolo Bonzini wrote:
> On 02/03/20 09:12, Viresh Kumar wrote:
> > On 02-03-20, 08:55, Paolo Bonzini wrote:
> >> On 02/03/20 08:15, Wanpeng Li wrote:
> >>> From: Wanpeng Li <wanpengli@tencent.com>
> >>>
> >>> cpufreq policy which is get by cpufreq_cpu_get() can be NULL if it is failure,
> >>> this patch takes care of it.
> >>>
> >>> Fixes: aaec7c03de (KVM: x86: avoid useless copy of cpufreq policy)
> >>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> >>> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> >>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> >>
> >> My bad, I checked kobject_put but didn't check that kobj is first in
> >> struct cpufreq_policy.
> >>
> >> I think we should do this in cpufreq_cpu_put or, even better, move the
> >> kobject struct first in struct cpufreq_policy.  Rafael, Viresh, any
> >> objection?
> >>
> >> Paolo
> >>
> >>>  		policy = cpufreq_cpu_get(cpu);
> >>> -		if (policy && policy->cpuinfo.max_freq)
> >>> -			max_tsc_khz = policy->cpuinfo.max_freq;
> >>> +		if (policy) {
> >>> +			if (policy->cpuinfo.max_freq)
> >>> +				max_tsc_khz = policy->cpuinfo.max_freq;
> >>> +			cpufreq_cpu_put(policy);
> >>> +		}
> > 
> > I think this change makes sense and I am not sure why should we even
> > try to support cpufreq_cpu_put(NULL).
> 
> For the same reason why we support kfree(NULL) and kobject_put(NULL)?

These two helpers are used widely within kernel and many a times the
resource is taken by one routine and dropped by another, and so
someone needed to check if it can call the resource-free helper safely
or not. IMO, that's not the case with cpufreq_cpu_put(). It is used
mostly by the cpufreq core only and not too often by external
entities. And even in that case we don't need to call
cpufreq_cpu_put() from a different routine than the one which called
cpufreq_cpu_get(). Like in your case. And so there is no need of an
extra check to be made.

I don't think we need to support cpufreq_cpu_put(NULL), but if Rafael
wants it to be supported, I won't object to it.

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: X86: Fix dereference null cpufreq policy
  2020-03-02  9:14       ` Viresh Kumar
@ 2020-03-02 10:23         ` Paolo Bonzini
  0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2020-03-02 10:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Wanpeng Li, linux-kernel, kvm, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Naresh Kamboju, Rafael J. Wysocki

On 02/03/20 10:14, Viresh Kumar wrote:
>> For the same reason why we support kfree(NULL) and kobject_put(NULL)?
>
> These two helpers are used widely within kernel and many a times the
> resource is taken by one routine and dropped by another, and so
> someone needed to check if it can call the resource-free helper safely
> or not. IMO, that's not the case with cpufreq_cpu_put(). It is used
> mostly by the cpufreq core only and not too often by external
> entities. And even in that case we don't need to call
> cpufreq_cpu_put() from a different routine than the one which called
> cpufreq_cpu_get(). Like in your case. And so there is no need of an
> extra check to be made.
> 
> I don't think we need to support cpufreq_cpu_put(NULL), but if Rafael
> wants it to be supported, I won't object to it.

Actually I think kobject_put is wrong in supporting NULL, because
documentation explicitly says to use container_of and not place kobj as
the first item.  However, there is going to be some place in the kernel
that relies on it, so either removing the check or moving all kobjs to
the beginning of the struct is a windmill fight.  I'll just apply
Wanpeng's patch.

Paolo


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-02 10:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-02  7:15 [PATCH] KVM: X86: Fix dereference null cpufreq policy Wanpeng Li
2020-03-02  7:55 ` Paolo Bonzini
2020-03-02  8:12   ` Viresh Kumar
2020-03-02  8:39     ` Paolo Bonzini
2020-03-02  9:14       ` Viresh Kumar
2020-03-02 10:23         ` Paolo Bonzini
2020-03-02  8:58 ` Naresh Kamboju

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).