diff mbox series

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

Message ID 4cda80a0049de6bc47480b4ad119091027c11d45.1652131695.git.reinette.chatre@intel.com (mailing list archive)
State New, archived
Headers show
Series [V2,1/5] x86/sgx: Disconnect backing page references from dirty status | expand

Commit Message

Reinette Chatre May 9, 2022, 9:47 p.m. UTC 
sgx_encl_put_backing() is used to release references to the
backing storage and, optionally, mark both backing store pages
as dirty.

Managing references and dirty status together has two consequences:
1) Both backing store pages are marked as dirty, even if only one of
   the backing store pages are changed.
2) Backing store pages are marked dirty after the important data
   has been written to it. This is against the custom of marking
   pages as dirty before important data are written to them.

Remove the option to sgx_encl_put_backing() to set the backing
pages as dirty and set the needed pages as dirty right before
receiving important data. This aligns the usage with the
existing custom and prepares the code for a following change
where only one of the backing pages need to be marked as dirty.

Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/sgx/encl.c | 10 ++--------
 arch/x86/kernel/cpu/sgx/encl.h |  2 +-
 arch/x86/kernel/cpu/sgx/main.c |  6 ++++--
 3 files changed, 7 insertions(+), 11 deletions(-)

Comments

Dave Hansen May 10, 2022, 8:12 p.m. UTC | #1 
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
Reinette Chatre May 10, 2022, 11 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7c63a1911fae..398695a20605 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -94,7 +94,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);
 
 	sgx_encl_truncate_backing_page(encl, page_index);
 
@@ -645,15 +645,9 @@  int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
 /**
  * sgx_encl_put_backing() - Unpin the backing storage
  * @backing:	data for accessing backing storage for the page
- * @do_write:	mark pages dirty
  */
-void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write)
+void sgx_encl_put_backing(struct sgx_backing *backing)
 {
-	if (do_write) {
-		set_page_dirty(backing->pcmd);
-		set_page_dirty(backing->contents);
-	}
-
 	put_page(backing->pcmd);
 	put_page(backing->contents);
 }
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index fec43ca65065..d44e7372151f 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -107,7 +107,7 @@  void sgx_encl_release(struct kref *ref);
 int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
 int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index,
 			 struct sgx_backing *backing);
-void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write);
+void sgx_encl_put_backing(struct sgx_backing *backing);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
 				  struct sgx_encl_page *page);
 
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8e4bc6453d26..fad3d6c4756e 100644
--- 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);
 
 	kunmap_atomic((void *)(unsigned long)(pginfo.metadata -
@@ -320,7 +322,7 @@  static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
 		sgx_encl_free_epc_page(encl->secs.epc_page);
 		encl->secs.epc_page = NULL;
 
-		sgx_encl_put_backing(&secs_backing, true);
+		sgx_encl_put_backing(&secs_backing);
 	}
 
 out:
@@ -411,7 +413,7 @@  static void sgx_reclaim_pages(void)
 
 		encl_page = epc_page->owner;
 		sgx_reclaimer_write(epc_page, &backing[i]);
-		sgx_encl_put_backing(&backing[i], true);
+		sgx_encl_put_backing(&backing[i]);
 
 		kref_put(&encl_page->encl->refcount, sgx_encl_release);
 		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;