Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error

Hi Jarkko,

On 9/1/2022 2:53 PM, Jarkko Sakkinen wrote:

On Wed, Aug 31, 2022 at 01:39:53PM -0700, Reinette Chatre wrote:

...

On 8/31/2022 10:38 AM, Jarkko Sakkinen wrote:

...

I think I am missing something here. A lot of logic is added here but I do not see why it is necessary. ksgxd() knows via kthread_should_stop() if the reclaimer was canceled. I am thus wondering, could the above not be simplified to something similar to V1:

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void) static int ksgxd(void *p) {

• unsigned long left_dirty;
• set_freezable();

/* @@ -395,10 +402,10 @@ static int ksgxd(void *p) * required for SECS pages, whose child pages blocked EREMOVE. */ __sgx_sanitize_pages(&sgx_dirty_page_list);

IMHO, would make sense also to have here:

    if (!kthread_should_stop())
            return 0;

Would this not prematurely stop the thread when it should not be?

...

• __sgx_sanitize_pages(&sgx_dirty_page_list);

• /* sanity check: */
• WARN_ON(!list_empty(&sgx_dirty_page_list));

• left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
• if (left_dirty && !kthread_should_stop())

• pr_err("%lu unsanitized pages\n", left_dirty);
  

That would be incorrect, if the function returned because of kthread stopped.

I should have highlighted this but in my example I changed left_dirty to be "unsigned long" with the intention that the "return -ECANCELED" is replaced with "return 0".

__sgx_sanitize_pages() returns 0 when it exits because of kthread stopped.

To elaborate I was thinking about:

+static unsigned long __sgx_sanitize_pages(struct list_head *dirty_page_list) { + unsigned long left_dirty = 0; struct sgx_epc_page *page; LIST_HEAD(dirty); int ret;

- /* dirty_page_list is thread-local, no need for a lock: */ while (!list_empty(dirty_page_list)) { if (kthread_should_stop()) - return; + return 0;

page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);

@@ -92,12 +95,14 @@ static void __sgx_sanitize_pages(struct list_head *dirty_page_list) } else { /* The page is not yet clean - move to the dirty list. */ list_move_tail(&page->list, &dirty); + left_dirty++; }

cond_resched(); }

list_splice(&dirty, dirty_page_list); + return left_dirty; }

and then with what I had in previous email the checks should work:

@@ -388,6 +393,8 @@ void sgx_reclaim_direct(void)

static int ksgxd(void *p) { + unsigned long left_dirty; + set_freezable();

/* @@ -395,10 +402,10 @@ static int ksgxd(void *p) * required for SECS pages, whose child pages blocked EREMOVE. */ __sgx_sanitize_pages(&sgx_dirty_page_list); - __sgx_sanitize_pages(&sgx_dirty_page_list);

- /* sanity check: */ - WARN_ON(!list_empty(&sgx_dirty_page_list)); + left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list); + if (left_dirty && !kthread_should_stop()) + pr_err("%lu unsanitized pages\n", left_dirty);

while (!kthread_should_stop()) { if (try_to_freeze())

If you do the check here you already have a window where kthread could have been stopped anyhow.

So even this would be less correct:

    if (kthreas_should_stop()) {
            return 0;
    }  else if (left_dirty) {
            pr_err("%lu unsanitized pages\n", left_dirty);
    }

So in the end you end as complicated and less correct fix. This all is explained in the commit message.

If you unconditionally print error, you don't have a meaning for the number of unsanitized pags.

Understood that the goal is to only print the number of unsanitized pages if ksgxd has not been stopped prematurely.

Reinette