linux-kernel - Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a49c523c-0c9e-47c6-ae2a-c84ff19f6717@intel.com>
Date: Wed, 25 Jun 2025 13:09:05 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Chao Gao <chao.gao@...el.com>,
 "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: pbonzini@...hat.com, seanjc@...gle.com, dave.hansen@...ux.intel.com,
 rick.p.edgecombe@...el.com, isaku.yamahata@...el.com, kai.huang@...el.com,
 yan.y.zhao@...el.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 kvm@...r.kernel.org, x86@...nel.org, linux-coco@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free_page() helpers

On 6/9/25 19:36, Chao Gao wrote:
>> +static int tdx_alloc_pamt_pages(struct list_head *pamt_pages)
>> +{
>> +	for (int i = 0; i < tdx_nr_pamt_pages(); i++) {
>> +		struct page *page = alloc_page(GFP_KERNEL);
>> +		if (!page)
>> +			goto fail;
> 
> this goto isn't needed. it is used only once. so we can just free the pages and
> return -ENOMEM here.

<shrug>

There's no rule saying that gotos need to be used more than once. It's
idiomatic kernel C to use a goto as an error landing site. In fact, I
*prefer* this because it lets me read the main, non-error-case flow
through the function. Then, at my leisure, I can review the error handling.

This is also, IMNHO, less error-prone to someone adding code and doing a
plain return without freeing the pages.

Third, the goto keeps the indentation down.

So, the suggestion here is well intended, but I think it's flawed in
multiple ways. If you write your code this way (free of one-use gotos),
I won't complain too much. But if you suggest other folks get rid of the
gotos, I'm not super happy.

So, Kirill, do it whatever way you want.

But, Chao, please don't keep suggesting things like this at least in
junk I've got to merge.

>> +	if (tdx_hpa_range_not_free(err))
>> +		return 1;
> 
> I think this needs a comment for the return values 0/1/-EIO above the function.

You and I are in full agreement that this series is gloriously
unencumbered by comments at this point.