[Question] Dead code in KVM PIT ioctl error handling?

[Question] Dead code in KVM PIT ioctl error handling?

[Ingyu Jang]

Hi,

I noticed that in arch/x86/kvm/x86.c, the functions
kvm_vm_ioctl_get_pit() and kvm_vm_ioctl_get_pit2() always return 0,
making their error checks unreachable.

Both functions (at lines 6408 and 6433) simply perform:
  1. Lock mutex
  2. Copy PIT state
  3. Unlock mutex
  4. return 0;

There are no error paths in either function.

However, their call sites check the return values:

1. At line 7164:
   r = kvm_vm_ioctl_get_pit(kvm, &u.ps);
   if (r)
       goto out;

2. At line 7190:
   r = kvm_vm_ioctl_get_pit2(kvm, &u.ps2);
   if (r)
       goto out;

Since both functions always return 0, these error checks appear to be
dead code.

Is this intentional defensive coding for potential future changes,
or could this be cleaned up?

Thanks,
Ingyu Jang

Re: [Question] Dead code in KVM PIT ioctl error handling?

[Sean Christopherson]

On Sun, Feb 01, 2026, Ingyu Jang wrote:
> Hi,
> 
> I noticed that in arch/x86/kvm/x86.c, the functions
> kvm_vm_ioctl_get_pit() and kvm_vm_ioctl_get_pit2() always return 0,
> making their error checks unreachable.
> 
> Both functions (at lines 6408 and 6433) simply perform:
>   1. Lock mutex
>   2. Copy PIT state
>   3. Unlock mutex
>   4. return 0;
> 
> There are no error paths in either function.
> 
> However, their call sites check the return values:
> 
> 1. At line 7164:
>    r = kvm_vm_ioctl_get_pit(kvm, &u.ps);
>    if (r)
>        goto out;
> 
> 2. At line 7190:
>    r = kvm_vm_ioctl_get_pit2(kvm, &u.ps2);
>    if (r)
>        goto out;
> 
> Since both functions always return 0, these error checks appear to be
> dead code.
> 
> Is this intentional defensive coding for potential future changes,
> or could this be cleaned up?

Hmm, the answer is probably somewhere in between :-)

My vote would be to push the !pit checks down into the helpers, to eliminate the
dead code without creating the potential for future bugs, e.g.

---
 arch/x86/kvm/i8254.c | 18 +++++++++++++++++-
 arch/x86/kvm/x86.c   | 14 --------------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 850972deac8e..fd3049c675a4 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -651,8 +651,12 @@ static void pit_mask_notifier(struct kvm_irq_mask_notifier *kimn, bool mask)
 
 int kvm_vm_ioctl_get_pit(struct kvm *kvm, struct kvm_pit_state *ps)
 {
-	struct kvm_kpit_state *kps = &kvm->arch.vpit->pit_state;
+	struct kvm_kpit_state *kps;
 
+	if (!kvm->arch.vpit)
+		return -ENXIO;
+
+	kps = &kvm->arch.vpit->pit_state;
 	BUILD_BUG_ON(sizeof(*ps) != sizeof(kps->channels));
 
 	mutex_lock(&kps->lock);
@@ -666,6 +670,9 @@ int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
 	int i;
 	struct kvm_pit *pit = kvm->arch.vpit;
 
+	if (!pit)
+		return -ENXIO;
+
 	mutex_lock(&pit->pit_state.lock);
 	memcpy(&pit->pit_state.channels, ps, sizeof(*ps));
 	for (i = 0; i < 3; i++)
@@ -676,6 +683,9 @@ int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
 
 int kvm_vm_ioctl_get_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
 {
+	if (!kvm->arch.vpit)
+		return -ENXIO;
+
 	mutex_lock(&kvm->arch.vpit->pit_state.lock);
 	memcpy(ps->channels, &kvm->arch.vpit->pit_state.channels,
 		sizeof(ps->channels));
@@ -692,6 +702,9 @@ int kvm_vm_ioctl_set_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
 	u32 prev_legacy, cur_legacy;
 	struct kvm_pit *pit = kvm->arch.vpit;
 
+	if (!pit)
+		return -ENXIO;
+
 	mutex_lock(&pit->pit_state.lock);
 	prev_legacy = pit->pit_state.flags & KVM_PIT_FLAGS_HPET_LEGACY;
 	cur_legacy = ps->flags & KVM_PIT_FLAGS_HPET_LEGACY;
@@ -711,6 +724,9 @@ int kvm_vm_ioctl_reinject(struct kvm *kvm, struct kvm_reinject_control *control)
 {
 	struct kvm_pit *pit = kvm->arch.vpit;
 
+	if (!pit)
+		return -ENXIO;
+
 	/* pit->pit_state.lock was overloaded to prevent userspace from getting
 	 * an inconsistent state after running multiple KVM_REINJECT_CONTROL
 	 * ioctls in parallel.  Use a separate lock if that ioctl isn't rare.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index db3f393192d9..b0112c515584 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7406,9 +7406,6 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		r = -EFAULT;
 		if (copy_from_user(&u.ps, argp, sizeof(struct kvm_pit_state)))
 			goto out;
-		r = -ENXIO;
-		if (!kvm->arch.vpit)
-			goto out;
 		r = kvm_vm_ioctl_get_pit(kvm, &u.ps);
 		if (r)
 			goto out;
@@ -7423,18 +7420,11 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		if (copy_from_user(&u.ps, argp, sizeof(u.ps)))
 			goto out;
 		mutex_lock(&kvm->lock);
-		r = -ENXIO;
-		if (!kvm->arch.vpit)
-			goto set_pit_out;
 		r = kvm_vm_ioctl_set_pit(kvm, &u.ps);
-set_pit_out:
 		mutex_unlock(&kvm->lock);
 		break;
 	}
 	case KVM_GET_PIT2: {
-		r = -ENXIO;
-		if (!kvm->arch.vpit)
-			goto out;
 		r = kvm_vm_ioctl_get_pit2(kvm, &u.ps2);
 		if (r)
 			goto out;
@@ -7449,11 +7439,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		if (copy_from_user(&u.ps2, argp, sizeof(u.ps2)))
 			goto out;
 		mutex_lock(&kvm->lock);
-		r = -ENXIO;
-		if (!kvm->arch.vpit)
-			goto set_pit2_out;
 		r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2);
-set_pit2_out:
 		mutex_unlock(&kvm->lock);
 		break;
 	}

base-commit: bca955f212e0583a15f7e9f681032cda36500eff
--