linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
@ 2020-04-06 20:56 Jarkko Sakkinen
  2020-04-14  4:32 ` Sean Christopherson
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2020-04-06 20:56 UTC (permalink / raw)
  To: linux-sgx; +Cc: Sean Christopherson, Haitao Huang, Jarkko Sakkinen

From: Sean Christopherson <sean.j.christopherson@intel.com>

Drop the synchronize_srcu() from sgx_encl_mm_add() and replace it with a
mm_list versioning concept to avoid deadlock when adding a mm during
dup_mmap()/fork(), and to ensure copied PTEs are zapped.

When dup_mmap() runs, it holds mmap_sem for write in both the old mm and
new mm.  Invoking synchronize_srcu() while holding mmap_sem of a mm that
is already attached to the enclave will deadlock if the reclaimer is in
the process of walking mm_list, as the reclaimer will try to acquire
mmap_sem (of the old mm) while holding encl->srcu for read.

 INFO: task ksgxswapd:181 blocked for more than 120 seconds.
 ksgxswapd       D    0   181      2 0x80004000
 Call Trace:
  __schedule+0x2db/0x700
  schedule+0x44/0xb0
  rwsem_down_read_slowpath+0x370/0x470
  down_read+0x95/0xa0
  sgx_reclaim_pages+0x1d2/0x7d0
  ksgxswapd+0x151/0x2e0
  kthread+0x120/0x140
  ret_from_fork+0x35/0x40

 INFO: task fork_consistenc:18824 blocked for more than 120 seconds.
 fork_consistenc D    0 18824  18786 0x00004320
 Call Trace:
  __schedule+0x2db/0x700
  schedule+0x44/0xb0
  schedule_timeout+0x205/0x300
  wait_for_completion+0xb7/0x140
  __synchronize_srcu.part.22+0x81/0xb0
  synchronize_srcu_expedited+0x27/0x30
  synchronize_srcu+0x57/0xe0
  sgx_encl_mm_add+0x12b/0x160
  sgx_vma_open+0x22/0x40
  dup_mm+0x521/0x580
  copy_process+0x1a56/0x1b50
  _do_fork+0x85/0x3a0
  __x64_sys_clone+0x8e/0xb0
  do_syscall_64+0x57/0x1b0
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Furthermore, doing synchronize_srcu() in sgx_encl_mm_add() does not
prevent the new mm from having stale PTEs pointing at the EPC page to be
reclaimed.  dup_mmap() calls vm_ops->open()/sgx_encl_mm_add() _after_
PTEs are copied to the new mm, i.e. blocking fork() until reclaim zaps
the old mm is pointless as the stale PTEs have already been created in
the new mm.

All other flows that walk mm_list can safely race with dup_mmap() or are
protected by a different mechanism.  Add comments to all srcu readers
that don't check the list version to document why its ok for the flow to
ignore the version.

Note, synchronize_srcu() is still needed when removing a mm from an
enclave, as the srcu readers must complete their walk before the mm can
be freed.  Removing a mm is never done while holding mmap_sem.

Cc: Haitao Huang <haitao.huang@linux.intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
v4:
* Reverted to v2.
* Added smp_wmb() with accompanying comment about reordering.
v3:
* Sanitized version list version handling in sgx_reclaimer_block().
  With the fences it was quite complicted given that the version
  was read both in the beginning and end of the loop.
* Removed comment before cpumask_clear() because technically it is
  not part of this bug fix.
v2:
* Remove smp_wmb() as x86 does not reorder writes in the pipeline.
* Refine comments to be more to the point and more maintainable when
  things might change.
* Replace the ad hoc (goto-based) loop construct with a proper loop
  construct.
 arch/x86/kernel/cpu/sgx/encl.c    | 17 +++++++++++--
 arch/x86/kernel/cpu/sgx/encl.h    |  1 +
 arch/x86/kernel/cpu/sgx/reclaim.c | 40 +++++++++++++++++++++----------
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index e0124a2f22d5..1646c3d1839c 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -196,6 +196,9 @@ int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
 	struct sgx_encl_mm *encl_mm;
 	int ret;
 
+	/* mm_list can be accessed only by a single thread at a time. */
+	lockdep_assert_held_write(&mm->mmap_sem);
+
 	if (atomic_read(&encl->flags) & SGX_ENCL_DEAD)
 		return -EINVAL;
 
@@ -221,11 +224,21 @@ int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
 		return ret;
 	}
 
+	/*
+	 * The page reclaimer uses list version for synchronization instead of
+	 * synchronize_scru() because otherwise we could conflict with
+	 * dup_mmap().
+	 */
+
 	spin_lock(&encl->mm_lock);
+
 	list_add_rcu(&encl_mm->list, &encl->mm_list);
-	spin_unlock(&encl->mm_lock);
 
-	synchronize_srcu(&encl->srcu);
+	/* Even if the CPU does not reorder writes, a compiler might. */
+	smp_wmb();
+	encl->mm_list_version++;
+
+	spin_unlock(&encl->mm_lock);
 
 	return 0;
 }
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 44b353aa8866..f0f72e591244 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -74,6 +74,7 @@ struct sgx_encl {
 	struct mutex lock;
 	struct list_head mm_list;
 	spinlock_t mm_lock;
+	unsigned long mm_list_version;
 	struct file *backing;
 	struct kref refcount;
 	struct srcu_struct srcu;
diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index 39f0ddefbb79..5e089f0db201 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -184,28 +184,39 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
 	struct sgx_encl_page *page = epc_page->owner;
 	unsigned long addr = SGX_ENCL_PAGE_ADDR(page);
 	struct sgx_encl *encl = page->encl;
+	unsigned long mm_list_version;
 	struct sgx_encl_mm *encl_mm;
 	struct vm_area_struct *vma;
 	int idx, ret;
 
-	idx = srcu_read_lock(&encl->srcu);
+	do {
+		mm_list_version = encl->mm_list_version;
 
-	list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
-		if (!mmget_not_zero(encl_mm->mm))
-			continue;
+		/*
+		 * Fence the read. This guarantees that we don't mutate the old
+		 * list with a new version.
+		 */
+		smp_rmb();
 
-		down_read(&encl_mm->mm->mmap_sem);
+		idx = srcu_read_lock(&encl->srcu);
 
-		ret = sgx_encl_find(encl_mm->mm, addr, &vma);
-		if (!ret && encl == vma->vm_private_data)
-			zap_vma_ptes(vma, addr, PAGE_SIZE);
+		list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
+			if (!mmget_not_zero(encl_mm->mm))
+				continue;
 
-		up_read(&encl_mm->mm->mmap_sem);
+			down_read(&encl_mm->mm->mmap_sem);
 
-		mmput_async(encl_mm->mm);
-	}
+			ret = sgx_encl_find(encl_mm->mm, addr, &vma);
+			if (!ret && encl == vma->vm_private_data)
+				zap_vma_ptes(vma, addr, PAGE_SIZE);
 
-	srcu_read_unlock(&encl->srcu, idx);
+			up_read(&encl_mm->mm->mmap_sem);
+
+			mmput_async(encl_mm->mm);
+		}
+
+		srcu_read_unlock(&encl->srcu, idx);
+	} while (unlikely(encl->mm_list_version != mm_list_version));
 
 	mutex_lock(&encl->lock);
 
@@ -250,6 +261,11 @@ static const cpumask_t *sgx_encl_ewb_cpumask(struct sgx_encl *encl)
 	struct sgx_encl_mm *encl_mm;
 	int idx;
 
+	/*
+	 * Can race with sgx_encl_mm_add(), but ETRACK has already been
+	 * executed, which means that the CPUs running in the new mm will enter
+	 * into the enclave with a fresh epoch.
+	 */
 	cpumask_clear(cpumask);
 
 	idx = srcu_read_lock(&encl->srcu);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-06 20:56 [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim Jarkko Sakkinen
@ 2020-04-14  4:32 ` Sean Christopherson
  2020-04-14  7:17   ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2020-04-14  4:32 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Haitao Huang

On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
  	spin_lock(&encl->mm_lock);
> +
>  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> -	spin_unlock(&encl->mm_lock);
>  
> -	synchronize_srcu(&encl->srcu);
> +	/* Even if the CPU does not reorder writes, a compiler might. */

The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
comments is to explicitly call out the associated reader/writer.  If you
want to go with a minimal comment, my vote is for something like:

	/*
	 * Add to list before updating version.  Pairs the with smp_rmb() in
	 * sgx_reclaimer_block().
	 */

And if you want to go really spartan, I'd take:

	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */

over a generic comment about the compiler reordering instructions.

> +	smp_wmb();
> +	encl->mm_list_version++;
> +
> +	spin_unlock(&encl->mm_lock);
>  
>  	return 0;
>  }
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index 44b353aa8866..f0f72e591244 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -74,6 +74,7 @@ struct sgx_encl {
>  	struct mutex lock;
>  	struct list_head mm_list;
>  	spinlock_t mm_lock;
> +	unsigned long mm_list_version;
>  	struct file *backing;
>  	struct kref refcount;
>  	struct srcu_struct srcu;
> diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> index 39f0ddefbb79..5e089f0db201 100644
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -184,28 +184,39 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
>  	struct sgx_encl_page *page = epc_page->owner;
>  	unsigned long addr = SGX_ENCL_PAGE_ADDR(page);
>  	struct sgx_encl *encl = page->encl;
> +	unsigned long mm_list_version;
>  	struct sgx_encl_mm *encl_mm;
>  	struct vm_area_struct *vma;
>  	int idx, ret;
>  
> -	idx = srcu_read_lock(&encl->srcu);
> +	do {
> +		mm_list_version = encl->mm_list_version;
>  
> -	list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
> -		if (!mmget_not_zero(encl_mm->mm))
> -			continue;
> +		/*
> +		 * Fence the read. This guarantees that we don't mutate the old
> +		 * list with a new version.
> +		 */

As above, would prefer something like:

		/*
		 * Read the version before walking the list.  Pairs with the
		 * smp_wmb() in sgx_encl_mm_add().
		 */

or just

		/* Pairs with the smp_wmb() in sgx_encl_mm_add(). */
	

> +		smp_rmb();
>  
> -		down_read(&encl_mm->mm->mmap_sem);
> +		idx = srcu_read_lock(&encl->srcu);
>  
> -		ret = sgx_encl_find(encl_mm->mm, addr, &vma);
> -		if (!ret && encl == vma->vm_private_data)
> -			zap_vma_ptes(vma, addr, PAGE_SIZE);
> +		list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) {
> +			if (!mmget_not_zero(encl_mm->mm))
> +				continue;
>  
> -		up_read(&encl_mm->mm->mmap_sem);
> +			down_read(&encl_mm->mm->mmap_sem);
>  
> -		mmput_async(encl_mm->mm);
> -	}
> +			ret = sgx_encl_find(encl_mm->mm, addr, &vma);
> +			if (!ret && encl == vma->vm_private_data)
> +				zap_vma_ptes(vma, addr, PAGE_SIZE);
>  
> -	srcu_read_unlock(&encl->srcu, idx);
> +			up_read(&encl_mm->mm->mmap_sem);
> +
> +			mmput_async(encl_mm->mm);
> +		}
> +
> +		srcu_read_unlock(&encl->srcu, idx);
> +	} while (unlikely(encl->mm_list_version != mm_list_version));
>  
>  	mutex_lock(&encl->lock);
>  
> @@ -250,6 +261,11 @@ static const cpumask_t *sgx_encl_ewb_cpumask(struct sgx_encl *encl)
>  	struct sgx_encl_mm *encl_mm;
>  	int idx;
>  
> +	/*
> +	 * Can race with sgx_encl_mm_add(), but ETRACK has already been
> +	 * executed, which means that the CPUs running in the new mm will enter
> +	 * into the enclave with a fresh epoch.
> +	 */
>  	cpumask_clear(cpumask);
>  
>  	idx = srcu_read_lock(&encl->srcu);
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-14  4:32 ` Sean Christopherson
@ 2020-04-14  7:17   ` Jarkko Sakkinen
  2020-04-14 18:45     ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2020-04-14  7:17 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Haitao Huang

On Mon, Apr 13, 2020 at 09:32:34PM -0700, Sean Christopherson wrote:
> On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
>   	spin_lock(&encl->mm_lock);
> > +
> >  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> > -	spin_unlock(&encl->mm_lock);
> >  
> > -	synchronize_srcu(&encl->srcu);
> > +	/* Even if the CPU does not reorder writes, a compiler might. */
> 
> The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
> comments is to explicitly call out the associated reader/writer.  If you
> want to go with a minimal comment, my vote is for something like:
> 
> 	/*
> 	 * Add to list before updating version.  Pairs the with smp_rmb() in
> 	 * sgx_reclaimer_block().
> 	 */
> 
> And if you want to go really spartan, I'd take:
> 
> 	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */
> 
> over a generic comment about the compiler reordering instructions.

Thaks Sean, makes sense, I'll go with your "spartan" suggestion.

/Jarkko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-14  7:17   ` Jarkko Sakkinen
@ 2020-04-14 18:45     ` Jarkko Sakkinen
  2020-04-16  4:28       ` Sean Christopherson
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2020-04-14 18:45 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Haitao Huang

On Tue, Apr 14, 2020 at 10:17:49AM +0300, Jarkko Sakkinen wrote:
> On Mon, Apr 13, 2020 at 09:32:34PM -0700, Sean Christopherson wrote:
> > On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> >   	spin_lock(&encl->mm_lock);
> > > +
> > >  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> > > -	spin_unlock(&encl->mm_lock);
> > >  
> > > -	synchronize_srcu(&encl->srcu);
> > > +	/* Even if the CPU does not reorder writes, a compiler might. */
> > 
> > The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
> > comments is to explicitly call out the associated reader/writer.  If you
> > want to go with a minimal comment, my vote is for something like:
> > 
> > 	/*
> > 	 * Add to list before updating version.  Pairs the with smp_rmb() in
> > 	 * sgx_reclaimer_block().
> > 	 */
> > 
> > And if you want to go really spartan, I'd take:
> > 
> > 	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */
> > 
> > over a generic comment about the compiler reordering instructions.
> 
> Thaks Sean, makes sense, I'll go with your "spartan" suggestion.

Updated, ready to squash?

/Jarkko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-14 18:45     ` Jarkko Sakkinen
@ 2020-04-16  4:28       ` Sean Christopherson
  2020-04-16 17:24         ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2020-04-16  4:28 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Haitao Huang

On Tue, Apr 14, 2020 at 09:45:28PM +0300, Jarkko Sakkinen wrote:
> On Tue, Apr 14, 2020 at 10:17:49AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Apr 13, 2020 at 09:32:34PM -0700, Sean Christopherson wrote:
> > > On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > >   	spin_lock(&encl->mm_lock);
> > > > +
> > > >  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> > > > -	spin_unlock(&encl->mm_lock);
> > > >  
> > > > -	synchronize_srcu(&encl->srcu);
> > > > +	/* Even if the CPU does not reorder writes, a compiler might. */
> > > 
> > > The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
> > > comments is to explicitly call out the associated reader/writer.  If you
> > > want to go with a minimal comment, my vote is for something like:
> > > 
> > > 	/*
> > > 	 * Add to list before updating version.  Pairs the with smp_rmb() in
> > > 	 * sgx_reclaimer_block().
> > > 	 */
> > > 
> > > And if you want to go really spartan, I'd take:
> > > 
> > > 	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */
> > > 
> > > over a generic comment about the compiler reordering instructions.
> > 
> > Thaks Sean, makes sense, I'll go with your "spartan" suggestion.
> 
> Updated, ready to squash?

Any objection to using the spartan comment for the smb_rmb() in
sgx_reclaimer_block() as well?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-16  4:28       ` Sean Christopherson
@ 2020-04-16 17:24         ` Jarkko Sakkinen
  2020-04-16 17:39           ` Jarkko Sakkinen
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2020-04-16 17:24 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Haitao Huang

On Wed, Apr 15, 2020 at 09:28:10PM -0700, Sean Christopherson wrote:
> On Tue, Apr 14, 2020 at 09:45:28PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Apr 14, 2020 at 10:17:49AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Apr 13, 2020 at 09:32:34PM -0700, Sean Christopherson wrote:
> > > > On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> > > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > >   	spin_lock(&encl->mm_lock);
> > > > > +
> > > > >  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> > > > > -	spin_unlock(&encl->mm_lock);
> > > > >  
> > > > > -	synchronize_srcu(&encl->srcu);
> > > > > +	/* Even if the CPU does not reorder writes, a compiler might. */
> > > > 
> > > > The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
> > > > comments is to explicitly call out the associated reader/writer.  If you
> > > > want to go with a minimal comment, my vote is for something like:
> > > > 
> > > > 	/*
> > > > 	 * Add to list before updating version.  Pairs the with smp_rmb() in
> > > > 	 * sgx_reclaimer_block().
> > > > 	 */
> > > > 
> > > > And if you want to go really spartan, I'd take:
> > > > 
> > > > 	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */
> > > > 
> > > > over a generic comment about the compiler reordering instructions.
> > > 
> > > Thaks Sean, makes sense, I'll go with your "spartan" suggestion.
> > 
> > Updated, ready to squash?
> 
> Any objection to using the spartan comment for the smb_rmb() in
> sgx_reclaimer_block() as well?

For sure. I think here the role of the comment is to help with
the navigation.

/Jarkko
the navigation.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim
  2020-04-16 17:24         ` Jarkko Sakkinen
@ 2020-04-16 17:39           ` Jarkko Sakkinen
  0 siblings, 0 replies; 7+ messages in thread
From: Jarkko Sakkinen @ 2020-04-16 17:39 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Haitao Huang

On Thu, Apr 16, 2020 at 08:25:01PM +0300, Jarkko Sakkinen wrote:
> On Wed, Apr 15, 2020 at 09:28:10PM -0700, Sean Christopherson wrote:
> > On Tue, Apr 14, 2020 at 09:45:28PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Apr 14, 2020 at 10:17:49AM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Apr 13, 2020 at 09:32:34PM -0700, Sean Christopherson wrote:
> > > > > On Mon, Apr 06, 2020 at 11:56:26PM +0300, Jarkko Sakkinen wrote:
> > > > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > >   	spin_lock(&encl->mm_lock);
> > > > > > +
> > > > > >  	list_add_rcu(&encl_mm->list, &encl->mm_list);
> > > > > > -	spin_unlock(&encl->mm_lock);
> > > > > >  
> > > > > > -	synchronize_srcu(&encl->srcu);
> > > > > > +	/* Even if the CPU does not reorder writes, a compiler might. */
> > > > > 
> > > > > The preferred (by maintainers) style of comment for smp_wmb()/smp_rmb()
> > > > > comments is to explicitly call out the associated reader/writer.  If you
> > > > > want to go with a minimal comment, my vote is for something like:
> > > > > 
> > > > > 	/*
> > > > > 	 * Add to list before updating version.  Pairs the with smp_rmb() in
> > > > > 	 * sgx_reclaimer_block().
> > > > > 	 */
> > > > > 
> > > > > And if you want to go really spartan, I'd take:
> > > > > 
> > > > > 	/* Pairs with smp_rmb() in sgx_reclaimer_block(). */
> > > > > 
> > > > > over a generic comment about the compiler reordering instructions.
> > > > 
> > > > Thaks Sean, makes sense, I'll go with your "spartan" suggestion.
> > > 
> > > Updated, ready to squash?
> > 
> > Any objection to using the spartan comment for the smb_rmb() in
> > sgx_reclaimer_block() as well?
> 
> For sure. I think here the role of the comment is to help with
> the navigation.
> 
> /Jarkko

Finally squashed.

/Jarkko

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-16 17:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-06 20:56 [PATCH v4] x86/sgx: Fix deadlock and race conditions between fork() and EPC reclaim Jarkko Sakkinen
2020-04-14  4:32 ` Sean Christopherson
2020-04-14  7:17   ` Jarkko Sakkinen
2020-04-14 18:45     ` Jarkko Sakkinen
2020-04-16  4:28       ` Sean Christopherson
2020-04-16 17:24         ` Jarkko Sakkinen
2020-04-16 17:39           ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).