linux-kernel - Re: [PATCH v12 18/22] x86/virt/tdx: Keep TDMRs when module initialization is successful

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d62ebdf971ef52fe38cfb0a5edb7566e9f6d7e14.camel@intel.com>
Date:   Thu, 29 Jun 2023 00:24:12 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "david@...hat.com" <david@...hat.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v12 18/22] x86/virt/tdx: Keep TDMRs when module
 initialization is successful

On Wed, 2023-06-28 at 15:48 +0300, Nikolay Borisov wrote:
> > This diff is extremely hard to follow, but I think the change to error
> > handling Nikolay proposed has to be applied to the function from the
> > beginning, not changed drastically in this patch.
> > 
> 
> 
> I agree. That change should be broken across the various patches 
> introducing each piece of error handling.

No I don't want to do this.  The TDMRs are only needed to be saved if we want to
do the next patch (reset TDX memory).  They are always freed in previous patch.
We can add justification to keep in previous patch but I now want to avoid such
pattern because I now believe it's not the right way to organize patches:

Obviously such justification depends on the later patch.  In case the later
patch has something wrong and needs to be updated, the justification can be
invalid, and we need to adjust the previous patches accordingly.  This could
result in code review frustration.

Specifically for this issue, if we always free TDMRs in previous patches, then 
it's just not right to do what you suggested there.  Also, now with dynamic
allocation of TDSYSINFO_STRUCT and CMR array, we need to do 3 things when module
initialization is successful:

	put_online_mems();
	kfree(sysinfo);
	kfree(cmr_array);
	return 0;
out_xxx:
	....
	put_online_mems();
	kfree(sysinfo);
	kfree(cmr_array);
	return ret;

I can hardly say which is better.  I am willing to do the above pattern if you
guys prefer but I certainly don't want to mix this logic to previous patches.