[PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[Tony Lindgren]

Fix a Smatch static checker warning reported by Dan:

	arch/x86/kvm/vmx/tdx.c:3464 __tdx_bringup()
	warn: missing error code 'r'

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes:  61bb28279623 ("KVM: TDX: Get system-wide info about TDX module on initialization")
Signed-off-by: Tony Lindgren <tony.lindgren@linux.intel.com>
---
 arch/x86/kvm/vmx/tdx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 66744f5768c8..3ce7fe08afd8 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3466,11 +3466,15 @@ static int __init __tdx_bringup(void)
 
 	/* Check TDX module and KVM capabilities */
 	if (!tdx_get_supported_attrs(&tdx_sysinfo->td_conf) ||
-	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf))
+	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf)) {
+		r = -EINVAL;
 		goto get_sysinfo_err;
+	}
 
-	if (!(tdx_sysinfo->features.tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
+	if (!(tdx_sysinfo->features.tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)) {
+		r = -EINVAL;
 		goto get_sysinfo_err;
+	}
 
 	/*
 	 * TDX has its own limit of maximum vCPUs it can support for all
-- 
2.43.0

Re: [PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[Huang, Kai]

On Tue, 2025-09-09 at 13:16 +0300, Tony Lindgren wrote:
> Fix a Smatch static checker warning reported by Dan:
> 
> 	arch/x86/kvm/vmx/tdx.c:3464 __tdx_bringup()
> 	warn: missing error code 'r'
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes:  61bb28279623 ("KVM: TDX: Get system-wide info about TDX module on initialization")
> Signed-off-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> ---
>  arch/x86/kvm/vmx/tdx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 66744f5768c8..3ce7fe08afd8 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -3466,11 +3466,15 @@ static int __init __tdx_bringup(void)
>  
>  	/* Check TDX module and KVM capabilities */
>  	if (!tdx_get_supported_attrs(&tdx_sysinfo->td_conf) ||
> -	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf))
> +	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf)) {
> +		r = -EINVAL;
>  		goto get_sysinfo_err;
> +	}
>  
> -	if (!(tdx_sysinfo->features.tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
> +	if (!(tdx_sysinfo->features.tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)) {
> +		r = -EINVAL;
>  		goto get_sysinfo_err;
> +	}
>  
>  	/*
>  	 * TDX has its own limit of maximum vCPUs it can support for all

Looking at the code, seems all checks after tdx_get_sysinfo() have the
same pattern when the check fails:

	r = -EINVAL;
	goto get_sysinfo_err;

How about we just initialize r to -EINVAL once before tdx_get_sysinfo() so
that all 'r = -EINVAL;' can be removed?  I think in this way the code
would be simpler (see below diff [*])?

The "Fixes" tag would be hard to identify, though, because the diff
touches the code introduced multiple commits.  But I am not sure whether
this is a true issue since AFAICT we can use multiple "Fixes" tags.

[*] the diff:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 7b81cd1fbba5..113cb074b90c 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3460,12 +3460,11 @@ static int __init __tdx_bringup(void)
        if (r)
                goto tdx_bringup_err;
 
+       r = -EINVAL;
        /* Get TDX global information for later use */
        tdx_sysinfo = tdx_get_sysinfo();
-       if (WARN_ON_ONCE(!tdx_sysinfo)) {
-               r = -EINVAL;
+       if (WARN_ON_ONCE(!tdx_sysinfo))
                goto get_sysinfo_err;
-       }
 
        /* Check TDX module and KVM capabilities */
        if (!tdx_get_supported_attrs(&tdx_sysinfo->td_conf) ||
@@ -3508,14 +3507,11 @@ static int __init __tdx_bringup(void)
        if (td_conf->max_vcpus_per_td < num_present_cpus()) {
                pr_err("Disable TDX: MAX_VCPU_PER_TD (%u) smaller than
number of logical CPUs (%u).\n",
                                td_conf->max_vcpus_per_td,
num_present_cpus());
-               r = -EINVAL;
                goto get_sysinfo_err;
        }
 
-       if (misc_cg_set_capacity(MISC_CG_RES_TDX,
tdx_get_nr_guest_keyids())) {
-               r = -EINVAL;
+       if (misc_cg_set_capacity(MISC_CG_RES_TDX,
tdx_get_nr_guest_keyids()))
                goto get_sysinfo_err;
-       }
 
        /*
         * Leave hardware virtualization enabled after TDX is enabled

Re: [PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[Tony Lindgren]

On Tue, Sep 09, 2025 at 10:55:18AM +0000, Huang, Kai wrote:
> How about we just initialize r to -EINVAL once before tdx_get_sysinfo() so
> that all 'r = -EINVAL;' can be removed?  I think in this way the code
> would be simpler (see below diff [*])?
> 
> The "Fixes" tag would be hard to identify, though, because the diff
> touches the code introduced multiple commits.  But I am not sure whether
> this is a true issue since AFAICT we can use multiple "Fixes" tags.

Your diff looks fine to me, however my personal preference would be to do
the fix first then clean-up :) 

Regards,

Tony

Re: [PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[Sean Christopherson]

On Tue, Sep 09, 2025, Tony Lindgren wrote:
> On Tue, Sep 09, 2025 at 10:55:18AM +0000, Huang, Kai wrote:
> > How about we just initialize r to -EINVAL once before tdx_get_sysinfo() so
> > that all 'r = -EINVAL;' can be removed?  I think in this way the code
> > would be simpler (see below diff [*])?
> > 
> > The "Fixes" tag would be hard to identify,

No, Fixes always points at the commit(s) that introduced buggy behavior.  While
one might argue that commit 61bb28279623 was set up to fail by earlier commits,
that commit is unequivocally the one and only Fixes commit.

> > though, because the diff
> > touches the code introduced multiple commits.  But I am not sure whether
> > this is a true issue since AFAICT we can use multiple "Fixes" tags.
> 
> Your diff looks fine to me, however my personal preference would be to do
> the fix first then clean-up :) 

Eh, fixes can also harden against similar failures in the future.  I don't see
any reason to split this one up.  The buggy commit was introduced in v6.16 and
Kai's suggestion applies cleanly there, so the more aggressive fix won't lead to
stable@ conflicts either.

In short, let's go straight to Kai's version.

Re: [PATCH 1/1] KVM: TDX: Fix uninitialized error code for __tdx_bringup()

[Huang, Kai]

On Tue, 2025-09-09 at 06:45 -0700, Sean Christopherson wrote:
> On Tue, Sep 09, 2025, Tony Lindgren wrote:
> > On Tue, Sep 09, 2025 at 10:55:18AM +0000, Huang, Kai wrote:
> > > How about we just initialize r to -EINVAL once before tdx_get_sysinfo() so
> > > that all 'r = -EINVAL;' can be removed?  I think in this way the code
> > > would be simpler (see below diff [*])?
> > > 
> > > The "Fixes" tag would be hard to identify,
> 
> No, Fixes always points at the commit(s) that introduced buggy behavior.  While
> one might argue that commit 61bb28279623 was set up to fail by earlier commits,
> that commit is unequivocally the one and only Fixes commit.

Good to know.  Thanks Sean!