Re: [PATCH v3] x86/sgx: Fix sgx_encl_may_map locking

From: Dave Hansen <dave.hansen@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-sgx@vger.kernel.org
Cc: Haitao Huang <haitao.huang@linux.intel.com>,
	Matthew Wilcox <willy@infradead.org>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v3] x86/sgx: Fix sgx_encl_may_map locking
Date: Mon, 5 Oct 2020 07:28:38 -0700	[thread overview]
Message-ID: <9f1b6d7e-1990-925b-c124-adfef7f2ddfc@intel.com> (raw)
In-Reply-To: <20201005141119.5395-1-jarkko.sakkinen@linux.intel.com>

On 10/5/20 7:11 AM, Jarkko Sakkinen wrote:
> +	unsigned long count = 0;
...
> +	xas_lock(&xas);
> +	xas_for_each(&xas, page, idx_end) {
> +		if (++count % XA_CHECK_SCHED)
> +			continue;

Let's slow down and think through the loop, please.

First time through the loop, count=0, XA_CHECK_SCHED=4096, it will do a
++count, and now count=1.  (count % XA_CHECK_SCHED) == 1.  It will
continue.  It skips the page->vm_max_prot_bits checks.

Next time through the loop, count=1, XA_CHECK_SCHED=4096, it will do a
++count, and now count=2.  (count % XA_CHECK_SCHED) == 2.  It will
continue.  It skips the page->vm_max_prot_bits checks.

...

It will do this until it hits count=4095 where it will actually fall
into the rest of the loop, doing the page->vm_max_prot_bits checks.

So, in the end the loop only does what it's supposed to be doing 1/4096
times.  Not great.  Don't we have tests that will notice breakage like this?

The XA_CHECK_SCHED needs to be stuck near the *end* of the loop, just
before the lock dropping and resched stuff.