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: <3516813d-5d2a-821a-81e8-1ed78ad63561@intel.com>
Date:   Thu, 11 Aug 2022 20:28:37 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [RFC PATCH 1/4] x86/mm/cpa: restore global bit when page is
 present
Content-Language: en-US
To:     Hyeonggon Yoo <42.hyeyoo@gmail.com>
CC:     "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
        "song@kernel.org" <song@kernel.org>
References: <20220808145649.2261258-1-aaron.lu@intel.com>
 <20220808145649.2261258-2-aaron.lu@intel.com>
 <YvSRyjDsrbB7v2JT@ip-172-31-24-42.ap-northeast-1.compute.internal>
 <df4f40ff3e4c408931ed21ab4e8968bdb1871f79.camel@intel.com>
 <YvToWsNqXudd6cSN@hyeyoo>
From:   Aaron Lu <aaron.lu@intel.com>
In-Reply-To: <YvToWsNqXudd6cSN@hyeyoo>
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?UnNzcUtkL0lYMnNCVVZwUWlhSm5kdFJVN2lZYWhSa1JGUDNTajJmQnpYT3F0?=
 =?utf-8?B?K3VFWnJlRFc4NUdUS0NFUnh6NGZXR0ZESm9FWkNsbUtNSFpySkR1MGdYOS9F?=
 =?utf-8?B?b0lKdms1WmJIb2VhYkFEUFpvSEJDUG5YL1NvRlJUcUNyZVN1ZkNVODU2dlVL?=
 =?utf-8?B?VEdTWk5MeS9iSlRRVGZINGkveldlbklJVktlbjlubTVBcDYyelVOQ3puUFU0?=
 =?utf-8?B?d1RGbU9wZDBRRkh5QUVvZ0RacitDSXFQYk1PN1Y3U3pRTFZuR3FCVGs2RnRH?=
 =?utf-8?B?OStwVzlxa0dQYzhEdzErdTRJa3hIYy9SVEtoUytDVS9wQy9zQTVQRDBDVnVH?=
 =?utf-8?B?eTZBMUNnejMzUGhDSldLbjk3VVVwUG9maFVRdkdDd0UvQUo0cFdxNXl6cmVG?=
 =?utf-8?B?ZitmeHFUR3ViNUZMUXBBaGJUVUFqRVFRYjdkT0dlSDR4YVYwZmZIdjV3ZnZl?=
 =?utf-8?B?NG5PS2hiVWlaT0VYdVJCMlh2SGlXRjJ1YTNzMTZRN0VkNXNTakcycFc5bWtu?=
 =?utf-8?B?Z051K2h4RDVlY2JaOVQ0eUh0OTFyTGNmMml6YysxTGNwd2dGbVA3SnNZT0ln?=
 =?utf-8?B?QklUdk5VVnFBckNseWhWOU9sU3VmMU9sS09iODZEZUlVMlhZOEVFNzM5czNC?=
 =?utf-8?B?SDVleU1KWUxIVDJBV21wMVJkTFBxZ0t0TkF2WjJQYzFhdzlQMjc4TDRNL1BX?=
 =?utf-8?B?WDhyRWxjVkZRSWZ3ZTR2QmpydFhRbG5DUFZPR2ZHZXQ4V3c5Z0xROWh3RENn?=
 =?utf-8?B?ci9mOWJnOEVyWW1hQWFiOTJ2TVdJNDhDVk5MTHY2cGMyRE1uRTNWLzNhang1?=
 =?utf-8?B?dVZtRkpEd2kwRVE5YysrQktTMzNmOGhhbDBtU01rbnhlYjg0ZzdIQkt4RnpC?=
 =?utf-8?B?MkVMVFlmWHdVV0RxSUQ2WHNTZFl5M0JPQVZIZHI0RVBjeTI0VDlyelRkYzBN?=
 =?utf-8?B?blh2RUxsem5FS2F5cjRaSlNNbDYxSXFQVkJWN1A5TjUzZ1owUVhRNlRpdi9z?=
 =?utf-8?B?Y1hKNlM1RXI1N2RrZGg2cmk1NnhzNWlRNE1pdWVnTm1FL1JManpPOG5oeUxV?=
 =?utf-8?B?UGVJYzFhZTBrU3d3QnVBNWsyYTAzNk8vcndXSk1TR1JxaEpqMS9EUUp6eUsv?=
 =?utf-8?B?Y3dmREhLM29mWXJ0MUxNWnMyVEJHSzVGek01bmlDLzFOSVFoWDY2NFRlREJ0?=
 =?utf-8?B?blFnTmJNbjdZN0lYYzlOMm0xTnJtT0FpWlB3L25vYjJ6NC8wVk0yL3pxSVgx?=
 =?utf-8?B?WU5YMnBCK2lLSTVXRkJhMWtFaWhTTEE3djBhajFmRjJqSlc0Wk1Gc0FSYS92?=
 =?utf-8?B?TklKakhqRjFOaXRoSHhSSmZZYVc0NXF0elpZNzdzd2JYKzdpd2NFT3lwMDVD?=
 =?utf-8?B?bXhhZjVIL1JIMjVlYk5SbkZCNFNvMnI4MmNOZHN4bWZWdEticUUwNXdnK3F0?=
 =?utf-8?B?WUpkd21nbFlZZHoxam5SS01BdVhRUjc4UklhVnFmQ1pDcVMxbHJrMk9Gcm1v?=
 =?utf-8?B?d0VrRWdVWXpiZU1pS1RuaTZhV0RCNncxd1ZoY3I0MjJIOFM1T1E1SVBGd2du?=
 =?utf-8?B?RXgrdDJ2cTdITjNZcEJoYklLS0ZUM3FOVTIyZlNQdnpJQW94YmtQVENTemI1?=
 =?utf-8?B?Zi95N3E4WDhIQnBqNkh0eHRHVDNKZzVFcng4Skd1SndoR2NSbStRbnZQMzZk?=
 =?utf-8?B?bFhWMU9tUG85UXF5Wnplc1RxSVJMOTBwQWVrV25SL0U0NVBaRnR5TVArWVc0?=
 =?utf-8?B?UlhYV1J3Y3lTRUd6ZnVzd1l2aXNJU2V1M1VlN1JWNlp2ZXlkMHd4Y0ZIRk1B?=
 =?utf-8?B?S3drV2xMS0IyOGFsNDdnR2xqZDBGZ1p3NERFQUpBVFZ6NzdWSTJPd2hwN212?=
 =?utf-8?B?YjN5bnhQVGxGMzY2UUtzWVpxdnp0NkZsV2I1MERPQlFwelFHcCt3NldrODRm?=
 =?utf-8?B?RVBRa1MxTysrR3FISjRaYVh6RDUrUjl3ZUpQWXd1YmFTVVA1bzd5OUQvQUtp?=
 =?utf-8?B?OXhOQVZFa3hYeHhvMHdBbkpyOElaZXJIM0E2YmhocjdlakozdHAzTjBDb2kz?=
 =?utf-8?B?Nk5VUmxyM2R6YzUrV0cwKzFlM0VnZUNqOEFPVndaMHFuL3FWT2hmUnNoeVcw?=
 =?utf-8?Q?wWrjAfmTCn6HpGkzm/8QqBbbQ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 54a4a1f7-e5ea-4cf9-76fb-08da7b9508ab
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Aug 2022 12:28:45.3707
 (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: xXL3qEWjfCtPTyRc9Xv1tOfejirITKhlYiiqvD9YRxpuJqn/Vriqtj8K1AeBcYOwTWf18wJRFtzPsXuIl21yag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5412
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 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_MED,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

On 8/11/2022 7:30 PM, Hyeonggon Yoo wrote:
> On Thu, Aug 11, 2022 at 08:16:08AM +0000, Lu, Aaron wrote:
>> On Thu, 2022-08-11 at 05:21 +0000, Hyeonggon Yoo wrote:
>>> On Mon, Aug 08, 2022 at 10:56:46PM +0800, Aaron Lu wrote:
>>>> For configs that don't have PTI enabled or cpus that don't need
>>>> meltdown mitigation, current kernel can lose GLOBAL bit after a page
>>>> goes through a cycle of present -> not present -> present.
>>>>
>>>> It happened like this(__vunmap() does this in vm_remove_mappings()):
>>>> original page protection: 0x8000000000000163 (NX/G/D/A/RW/P)
>>>> set_memory_np(page, 1):   0x8000000000000062 (NX/D/A/RW) lose G and P
>>>> set_memory_p(pagem 1):    0x8000000000000063 (NX/D/A/RW/P) restored P
>>>>
>>>> In the end, this page's protection no longer has Global bit set and this
>>>> would create problem for this merge small mapping feature.
>>>>
>>>> For this reason, restore Global bit for systems that do not have PTI
>>>> enabled if page is present.
>>>>
>>>> (pgprot_clear_protnone_bits() deserves a better name if this patch is
>>>> acceptible but first, I would like to get some feedback if this is the
>>>> right way to solve this so I didn't bother with the name yet)
>>>>
>>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>>>> ---
>>>>  arch/x86/mm/pat/set_memory.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
>>>> index 1abd5438f126..33657a54670a 100644
>>>> --- a/arch/x86/mm/pat/set_memory.c
>>>> +++ b/arch/x86/mm/pat/set_memory.c
>>>> @@ -758,6 +758,8 @@ static pgprot_t pgprot_clear_protnone_bits(pgprot_t prot)
>>>>  	 */
>>>>  	if (!(pgprot_val(prot) & _PAGE_PRESENT))
>>>>  		pgprot_val(prot) &= ~_PAGE_GLOBAL;
>>>> +	else
>>>> +		pgprot_val(prot) |= _PAGE_GLOBAL & __default_kernel_pte_mask;
>>>>  
>>>>  	return prot;
>>>>  }
>>>
>>> IIUC It makes it unable to set _PAGE_GLOBL when PTI is on.
>>>
>>
>> Yes. Is this a problem?
>> I think that is the intended behaviour when PTI is on: not to enable
>> Gloabl bit on kernel mappings.
> 
> Please note that I'm not expert on PTI.
> 
> but AFAIK with PTI, at least everything (kernel part) mapped to user page table is
> mapped as global when PGE is supported.
> 
> Not sure "Global bit is never used for kernel part when PTI is enabled"
> is true.
>
> Also, commit d1440b23c922d ("x86/mm: Factor out pageattr _PAGE_GLOBAL setting") that introduced
> pgprot_clear_protnone_bits() says:
> 	
> 	This unconditional setting of _PAGE_GLOBAL is a problem when we have
> 	PTI and non-PTI and we want some areas to have _PAGE_GLOBAL and some
> 	not.
> 
> 	This updated version of the code says:
> 	1. Clear _PAGE_GLOBAL when !_PAGE_PRESENT
> 	2. Never set _PAGE_GLOBAL implicitly
> 	3. Allow _PAGE_GLOBAL to be in cpa.set_mask
> 	4. Allow _PAGE_GLOBAL to be inherited from previous PTE
>

Thanks for these info, I'll need to take a closer look at PTI.

>>> Maybe it would be less intrusive to make
>>> set_direct_map_default_noflush() replace protection bits
>>> with PAGE_KENREL as it's only called for direct map, and the function
>>> is to reset permission to default:
>>>
>>> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
>>> index 1abd5438f126..0dd4433c1382 100644
>>> --- a/arch/x86/mm/pat/set_memory.c
>>> +++ b/arch/x86/mm/pat/set_memory.c
>>> @@ -2250,7 +2250,16 @@ int set_direct_map_invalid_noflush(struct page *page)
>>>
>>>  int set_direct_map_default_noflush(struct page *page)
>>>  {
>>> -       return __set_pages_p(page, 1);
>>> +       unsigned long tempaddr = (unsigned long) page_address(page);
>>> +       struct cpa_data cpa = {
>>> +                       .vaddr = &tempaddr,
>>> +                       .pgd = NULL,
>>> +                       .numpages = 1,
>>> +                       .mask_set = PAGE_KERNEL,
>>> +                       .mask_clr = __pgprot(~0),
> 
> Nah, this sets _PAGE_ENC unconditionally, which should be evaluated.
> Maybe less intrusive way would be:
> 		       .mask_set = __pgprot(_PAGE_PRESENT |
> 					   (_PAGE_GLOBAL & __kernel_default_pte_mask)),
>                        .mask_clr = __pgprot(0),
> 
>>> +                       .flags = 0};
>>> +
>>> +       return __change_page_attr_set_clr(&cpa, 0);
>>>  }
>>
>> Looks reasonable to me and it is indeed less intrusive. I'm only
>> concerned there might be other paths that also go through present ->
>> not present -> present and this change can not cover them.
>>
> 
> AFAIK other paths going through present->not present->present (using CPA)
> is only when DEBUG_PAGEALLOC is used.
> 
> Do we care direct map fragmentation when using DEBUG_PAGEALLOC?
> 

No, direct mapping does not use large page mapping when DEBUG_PAGEALLOC.

>>>
>>> set_direct_map_{invalid,default}_noflush() is the exact reason
>>> why direct map become split after vmalloc/vfree with special
>>> permissions.
>>
>> Yes I agree, because it can lose G bit after the whole cycle when PTI
>> is not on. When PTI is on, there is no such problem because G bit is
>> not there initially.
>>
>> Thanks,
>> Aaron
>