Re: [PATCH v2 03/31] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects

[Nikolay Borisov]

On 27.03.26 г. 18:01 ч., Xu Yilun wrote:
<snip>

> +/**
> + * tdx_page_array_ctrl_leak() - Leak data pages and free the container
> + * @array: The tdx_page_array to be leaked.
> + *
> + * Call this function when failed to reclaim the control pages. Free the root
> + * page and the holding structures, but orphan the data pages, to prevent the
> + * host from re-allocating and accessing memory that the hardware may still
> + * consider private.
> + */
> +void tdx_page_array_ctrl_leak(struct tdx_page_array *array)
> +{
> +	if (!array)
> +		return;
> +
> +	kfree(array->pages);
> +	kfree(array->root);
> +	kfree(array);
> +}
> +EXPORT_SYMBOL_GPL(tdx_page_array_ctrl_leak);

This instantly raises a red flag if by design an API has the ability to 
simply leak memory. Under what conditions this might be required, can't 
we do something to gracefully handle the case when pages cannot be freed 
instantly, i.e queued freeing or some such ? Simply leaking them is a 
big NO.

> +
> +static bool tdx_page_array_validate_release(struct tdx_page_array *array,
> +					    unsigned int offset,

This function is only ever called with offset of 0, if it's intended to 
be used later then I'd rather see this argument added in an explicit 
patch with rationale why it's needed.

> +					    unsigned int nr_released,
> +					    u64 released_hpa)
> +{
> +	unsigned int nents;
> +
> +	if (offset >= array->nr_pages)
> +		return false;
> +
> +	nents = umin(array->nr_pages - offset, TDX_PAGE_ARRAY_MAX_NENTS);
> +
> +	if (nents != nr_released) {
> +		pr_err("%s nr_released [%d] doesn't match page array nents [%d]\n",
> +		       __func__, nr_released, nents);
> +		return false;
> +	}
> +
> +	/*
> +	 * Unfortunately TDX has multiple page allocation protocols, check the
> +	 * "singleton" case required for HPA_ARRAY_T.
> +	 */
> +	if (page_to_phys(array->pages[0]) == released_hpa &&
> +	    array->nr_pages == 1)
> +		return true;
> +
> +	/* Then check the "non-singleton" case */
> +	if (virt_to_phys(array->root) == released_hpa) {
> +		u64 *entries = array->root;
> +		int i;
> +
> +		for (i = 0; i < nents; i++) {
> +			struct page *page = array->pages[offset + i];
> +			u64 val = page_to_phys(page);
> +
> +			if (val != entries[i]) {
> +				pr_err("%s entry[%d] [0x%llx] doesn't match page hpa [0x%llx]\n",
> +				       __func__, i, entries[i], val);
> +				return false;
> +			}
> +		}
> +
> +		return true;
> +	}
> +
> +	pr_err("%s failed to validate, released_hpa [0x%llx], root page hpa [0x%llx], page0 hpa [%#llx], number pages %u\n",
> +	       __func__, released_hpa, virt_to_phys(array->root),
> +	       page_to_phys(array->pages[0]), array->nr_pages);
> +
> +	return false;
> +}


> +<snip>