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

From: Reinette Chatre <reinette.chatre@intel.com>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: <linux-sgx@vger.kernel.org>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Vijay Dhanraj <vijay.dhanraj@intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Paul Menzel <pmenzel@molgen.mpg.de>, <stable@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an error
Date: Fri, 2 Sep 2022 10:38:34 -0700	[thread overview]
Message-ID: <8d66f94f-9981-1456-9040-066e35c7ba1f@intel.com> (raw)
In-Reply-To: <YxIvr33xgjCbW6qu@kernel.org>

Hi Jarkko,

On 9/2/2022 9:30 AM, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2022 at 09:08:23AM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 9/2/2022 8:53 AM, Jarkko Sakkinen wrote:
>>> On Fri, Sep 02, 2022 at 04:26:51PM +0300, Jarkko Sakkinen wrote:
>>>> +	if (ret)
>>>> +		pr_err("%ld unsanitized pages\n", left_dirty);
>>>
>>> Yeah, I know, should be 'left_dirty'. I just quickly drafted
>>> the patch for the email.
>>>
>>
>> No problem - you did mention that it was an informal patch.
>>
>> (btw ... also watch out for the long local parameter returned
>> as an unsigned long and the signed vs unsigned printing
>> format string.) I also continue to recommend that you trim
> 
> Point taken.
> 
>> that backtrace ... this patch is heading to x86 area where
>> this is required.
> 
> Should I just cut the whole stack trace, and leave the
> part before it?

The trace is printed because of a WARN_ON() in the code.
I do not think there is anything very helpful in that trace.
I think the only helpful parts are the WARN itself that includes
the line number and then information on which kernel it was
encountered on.

How about something like (please note the FIXME within):

"
Paul reported the following WARN while running kernel vFIXME:
  WARNING: CPU: 6 PID: 83 at arch/x86/kernel/cpu/sgx/main.c:401 ksgxd+0x1b7/0x1d0

This is the WARN_ON() within ksgxd() that is triggered when
sgx_dirty_page_list (the list containing unsanitized pages) is non-empty.

In sgx_init(), if misc_register() fails or misc_register() succeeds but
neither sgx_drv_init() nor sgx_vepc_init() succeeds, then ksgxd will be
prematurely stopped. This may leave some unsanitized pages, which does
not matter, because SGX will be disabled for the whole power cycle.

Ultimately this can crash the kernel, if the following is set:

	/proc/sys/kernel/panic_on_warn

In premature stop, print nothing, as the number is by practical means a
random number. Otherwise, it is an indicator of a bug in the driver, and
therefore print the number of unsanitized pages with pr_err().
"

Reinette