Re: [PATCH 2/2] LoongArch: KVM: Protect kvm_io_bus_{read,write}() with SRCU

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 3, 2024 at 4:51 PM bibo mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2024/12/3 下午2:50, Huacai Chen wrote:
> > When we enable lockdep we get such a warning:
> >
> >   =============================
> >   WARNING: suspicious RCU usage
> >   6.12.0-rc7+ #1891 Tainted: G        W
> >   -----------------------------
> >   arch/loongarch/kvm/../../../virt/kvm/kvm_main.c:5945 suspicious rcu_dereference_check() usage!
> >   other info that might help us debug this:
> >   rcu_scheduler_active = 2, debug_locks = 1
> >   1 lock held by qemu-system-loo/948:
> >    #0: 90000001184a00a8 (&vcpu->mutex){+.+.}-{4:4}, at: kvm_vcpu_ioctl+0xf4/0xe20 [kvm]
> >   stack backtrace:
> >   CPU: 2 UID: 0 PID: 948 Comm: qemu-system-loo Tainted: G        W          6.12.0-rc7+ #1891
> >   Tainted: [W]=WARN
> >   Hardware name: Loongson Loongson-3A5000-7A1000-1w-CRB/Loongson-LS3A5000-7A1000-1w-CRB, BIOS vUDK2018-LoongArch-V2.0.0-prebeta9 10/21/2022
> >   Stack : 0000000000000089 9000000005a0db9c 90000000071519c8 900000012c578000
> >           900000012c57b940 0000000000000000 900000012c57b948 9000000007e53788
> >           900000000815bcc8 900000000815bcc0 900000012c57b7b0 0000000000000001
> >           0000000000000001 4b031894b9d6b725 0000000005dec000 9000000100427b00
> >           00000000000003d2 0000000000000001 000000000000002d 0000000000000003
> >           0000000000000030 00000000000003b4 0000000005dec000 0000000000000000
> >           900000000806d000 9000000007e53788 00000000000000b4 0000000000000004
> >           0000000000000004 0000000000000000 0000000000000000 9000000107baf600
> >           9000000008916000 9000000007e53788 9000000005924778 000000001fe001e5
> >           00000000000000b0 0000000000000007 0000000000000000 0000000000071c1d
> >           ...
> >   Call Trace:
> >   [<9000000005924778>] show_stack+0x38/0x180
> >   [<90000000071519c4>] dump_stack_lvl+0x94/0xe4
> >   [<90000000059eb754>] lockdep_rcu_suspicious+0x194/0x240
> >   [<ffff80000221f47c>] kvm_io_bus_read+0x19c/0x1e0 [kvm]
> >   [<ffff800002225118>] kvm_emu_mmio_read+0xd8/0x440 [kvm]
> >   [<ffff8000022254bc>] kvm_handle_read_fault+0x3c/0xe0 [kvm]
> >   [<ffff80000222b3c8>] kvm_handle_exit+0x228/0x480 [kvm]
> >
> > Fix it by protecting kvm_io_bus_{read,write}() with SRCU.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > ---
> >   arch/loongarch/kvm/exit.c     | 31 +++++++++++++++++++++----------
> >   arch/loongarch/kvm/intc/ipi.c |  6 +++++-
> >   2 files changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/loongarch/kvm/exit.c b/arch/loongarch/kvm/exit.c
> > index 69f3e3782cc9..a7893bd01e73 100644
> > --- a/arch/loongarch/kvm/exit.c
> > +++ b/arch/loongarch/kvm/exit.c
> > @@ -156,7 +156,7 @@ static int kvm_handle_csr(struct kvm_vcpu *vcpu, larch_inst inst)
> >
> >   int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu)
> >   {
> > -     int ret;
> > +     int idx, ret;
> >       unsigned long *val;
> >       u32 addr, rd, rj, opcode;
> >
> > @@ -167,7 +167,6 @@ int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu)
> >       rj = inst.reg2_format.rj;
> >       opcode = inst.reg2_format.opcode;
> >       addr = vcpu->arch.gprs[rj];
> > -     ret = EMULATE_DO_IOCSR;
> >       run->iocsr_io.phys_addr = addr;
> >       run->iocsr_io.is_write = 0;
> >       val = &vcpu->arch.gprs[rd];
> > @@ -207,20 +206,28 @@ int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu)
> >       }
> >
> >       if (run->iocsr_io.is_write) {
> > -             if (!kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val))
> > +             idx = srcu_read_lock(&vcpu->kvm->srcu);
> > +             ret = kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val);
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> > +             if (ret == 0)
> >                       ret = EMULATE_DONE;
> > -             else
> > +             else {
> > +                     ret = EMULATE_DO_IOCSR;
> >                       /* Save data and let user space to write it */
> >                       memcpy(run->iocsr_io.data, val, run->iocsr_io.len);
> > -
> > +             }
> >               trace_kvm_iocsr(KVM_TRACE_IOCSR_WRITE, run->iocsr_io.len, addr, val);
> >       } else {
> > -             if (!kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val))
> > +             idx = srcu_read_lock(&vcpu->kvm->srcu);
> > +             ret = kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val);
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> > +             if (ret == 0)
> >                       ret = EMULATE_DONE;
> > -             else
> > +             else {
> > +                     ret = EMULATE_DO_IOCSR;
> >                       /* Save register id for iocsr read completion */
> >                       vcpu->arch.io_gpr = rd;
> > -
> > +             }
> >               trace_kvm_iocsr(KVM_TRACE_IOCSR_READ, run->iocsr_io.len, addr, NULL);
> >       }
> >
> > @@ -359,7 +366,7 @@ static int kvm_handle_gspr(struct kvm_vcpu *vcpu)
> >
> >   int kvm_emu_mmio_read(struct kvm_vcpu *vcpu, larch_inst inst)
> >   {
> > -     int ret;
> > +     int idx, ret;
> >       unsigned int op8, opcode, rd;
> >       struct kvm_run *run = vcpu->run;
> >
> > @@ -464,8 +471,10 @@ int kvm_emu_mmio_read(struct kvm_vcpu *vcpu, larch_inst inst)
> >                * it need not return to user space to handle the mmio
> >                * exception.
> >                */
> > +             idx = srcu_read_lock(&vcpu->kvm->srcu);
> >               ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, vcpu->arch.badv,
> >                                     run->mmio.len, &vcpu->arch.gprs[rd]);
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >               if (!ret) {
> >                       update_pc(&vcpu->arch);
> >                       vcpu->mmio_needed = 0;
> > @@ -531,7 +540,7 @@ int kvm_complete_mmio_read(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >
> >   int kvm_emu_mmio_write(struct kvm_vcpu *vcpu, larch_inst inst)
> >   {
> > -     int ret;
> > +     int idx, ret;
> >       unsigned int rd, op8, opcode;
> >       unsigned long curr_pc, rd_val = 0;
> >       struct kvm_run *run = vcpu->run;
> > @@ -631,7 +640,9 @@ int kvm_emu_mmio_write(struct kvm_vcpu *vcpu, larch_inst inst)
> >                * it need not return to user space to handle the mmio
> >                * exception.
> >                */
> > +             idx = srcu_read_lock(&vcpu->kvm->srcu);
> >               ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, vcpu->arch.badv, run->mmio.len, data);
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >               if (!ret)
> >                       return EMULATE_DONE;
> >
> > diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c
> > index a233a323e295..4b7ff20ed438 100644
> > --- a/arch/loongarch/kvm/intc/ipi.c
> > +++ b/arch/loongarch/kvm/intc/ipi.c
> > @@ -98,7 +98,7 @@ static void write_mailbox(struct kvm_vcpu *vcpu, int offset, uint64_t data, int
> >
> >   static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data)
> >   {
> > -     int i, ret;
> > +     int i, idx, ret;
> >       uint32_t val = 0, mask = 0;
> >
> >       /*
> > @@ -107,7 +107,9 @@ static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data)
> >        */
> >       if ((data >> 27) & 0xf) {
> >               /* Read the old val */
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> here should be idx = srcu_read_lock(&vcpu->kvm->srcu) ?
>
> >               ret = kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, sizeof(val), &val);
> > +             srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >               if (unlikely(ret)) {
> >                       kvm_err("%s: : read date from addr %llx failed\n", __func__, addr);
> >                       return ret;
> > @@ -121,7 +123,9 @@ static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data)
> >               val &= mask;
> >       }
> >       val |= ((uint32_t)(data >> 32) & ~mask);
> > +     srcu_read_unlock(&vcpu->kvm->srcu, idx);
> here should be idx = srcu_read_lock(&vcpu->kvm->srcu)
Yes, this is my mistake, thank you.

Huacai

>
> >       ret = kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, sizeof(val), &val);
> > +     srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >       if (unlikely(ret))
> >               kvm_err("%s: : write date to addr %llx failed\n", __func__, addr);
> >
> >
> otherwise looks good to me.
>
> Reviewed-by: Bibo Mao <maobibo@xxxxxxxxxxx>
>
>





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux