| Message ID | 1499345511-23632-1-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/07/2017 14:51, Christian Borntraeger wrote: > We do use rcu to protect the pid pointer. Mark it as such and > adopt all code to use the proper access methods. > > This was detected by sparse. > "virt/kvm/kvm_main.c:2248:15: error: incompatible types in comparison > expression (different address spaces)" > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 15 +++++++++++---- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 0b50e7b..bcd37b8 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -234,7 +234,7 @@ struct kvm_vcpu { > > int guest_fpu_loaded, guest_xcr0_loaded; > struct swait_queue_head wq; > - struct pid *pid; > + struct pid __rcu *pid; > int sigset_active; > sigset_t sigset; > struct kvm_vcpu_stat stat; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 19f0ecb..7bd5509 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -293,7 +293,12 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init); > > void kvm_vcpu_uninit(struct kvm_vcpu *vcpu) > { > - put_pid(vcpu->pid); > + /* > + * no need for rcu_read_lock as VCPU_RUN is the only place that > + * will change the vcpu->pid pointer and on uninit all file > + * descriptors are already gone. > + */ > + put_pid(rcu_dereference(vcpu->pid)); > kvm_arch_vcpu_uninit(vcpu); > free_page((unsigned long)vcpu->run); > } > @@ -2551,13 +2556,14 @@ static long kvm_vcpu_ioctl(struct file *filp, > if (r) > return r; > switch (ioctl) { > - case KVM_RUN: > + case KVM_RUN: { > + struct pid *oldpid; > r = -EINVAL; > if (arg) > goto out; > - if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) { > + oldpid = rcu_dereference(vcpu->pid); > + if (unlikely(oldpid != current->pids[PIDTYPE_PID].pid)) { Since we never dereference oldpid, the rcu_dereference should be rcu_access_pointer instead. Otherwise Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > /* The thread running this VCPU changed. */ > - struct pid *oldpid = vcpu->pid; > struct pid *newpid = get_task_pid(current, PIDTYPE_PID); > > rcu_assign_pointer(vcpu->pid, newpid); > @@ -2568,6 +2574,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); > trace_kvm_userspace_exit(vcpu->run->exit_reason, r); > break; > + } > case KVM_GET_REGS: { > struct kvm_regs *kvm_regs; > >
On 06/07/2017 14:51, Christian Borntraeger wrote: > + /* > + * no need for rcu_read_lock as VCPU_RUN is the only place that > + * will change the vcpu->pid pointer and on uninit all file > + * descriptors are already gone. > + */ > + put_pid(rcu_dereference(vcpu->pid)); Hmm, I missed this. This would fail with lockdep turned on, so you need rcu_dereference_protected(p, 1) Paolo
On 07/06/2017 03:04 PM, Paolo Bonzini wrote: > > > On 06/07/2017 14:51, Christian Borntraeger wrote: >> + /* >> + * no need for rcu_read_lock as VCPU_RUN is the only place that >> + * will change the vcpu->pid pointer and on uninit all file >> + * descriptors are already gone. >> + */ >> + put_pid(rcu_dereference(vcpu->pid)); > > Hmm, I missed this. > > This would fail with lockdep turned on, so you need > > rcu_dereference_protected(p, 1) yes, will fix.
On 07/06/2017 03:01 PM, Paolo Bonzini wrote: > > > On 06/07/2017 14:51, Christian Borntraeger wrote: >> We do use rcu to protect the pid pointer. Mark it as such and >> adopt all code to use the proper access methods. >> >> This was detected by sparse. >> "virt/kvm/kvm_main.c:2248:15: error: incompatible types in comparison >> expression (different address spaces)" >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> include/linux/kvm_host.h | 2 +- >> virt/kvm/kvm_main.c | 15 +++++++++++---- >> 2 files changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 0b50e7b..bcd37b8 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -234,7 +234,7 @@ struct kvm_vcpu { >> >> int guest_fpu_loaded, guest_xcr0_loaded; >> struct swait_queue_head wq; >> - struct pid *pid; >> + struct pid __rcu *pid; >> int sigset_active; >> sigset_t sigset; >> struct kvm_vcpu_stat stat; >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 19f0ecb..7bd5509 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -293,7 +293,12 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init); >> >> void kvm_vcpu_uninit(struct kvm_vcpu *vcpu) >> { >> - put_pid(vcpu->pid); >> + /* >> + * no need for rcu_read_lock as VCPU_RUN is the only place that >> + * will change the vcpu->pid pointer and on uninit all file >> + * descriptors are already gone. >> + */ >> + put_pid(rcu_dereference(vcpu->pid)); >> kvm_arch_vcpu_uninit(vcpu); >> free_page((unsigned long)vcpu->run); >> } >> @@ -2551,13 +2556,14 @@ static long kvm_vcpu_ioctl(struct file *filp, >> if (r) >> return r; >> switch (ioctl) { >> - case KVM_RUN: >> + case KVM_RUN: { >> + struct pid *oldpid; >> r = -EINVAL; >> if (arg) >> goto out; >> - if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) { >> + oldpid = rcu_dereference(vcpu->pid); >> + if (unlikely(oldpid != current->pids[PIDTYPE_PID].pid)) { > > Since we never dereference oldpid, the rcu_dereference should be > rcu_access_pointer instead. Right. Will fix and send a v2. > > Otherwise > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > >> /* The thread running this VCPU changed. */ >> - struct pid *oldpid = vcpu->pid; >> struct pid *newpid = get_task_pid(current, PIDTYPE_PID); >> >> rcu_assign_pointer(vcpu->pid, newpid); >> @@ -2568,6 +2574,7 @@ static long kvm_vcpu_ioctl(struct file *filp, >> r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); >> trace_kvm_userspace_exit(vcpu->run->exit_reason, r); >> break; >> + } >> case KVM_GET_REGS: { >> struct kvm_regs *kvm_regs; >> >> >
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 0b50e7b..bcd37b8 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -234,7 +234,7 @@ struct kvm_vcpu { int guest_fpu_loaded, guest_xcr0_loaded; struct swait_queue_head wq; - struct pid *pid; + struct pid __rcu *pid; int sigset_active; sigset_t sigset; struct kvm_vcpu_stat stat; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 19f0ecb..7bd5509 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -293,7 +293,12 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init); void kvm_vcpu_uninit(struct kvm_vcpu *vcpu) { - put_pid(vcpu->pid); + /* + * no need for rcu_read_lock as VCPU_RUN is the only place that + * will change the vcpu->pid pointer and on uninit all file + * descriptors are already gone. + */ + put_pid(rcu_dereference(vcpu->pid)); kvm_arch_vcpu_uninit(vcpu); free_page((unsigned long)vcpu->run); } @@ -2551,13 +2556,14 @@ static long kvm_vcpu_ioctl(struct file *filp, if (r) return r; switch (ioctl) { - case KVM_RUN: + case KVM_RUN: { + struct pid *oldpid; r = -EINVAL; if (arg) goto out; - if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) { + oldpid = rcu_dereference(vcpu->pid); + if (unlikely(oldpid != current->pids[PIDTYPE_PID].pid)) { /* The thread running this VCPU changed. */ - struct pid *oldpid = vcpu->pid; struct pid *newpid = get_task_pid(current, PIDTYPE_PID); rcu_assign_pointer(vcpu->pid, newpid); @@ -2568,6 +2574,7 @@ static long kvm_vcpu_ioctl(struct file *filp, r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); trace_kvm_userspace_exit(vcpu->run->exit_reason, r); break; + } case KVM_GET_REGS: { struct kvm_regs *kvm_regs;
We do use rcu to protect the pid pointer. Mark it as such and adopt all code to use the proper access methods. This was detected by sparse. "virt/kvm/kvm_main.c:2248:15: error: incompatible types in comparison expression (different address spaces)" Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- include/linux/kvm_host.h | 2 +- virt/kvm/kvm_main.c | 15 +++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-)