Re: [PATCH V2 1/5] x86/sgx: Disconnect backing page references from dirty status

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Dave,

On 5/10/2022 1:12 PM, Dave Hansen wrote:
> On 5/9/22 14:47, Reinette Chatre wrote:
>> --- a/arch/x86/kernel/cpu/sgx/main.c
>> +++ b/arch/x86/kernel/cpu/sgx/main.c
>> @@ -190,6 +190,8 @@ static int __sgx_encl_ewb(struct sgx_epc_page *epc_page, void *va_slot,
>>  	pginfo.metadata = (unsigned long)kmap_atomic(backing->pcmd) +
>>  			  backing->pcmd_offset;
>>  
>> +	set_page_dirty(backing->pcmd);
>> +	set_page_dirty(backing->contents);
>>  	ret = __ewb(&pginfo, sgx_get_epc_virt_addr(epc_page), va_slot);
> 
> I think I may have led you astray on this.  It's typical to do:
> 
> 	lock_page(page)
> 	set_page_dirty(backing->contents);
> 	// modify page
> 	unlock_page(page)
> 
> That's safe because laundering the page requires locking it.  But, the
> page lock isn't held in this case.  So, it's possible to do:
> 
> 	get_page(page) // page can be truncated, but not reclaimed
> 	set_page_dirty(page)
> 	// race: something launders page here
> 	__ewb(page)
> 	put_page(page)
> 	// page is reclaimed, __ewb() contents were thrown away
> 
> __shmem_rw() has a similar pattern to what __sgx_encl_ewb() uses.  It
> doesn't hold a lock_page(), but does dirty the page *after* its memset()
> writes to the page.
> 
> In other words, I think the page dirtying order in the previous (before
> this patch) SGX code was correct.  The concept of this patch is correct,
> but let's just change the dirtying order, please.
> 
> Could you also please add a cc: stable@ and a Link: to either this
> message or the original message where I suggested this, like this
> (temporary) commit has:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/daveh/devel.git/commit/?h=testme&id=5ee32d5d3f3cdb943b01992d2ffc5093b139d023

Will do. Thank you very much.

Reinette