Re: [RFC PATCH 2/4] x86/sgx: Set dirty bit after modifying page contents

[Jarkko Sakkinen <jarkko@xxxxxxxxxx>]

On Thu, 2022-04-28 at 13:11 -0700, Reinette Chatre wrote:
> Recent commit 08999b2489b4 ("x86/sgx: Free backing memory
> after faulting the enclave page") expanded __sgx_encl_eldu()
> to clear an enclave page's PCMD (Paging Crypto MetaData)
> from the PCMD page in the backing store after the enclave
> page is restored to the enclave.
> 
> Since the PCMD page in the backing store is modified the page
> should be set as dirty when releasing the reference to
> ensure the modified data is retained.
> 
> Fixes: 08999b2489b4 ("x86/sgx: Free backing memory after faulting the enclave page")
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index e5d2661800ac..e03f124ce772 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -98,7 +98,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
>         kunmap_atomic(pcmd_page);
>         kunmap_atomic((void *)(unsigned long)pginfo.contents);
>  
> -       sgx_encl_put_backing(&b, false);
> +       sgx_encl_put_backing(&b, true);
>  
>         sgx_encl_truncate_backing_page(encl, page_index);
>  

So, the implementation is:

void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write)
{
	if (do_write) {
		set_page_dirty(backing->pcmd);
		set_page_dirty(backing->contents);
	}

	put_page(backing->pcmd);
	put_page(backing->contents);
}

And we only want to set dirty for PCMD part, right?

Thus, perhaps we should fix it instead by:

	set_page_dirty(backing->pcmd);
	put_page(backing->pcmd):
	put_page(backing->contents);
	
?	

I would not mind getting rid of that function anyway. It's kind
of bad wrapping IMHO.
	
BR, Jarkko