| Message ID | 20240110011533.503302-5-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Async #PF fixes and cleanups | expand |
On Tue, Jan 09, 2024 at 05:15:33PM -0800, Sean Christopherson wrote: > Nullify the async #PF worker's local "apf" pointer immediately after the > point where the structure can be freed by the vCPU. The existing comment > is helpful, but easy to overlook as there is no associated code. > > Update the comment to clarify that it can be freed by as soon as the lock > is dropped, as "after this point" isn't strictly accurate, nor does it > help understand what prevents the structure from being freed earlier. > > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Xu Yilun <yilun.xu@intel.com> > --- > virt/kvm/async_pf.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index c3f4f351a2ae..1088c6628de9 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -83,13 +83,14 @@ static void async_pf_execute(struct work_struct *work) > apf->vcpu = NULL; > spin_unlock(&vcpu->async_pf.lock); > > - if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) > - kvm_arch_async_page_present_queued(vcpu); > - > /* > - * apf may be freed by kvm_check_async_pf_completion() after > - * this point > + * The apf struct may freed by kvm_check_async_pf_completion() as soon > + * as the lock is dropped. Nullify it to prevent improper usage. > */ > + apf = NULL; > + > + if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) > + kvm_arch_async_page_present_queued(vcpu); > > trace_kvm_async_pf_completed(addr, cr2_or_gpa); > > -- > 2.43.0.472.g3155946c3a-goog >
Sean Christopherson <seanjc@google.com> writes: > Nullify the async #PF worker's local "apf" pointer immediately after the > point where the structure can be freed by the vCPU. The existing comment > is helpful, but easy to overlook as there is no associated code. > > Update the comment to clarify that it can be freed by as soon as the lock > is dropped, as "after this point" isn't strictly accurate, nor does it > help understand what prevents the structure from being freed earlier. > "No functional change intended." must be made a requirement, especially for those who made it their trademark) > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/async_pf.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index c3f4f351a2ae..1088c6628de9 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -83,13 +83,14 @@ static void async_pf_execute(struct work_struct *work) > apf->vcpu = NULL; > spin_unlock(&vcpu->async_pf.lock); > > - if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) > - kvm_arch_async_page_present_queued(vcpu); > - > /* > - * apf may be freed by kvm_check_async_pf_completion() after > - * this point > + * The apf struct may freed by kvm_check_async_pf_completion() as soon Nit: "may be freed"/"may get freed" maybe? > + * as the lock is dropped. Nullify it to prevent improper usage. > */ > + apf = NULL; > + > + if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) > + kvm_arch_async_page_present_queued(vcpu); > > trace_kvm_async_pf_completed(addr, cr2_or_gpa); Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c index c3f4f351a2ae..1088c6628de9 100644 --- a/virt/kvm/async_pf.c +++ b/virt/kvm/async_pf.c @@ -83,13 +83,14 @@ static void async_pf_execute(struct work_struct *work) apf->vcpu = NULL; spin_unlock(&vcpu->async_pf.lock); - if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) - kvm_arch_async_page_present_queued(vcpu); - /* - * apf may be freed by kvm_check_async_pf_completion() after - * this point + * The apf struct may freed by kvm_check_async_pf_completion() as soon + * as the lock is dropped. Nullify it to prevent improper usage. */ + apf = NULL; + + if (!IS_ENABLED(CONFIG_KVM_ASYNC_PF_SYNC) && first) + kvm_arch_async_page_present_queued(vcpu); trace_kvm_async_pf_completed(addr, cr2_or_gpa);
Nullify the async #PF worker's local "apf" pointer immediately after the point where the structure can be freed by the vCPU. The existing comment is helpful, but easy to overlook as there is no associated code. Update the comment to clarify that it can be freed by as soon as the lock is dropped, as "after this point" isn't strictly accurate, nor does it help understand what prevents the structure from being freed earlier. Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/async_pf.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)