linux-kernel - Re: [PATCH] x86/sgx: Fix free page accounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <460c7c74-0921-03a4-ded0-afa31d9c8aa6@intel.com>
Date:   Thu, 4 Nov 2021 12:04:55 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Reinette Chatre <reinette.chatre@...el.com>
Cc:     dave.hansen@...ux.intel.com, jarkko@...nel.org, tglx@...utronix.de,
        bp@...en8.de, mingo@...hat.com, linux-sgx@...r.kernel.org,
        x86@...nel.org, seanjc@...gle.com, tony.luck@...el.com,
        hpa@...or.com, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] x86/sgx: Fix free page accounting

On 11/4/21 11:54 AM, Greg KH wrote:
>>  static bool sgx_should_reclaim(unsigned long watermark)
>>  {
>> -	return sgx_nr_free_pages < watermark && !list_empty(&sgx_active_page_list);
>> +	return atomic_long_read(&sgx_nr_free_pages) < watermark &&
>> +	       !list_empty(&sgx_active_page_list);
> What prevents the value from changing right after you test this?  Why is
> an atomic value somehow solving the problem?

Nothing.  It's fundamentally racy, and that's OK.

Just like the core VM, being under the watermark is an indication that
the kernel is low on pages (SGX pages in this case).  It means we should
try SGX reclaim.  Let's say there's a race and a bunch of pages are
freed.  Reclaim will run once iteration then stop.

We could make an argument that the sgx_reclaim_pages() loop should check
sgx_nr_free_pages in a few places to ensure it doesn't unnecessarily
reclaim too much memory.  That's something to look at, but it's beyond
the scope of this very simple bug fix.