Message ID | 1583133336-7832-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: X86: Fix dereference null cpufreq policy | expand |
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); > + }
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).
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
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
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.
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
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);