Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[Huacai Chen]

On Wed, Sep 30, 2026 at 9:41 AM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2026/9/29 下午6:28, Tao Cui wrote:
> > From: Tao Cui <cuitao@xxxxxxxxxx>
> >
> > pch_msi_set_irq() reads kvm->arch.dmsintc several times: the non-NULL
> > check and the address-window comparison each reload the pointer, so a
> > concurrent device removal can be observed between them and the
> > following dereference hits a freed object.
> >
> > Load the pointer once at the top of pch_msi_set_irq() and add a NULL
> > check with a local snapshot in dmsintc_set_irq().  Both loads use
> > READ_ONCE() so the compiler keeps them single.  This closes the
> > reload race; a narrower window where removal happens right after the
> > load remains, as the injection path takes no lock against destroy.
> >
> > Fixes: 03de5eecb0f0 ("LoongArch: KVM: Add DMSINTC inject msi to vCPU")
> > Signed-off-by: Tao Cui <cuitao@xxxxxxxxxx>
> > ---
> >   arch/loongarch/kvm/intc/dmsintc.c | 6 +++++-
> >   arch/loongarch/kvm/intc/pch_pic.c | 7 ++++---
> >   2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> > index 41c8b597b5bd..91a163698c3c 100644
> > --- a/arch/loongarch/kvm/intc/dmsintc.c
> > +++ b/arch/loongarch/kvm/intc/dmsintc.c
> > @@ -69,9 +69,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
> >   {
> >       unsigned int irq, cpu;
> >       struct kvm_vcpu *vcpu;
> > +     struct loongarch_dmsintc *s = READ_ONCE(kvm->arch.dmsintc);
> > +
> > +     if (!s)
> > +             return -EINVAL;
> NULL check with kvm->arch.dmsintc is already done in its caller function
> pch_msi_set_irq(). It is not necessary here.
> >
> >       irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
> > -     cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
> > +     cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
> >       if (cpu >= KVM_MAX_VCPUS)
> >               return -EINVAL;
> >       vcpu = kvm_get_vcpu_by_cpuid(kvm, cpu);
> > diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> > index 62c09b5f3937..30a8c65c7609 100644
> > --- a/arch/loongarch/kvm/intc/pch_pic.c
> > +++ b/arch/loongarch/kvm/intc/pch_pic.c
> > @@ -71,10 +71,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
> >   int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
> >   {
> >       u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
> > +     struct loongarch_dmsintc *dmsintc = READ_ONCE(kvm->arch.dmsintc);
> what is usage of READ_ONCE() here?
>
> If you want to simple the usage of kvm->arch.dmsintc in multiple places,
> just *struct loongarch_dmsintc *dmsintc = kvm->arch.dmsintc* is enough.
> And it is not fixup patch, it is code cleanup.
I completely don't think this patch is necessary.

Huacai

>
> Regards
> Bibo Mao
> >
> > -     if (cpu_has_msgint && kvm->arch.dmsintc &&
> > -             msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
> > -             msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
> > +     if (cpu_has_msgint && dmsintc &&
> > +             msg_addr >= dmsintc->msg_addr_base &&
> > +             msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
> >               return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
> >       }
> >
> >
>