From: "Huang, Kai" <kai.huang@intel.com>
To: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
"Chatre, Reinette" <reinette.chatre@intel.com>,
"jarkko@kernel.org" <jarkko@kernel.org>
Cc: "pmenzel@molgen.mpg.de" <pmenzel@molgen.mpg.de>,
"bp@alien8.de" <bp@alien8.de>,
"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
"Dhanraj, Vijay" <vijay.dhanraj@intel.com>,
"x86@kernel.org" <x86@kernel.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"hpa@zytor.com" <hpa@zytor.com>,
"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
"stable@vger.kernel.org" <stable@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error
Date: Thu, 1 Sep 2022 10:50:07 +0000 [thread overview]
Message-ID: <d6a3212aa11c2788d35094739abe40909373cd68.camel@intel.com> (raw)
In-Reply-To: <24906e57-461f-6c94-9e78-0d8507df01bb@intel.com>
On Wed, 2022-08-31 at 13:39 -0700, Reinette Chatre wrote:
> > static int ksgxd(void *p)
> > {
> > + long ret;
> > +
> > set_freezable();
> >
> > /*
> > * Sanitize pages in order to recover from kexec(). The 2nd pass is
> > * required for SECS pages, whose child pages blocked EREMOVE.
> > */
> > - __sgx_sanitize_pages(&sgx_dirty_page_list);
> > - __sgx_sanitize_pages(&sgx_dirty_page_list);
> > + ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > + if (ret == -ECANCELED)
> > + /* kthread stopped */
> > + return 0;
> >
> > - /* sanity check: */
> > - WARN_ON(!list_empty(&sgx_dirty_page_list));
> > + ret = __sgx_sanitize_pages(&sgx_dirty_page_list);
> > + switch (ret) {
> > + case 0:
> > + /* success, no unsanitized pages */
> > + break;
> > +
> > + case -ECANCELED:
> > + /* kthread stopped */
> > + return 0;
> > +
> > + default:
> > + /*
> > + * Never expected to happen in a working driver. If it
> > happens
> > + * the bug is expected to be in the sanitization process,
> > but
> > + * successfully sanitized pages are still valid and driver
> > can
> > + * be used and most importantly debugged without issues. To
> > put
> > + * short, the global state of kernel is not corrupted so no
> > + * reason to do any more complicated rollback.
> > + */
> > + pr_err("%ld unsanitized pages\n", ret);
> > + }
> >
> > while (!kthread_should_stop()) {
> > if (try_to_freeze())
>
>
> 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);
> - __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);
>
This basically means driver bug if I understand correctly. To be consistent
with the behaviour of existing code, how about just WARN()?
...
left_dirty = __sgx_sanitize_pages(&sgx_dirty_page_list);
WARN_ON(left_dirty && !kthread_should_stop());
It seems there's little value to print out the unsanitized pages here. The
existing code doesn't print it anyway.
--
Thanks,
-Kai
next prev parent reply other threads:[~2022-09-01 10:50 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-31 17:38 [PATCH v2 0/6] x86/sgx: A collection of tests and fixes Jarkko Sakkinen
2022-08-31 17:38 ` [PATCH v2 1/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning Jarkko Sakkinen
2022-08-31 17:38 ` [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error Jarkko Sakkinen
2022-08-31 20:39 ` Reinette Chatre
2022-09-01 10:50 ` Huang, Kai [this message]
2022-09-01 21:47 ` jarkko
2022-09-01 21:53 ` Jarkko Sakkinen
2022-09-01 21:56 ` Jarkko Sakkinen
2022-09-01 22:01 ` Jarkko Sakkinen
2022-09-01 22:34 ` Reinette Chatre
2022-09-01 23:56 ` Jarkko Sakkinen
2022-09-02 13:26 ` Jarkko Sakkinen
2022-09-02 15:53 ` Jarkko Sakkinen
2022-09-02 16:08 ` Reinette Chatre
2022-09-02 16:30 ` Jarkko Sakkinen
2022-09-02 17:38 ` Reinette Chatre
2022-09-02 19:20 ` Jarkko Sakkinen
2022-08-31 17:38 ` [PATCH v2 3/6] x86/sgx: Handle VA page allocation failure for EAUG on PF Jarkko Sakkinen
2022-08-31 18:08 ` Reinette Chatre
2022-08-31 18:21 ` Jarkko Sakkinen
2022-08-31 18:33 ` Reinette Chatre
2022-08-31 18:46 ` Jarkko Sakkinen
2022-08-31 17:38 ` [PATCH v2 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long Jarkko Sakkinen
2022-08-31 20:07 ` Reinette Chatre
2022-09-01 22:22 ` Jarkko Sakkinen
2022-09-01 23:12 ` Reinette Chatre
2022-09-02 0:03 ` Jarkko Sakkinen
2022-09-04 4:02 ` Jarkko Sakkinen
2022-09-04 4:21 ` Jarkko Sakkinen
2022-08-31 17:38 ` [PATCH v2 5/6] selftests/sgx: retry the ioctls returned with EAGAIN Jarkko Sakkinen
2022-08-31 20:08 ` Reinette Chatre
2022-08-31 17:38 ` [PATCH v2 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors Jarkko Sakkinen
2022-08-31 20:09 ` Reinette Chatre
2022-09-01 22:24 ` Jarkko Sakkinen
2022-08-31 17:43 ` [PATCH v2 0/6] x86/sgx: A collection of tests and fixes Dave Hansen
2022-08-31 18:11 ` Jarkko Sakkinen
2022-08-31 18:24 ` Dave Hansen
2022-08-31 18:47 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d6a3212aa11c2788d35094739abe40909373cd68.camel@intel.com \
--to=kai.huang@intel.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=haitao.huang@linux.intel.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pmenzel@molgen.mpg.de \
--cc=reinette.chatre@intel.com \
--cc=stable@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=vijay.dhanraj@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).