Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <84b8eb06-7b77-675f-5bc8-292fe27dd2f5@intel.com>
Date:   Thu, 1 Sep 2022 15:34:10 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.2.0
Subject: Re: [PATCH v2 2/6] x86/sgx: Do not consider unsanitized pages an
 error
Content-Language: en-US
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>
References: <20220831173829.126661-1-jarkko@kernel.org>
 <20220831173829.126661-3-jarkko@kernel.org>
 <24906e57-461f-6c94-9e78-0d8507df01bb@intel.com>
 <YxEp8Ji+ukLBoNE+@kernel.org>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <YxEp8Ji+ukLBoNE+@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?M01VazF0eFJNK0QzU3BDTUlQUm5sWmlWbzI4Y1dldkRlY29haXdoek5xWmxF?=
 =?utf-8?B?SzMvN1k2SjJHbi9jMnpWSzJIQzNvZkFGTVdRYTRIUlhORjVIUXBtRm11MENP?=
 =?utf-8?B?RjZZajUvQ2RyM0R6WnZZV053ZzhDWmprMFRPN2xxMmhodVZqYUNtd0UyY1J4?=
 =?utf-8?B?dzVXVllXcE9DaUFnT0NDR295SlZoM1U4dnBzazNGdUo1c1NrV3h2bHFGT2dj?=
 =?utf-8?B?bTdEUkNLTUpZbFBjZHNIK1UyOGo3Q1VjUEtGcXdERVNZdXdRdEhSYkJiejQ1?=
 =?utf-8?B?YXNROFlJd2dLaWF3V2pOODlxRlQ3bGRWamJXcHdWVU15NkROMGxzc3NWOFdY?=
 =?utf-8?B?WDhGQkdRN2ZlTStWZFhpRW1BZTVyejByNWZqSmxuVGZ1YWtaY2NjMjJvdmd0?=
 =?utf-8?B?M2VZYi9GVWE2WDZ4c21wREdNU0VNWFdnM2tIL0w4WEtPVE5taEo3dTRUc3FI?=
 =?utf-8?B?bCs2L3NRWExDNG5SSk9uT2ExOVh1dlRRQ09Hc1JjQng0NEFSNUd2dTdtczIv?=
 =?utf-8?B?Y3F0QVRGcUZpbGdGMkZjMFpVZ1hrSFFUMXozcnJKT3VlZHlwbXBNcE8xWjMy?=
 =?utf-8?B?cTNvTmJWVS9zRC9tMWR2RTVPdFdqdG14d1RoVFVuOHc5K0pVdVZ6Tk5JdlQr?=
 =?utf-8?B?RnZUWEo4Y05ZV0NmU1A0VDdkRCtSVDY4SkUwemVlMHJER1JuNDRxQTE2RnZV?=
 =?utf-8?B?NWF6YWxuTkw4Z2hDV2JlNk5vTnV3NkVuSE9pYUZLdjRuRUNDNTNpb2NoclZK?=
 =?utf-8?B?VEVuMGNmT0VFQ1A1UzRjM2E5WE8xT3Z3RStGTlVqc1JvK2pQQWFMMDY1RUN6?=
 =?utf-8?B?dHRVWkFMc1JXdEJ6bjlmRytpWXlsdXg5cDl1YlI2TWJLU2c0emNOQkJPRkRa?=
 =?utf-8?B?SXBUdUhQa3Y2RlMyYk5aeG53M1F0UlMyaU81OWZBeVk2clpFYlhZTzcwd3N2?=
 =?utf-8?B?NUtNa2ZjV2tsVkZsSnRhWnMzTFRoQWpNVkl5NVc3aVhESWRrc3p0dzJiNG80?=
 =?utf-8?B?RWJHZ3Z3Zk1wdUVZLzBSOE11U2FFWWErVTJZcEc3d2tFM0diUDJ6dmN6ZGRs?=
 =?utf-8?B?YmpoZ0FXaXJQTFBnWnQwZGZWbnp5VkZiRzZDUkh5OWgxemQ0bUwrZCs1UE1D?=
 =?utf-8?B?YnhmRVVKR083OTN5TjF5cXBrU2ZZaldDTkUzS0lZUitiVzVIOWh5RXVWWUMy?=
 =?utf-8?B?QllkWTgzaCtvNVM5WVBSSHFxS2IzS3hGUTFPbzZTeVJua2h3eCthNVZXWlFC?=
 =?utf-8?B?d1BSQ0tDRWM1dnpKNmQvVi8vOEdIRTlwaUVSNlB4Q0V6K3YwVU44OURHN0lS?=
 =?utf-8?B?RkZ4UXBCNTVrcDB3eUNtWjFCS2VKQ2czTC9HSWlTWElhMmtJZHg4eW42NWJx?=
 =?utf-8?B?aG1GVmVsSHFPMU42ejVmL29COTFFZW5xVjdmUVZsaytUMU5IRSt4NnYvalRQ?=
 =?utf-8?B?NGdrOUgrOVk5WFk0cjcxY0IyL0ttNHJVKzFUdnRINWVlcXZvS0hIUXlMVThK?=
 =?utf-8?B?bHUxdm5tK2ZaRm43cmI3cUR3QlhvdmZXQ3FvN09ON0ovaDEzMytacFM5Z1R1?=
 =?utf-8?B?MlhjVkhJcVBpdURrajJqc1dQS3NyUE95aUFlMnBMd2J6YzRtUm9Jck9mYktu?=
 =?utf-8?B?YjVPUWtVUXJGNmFzREltdkZnS05zamNiY2drVzZLZ0NCL1c4cUZINENvLzhl?=
 =?utf-8?B?TnVFd21hdG96WnMyOUtJS2pURjRXSEJCNXdHT1FuSGluZmIzcDZzeTBHcWsw?=
 =?utf-8?B?Z2RRSGhoYUQ2ZE9Pa1RPNElsZDFudS9YUWpTQkRmUlpwL2NsS0pXOU1HU0RJ?=
 =?utf-8?B?NXJ3TUVMd3JkYjNBaURxSWs4eVJaOUp4b2xCUGx4cmRwQ0pvWTR2VnZOcHlh?=
 =?utf-8?B?c1lUZHFSd0dYVXVKYkF6enhPamFQa0JvSUorakxyUlZrRkgreGpJVm1OZHV1?=
 =?utf-8?B?bFRvYXY5QjBSOHJTaUY2dnNrUUtDdjh4RjIzdDZheFJtYWdKRFRqWnJ2WE9v?=
 =?utf-8?B?NitQN1JlZ0F2UEQ1QUtKQ0ZEbFJmYTh6SWpMbzUwUEhrbmFQRGZ5V3ZUYnJO?=
 =?utf-8?B?QVRGQXlpVm9Mb1Y0Z0pSemlvUUx5OHZMRTFVckh6VmExb29JMjBjNmJuaURN?=
 =?utf-8?B?ekQ2N25CVUNhMDhzdGQ5elU4WVZjK3NkNDhmNmpSSXpMcEVpMEw0OFJSUmcx?=
 =?utf-8?B?eGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a19f658a-d5ae-4858-1fc3-08da8c6a189f
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Sep 2022 22:34:13.5412
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Flb1lGiTam958TqikzeJxXTJ6MiZbA7rxZtgOcDNSHf55EHU8zKsrUiXRDnvTjptR+Mo/yX2Q/Tqd8UiKd4ZaRCHgfYRR1/5q+iggJ1TOvo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3572
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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