Re: [PATCH v2] KVM: PPC: Book3S HV: Don't drop pending doorbell across L2 entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] KVM_SET_ONE_REG ioctl for KVM_REG_PPC_DPDES unconditionally increments doorbell_request, ignoring the userspace-provided value.
- [High] Changing DPDES doorbell tracking from a boolean state to a counting semaphore violates PowerISA semantics, causing spurious interrupts and leaking internal state.
- [High] Emulating the MSGCLRP instruction unconditionally decrements doorbell_request, allowing it to underflow to negative values and permanently mask future doorbells or fake pending ones.
- [High] TOCTOU race in nestedv2 entry/exit path silently drops concurrent doorbells that arrive while the L2 guest is running.
- [Medium] Unbounded queuing of doorbells via MSGSNDP allows an integer overflow that disables doorbells for sibling vCPUs.
--

--- Patch [1]: [PATCH v2] KVM: PPC: Book3S HV: Don't drop pending doorbell across L2 entry ---
commit 08c6169713a748bce823e2d3e8086fb958a9b53f
Author: Vaibhav Jain <vaibhav@linux.ibm.com>
Subject: KVM: PPC: Book3S HV: Don't drop pending doorbell across L2 entry

This commit converts the doorbell_request tracking to an atomic_t to
prevent the L1 from losing pending doorbells when an L2 exits without
handling them. It also updates APIv1 and Bare-Metal KVM infrastructure
on PPC64 to use atomic_t helpers.

Link: https://lore.kernel.org/linuxppc-dev/20260803034426.44249-1-vaibhav@linux.ibm.com/T/#u

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 61dbeea317f3b..fb80c13dbb298 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c

[ ... ]

> @@ -1557,17 +1558,16 @@ static int kvmppc_emulate_doorbell_instr(struct kvm_vcpu *vcpu)
>  		tvcpu = kvmppc_find_vcpu(kvm, vcpu->vcpu_id - thr + arg);
>  		if (!tvcpu)
>  			break;
> -		if (!tvcpu->arch.doorbell_request) {
> -			tvcpu->arch.doorbell_request = 1;
> +		if (atomic_inc_return(&tvcpu->arch.doorbell_request) >= 1)

[Severity: High]
PowerISA defines DPDES as a single-bit state. Does queuing doorbells by
allowing doorbell_request to increment beyond 1 violate these architectural
semantics? 

If multiple MSGSNDP instructions are executed, the count increases above 1,
which means a single MSGCLRP (which decrements by 1) might fail to clear the
pending state.

[Severity: Medium]
Also, is there anything preventing a guest from executing MSGSNDP repeatedly
to overflow this counter to a negative value, thereby disabling future
doorbells for a sibling vCPU?

>  			kvmppc_fast_vcpu_kick_hv(tvcpu);
> -		}
>  		break;
>  	case OP_31_XOP_MSGCLRP:
>  		arg = kvmppc_get_gpr(vcpu, rb);
>  		if (((arg >> 27) & 0x1f) != PPC_DBELL_SERVER)
>  			break;
> -		vcpu->arch.vcore->dpdes = 0;
> -		vcpu->arch.doorbell_request = 0;
> +
> +		if (atomic_dec_return(&vcpu->arch.doorbell_request) <= 0)
> +			vcpu->arch.vcore->dpdes = 0;

[Severity: High]
If a guest executes MSGCLRP when no doorbell is pending, does this
unconditional decrement allow doorbell_request to underflow to a negative
value?

If it drops below zero, kvmppc_doorbell_pending() will evaluate the negative
value as true:

kvmppc_doorbell_pending() {
    if (atomic_read(&vcpu->arch.doorbell_request))
        return true;
    ...
}

Could this permanently mask future doorbells or fake a pending doorbell,
preventing the vCPU from halting?

>  		break;
>  	case OP_31_XOP_MFSPR:

[ ... ]

> @@ -2313,7 +2313,7 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>  		 * On POWER8, doorbell_request is 0.
>  		 */
>  		if (cpu_has_feature(CPU_FTR_ARCH_300))
> -			*val = get_reg_val(id, vcpu->arch.doorbell_request);
> +			*val = get_reg_val(id, atomic_read(&vcpu->arch.doorbell_request));

[Severity: High]
Since doorbell_request can now exceed 1 due to the atomic_inc_return() above,
does this leak the internal integer counter to userspace? Userspace typically
expects a 1-bit boolean flag for KVM_REG_PPC_DPDES.

>  		else
>  			*val = get_reg_val(id, vcpu->arch.vcore->dpdes);
>  		break;

[ ... ]

> @@ -2565,7 +2565,7 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>  		break;
>  	case KVM_REG_PPC_DPDES:
>  		if (cpu_has_feature(CPU_FTR_ARCH_300))
> -			vcpu->arch.doorbell_request = set_reg_val(id, *val) & 1;
> +			atomic_inc(&vcpu->arch.doorbell_request);

[Severity: High]
Does this ignore the userspace-provided value (*val) completely and
unconditionally increment the counter?

If a userspace VMM like QEMU calls KVM_SET_ONE_REG with val=0 to restore
an idle vCPU, it looks like a phantom doorbell would be injected instead.

>  		else
>  			vcpu->arch.vcore->dpdes = set_reg_val(id, *val);
>  		break;

[ ... ]

> @@ -4298,6 +4298,17 @@ static int kvmhv_vcpu_entry_nestedv2(struct kvm_vcpu *vcpu, u64 time_limit,
>  
>  	timer_rearm_host_dec(*tb);
>  
> +	/* Check if privileged door bell was requested and handled */
> +	if (atomic_read(&vcpu->arch.doorbell_request) > 0) {
> +		/* In case PHYP doesn't return updated dpdes in output gsb */
> +		if (vcpu->arch.vcore->dpdes)
> +			kvmhv_nestedv2_cached_reload(vcpu,
> +						     KVMPPC_GSID_DPDES);
> +		/* if dpdes was handled then reduce the doorbell count */
> +		if (!vcpu->arch.vcore->dpdes)
> +			atomic_dec(&vcpu->arch.doorbell_request);
> +	}

[Severity: High]
Is there a race condition here that drops newly arrived doorbells?

If the L2 guest is running and a concurrent vCPU sends a doorbell via
MSGSNDP, doorbell_request will be incremented. When the L2 guest exits,
this post-exit logic might observe doorbell_request > 0 and 
vcore->dpdes == 0, and erroneously decrement doorbell_request.

Does this permanently drop the doorbell that arrived concurrently while the
L2 guest was running?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812034628.1122115-1-vaibhav@linux.ibm.com?part=1