Received-SPF: pass (google.com: domain of linux-kernel+bounces-202977-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <ee4d5664-a426-48d3-ad40-b33ac5f44a7f@os.amperecomputing.com>
Date: Wed, 5 Jun 2024 10:31:51 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [v3 PATCH] arm64: mm: force write fault for atomic RMW
 instructions
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: will@kernel.org, anshuman.khandual@arm.com, scott@os.amperecomputing.com,
 cl@gentwo.org, linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org
References: <20240604171516.2361853-1-yang@os.amperecomputing.com>
 <ZmCYLvb_JlkCuc_u@arm.com>
Content-Language: en-US
From: Yang Shi <yang@os.amperecomputing.com>
In-Reply-To: <ZmCYLvb_JlkCuc_u@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?SjRTSVZlNVV6SXp3eGNRRVVZMWt4Uzk5RG9WTWpxb2Q0NlFXb3JDWm5CN21s?=
 =?utf-8?B?MnZRUnc4UWZ5Q2oreGVrS1c3ckttK1d3bWJhWTlhQ1ZTUHlXcHFNanI3RC9N?=
 =?utf-8?B?ODJlenNuSUUzK0hXbDJoZm54K29oY0NPeVlQTGxtQnpxUkpPWjZkbkdXR01i?=
 =?utf-8?B?bncxZkVUNzVkWExlZkU4OTRLYTB1ZVFpR1BDQ0FIMmg2WU5qWjl1K2I1MHl1?=
 =?utf-8?B?M1ZLSjZWc1N1SFhGQUloMy9SQ000NVc1V2tWcHJFMmNTSkxhSC9lcFBYeEJX?=
 =?utf-8?B?UEFybnhsdDZRa0dJbTJIbUJNeVdZd3lqV2J6cFg4UDdmekJVcDFqdnhINkN4?=
 =?utf-8?B?amVOS2N3MzhkdHMweDBIZTE2aDJWbjBMV2YwbWRvSWFSZFBMUG5BcWVOT25T?=
 =?utf-8?B?aURjMVREQTYzdHpzVExnblNyUUI1VGlObHRGVWdrTkRCZTVxYWFXOUFCeXdT?=
 =?utf-8?B?WFdJdHNDV3RZSDhRd0Z5d0FTcGh2WG1mb0pLUFRrYmZrN0FnYXBmSDdHLzJo?=
 =?utf-8?B?c2M5eEc3ZDF6RmsvM1FnYnpKM01oT1lTWDRwc0MzRzkxbjRNeWdHYyt0RDZH?=
 =?utf-8?B?a29sa3FsMTZkdFpQRWV3V0xHam1VNWtqVzYvU3ZORkVjZmxneVEzTUlvNlhS?=
 =?utf-8?B?NjZWTGJzWlk5ZVBDRjVOZTJtaWt6elY1b1RwMlkyMzJaZkU0eFZlZ3VrT0Z5?=
 =?utf-8?B?WXd0a25uZWFIcFJxaHZQUVYyRTUveG53cDVYOVgyWGpNRFFnNm4rU1pRQjhW?=
 =?utf-8?B?ZFJCRTcvOHlsT2VkRFI2elJ1emVpdmhjTmZGcS9mSEdqNklndVF1eitkREdy?=
 =?utf-8?B?NkRvQ3VPc29Lb0FLa0NxY2Z0MVM0ZkdwSHZ4TDM5RVQ1Z0N4eFRrZys5QWdB?=
 =?utf-8?B?bTVmMGlPMGtvTTIvSDYrbWs5MXFwd1BVUnl5UUJZaU95YWpRbmMrSzgveWtF?=
 =?utf-8?B?QWUzdVd3WjRXcjdCVG5qOFl3T1Uwb1BXTUF1Q1JJc2J1YU5qYit1Wmp4ZXRP?=
 =?utf-8?B?U0ZpWVNmOTlsRHlmK08waEprYkxxVEZjWHN2MmpkT0dlRzdEbm9kR2c1aGJ5?=
 =?utf-8?B?a2FZWHhLd1JIdDI2TG41N0syaGdWMnZtY05UNktHQmxnU3FibzhhRGVvWmlx?=
 =?utf-8?B?TnVFdENZc00zdmVxMTQzTVhsSCthdy9MYjFjc3NBRHkwSExNVlF0OEJXQXRm?=
 =?utf-8?B?WU1Ka2JYRVNnNkJQVTNURzFpb1VvNEROMC9wUnFMVm55b2pPNnBhQ01FbEJv?=
 =?utf-8?B?MjZrbUtHNWZ1bGx0VFFJaldSVGRUanFZVklQUU5mU3N0bUpUVEwyekZ6SXIy?=
 =?utf-8?B?dFFYRHE3V3l2MkU0eFg1OXlLVzI1WHNjZllaY2g5TjhVaWJnZXhWK3RJRFJN?=
 =?utf-8?B?NWRpRXRMQVJraHp3SlFZTWVnQlljQ2o5LzBNT2tHQUsrdE9kUHNZSW0vei9E?=
 =?utf-8?B?RUZ0MUxTb05PWDBhejNUVFpuQ1ZOQ1dkbllleVpaVVFOWVF5U0luazRTYlJ3?=
 =?utf-8?B?dEFHcjFtdGFkclh5Y08zRWU1NXM5ODFIVDBBTzE1anZxYkVibER1OVRrVVY5?=
 =?utf-8?B?QzJRN1d4cFYrVkR6T2l6NGRXV2k4U1pFZXUrc0NCVExxNlFCdm9QK2VBa09D?=
 =?utf-8?B?LytocExpRjQ3VllSbUFWQzlqNzZSQ081K0tDVkd1WUZUYTBmRDJkbXRTc0Yz?=
 =?utf-8?B?Y0pmYjhGOUdBazF6TnBEYmxDQTlxb296eGNOc3J3cTVxc05KSlRwVmFPUmFI?=
 =?utf-8?B?cmowa3VZWU42NDI1cXZHSzVSTnFTQjNoN2VvUDEyazdGc1JVc2wxckZBRW9Q?=
 =?utf-8?B?VG5PSmtKNnNSRjdkYjlLVEI2NlRONHdzeVJscUsxNjdrclJGczlhSVZYbm9K?=
 =?utf-8?B?elByZzdWYVJYRjhEYmVNTkhiK3J6S2VDRjFYemt5YTNINnE5U2dFdkFzWHd3?=
 =?utf-8?B?WlNyZ21lZnFxL3ExVVdPU0pKSU53WWxoRE9leVV1b2RTMGs5MDFPWE9oK3E4?=
 =?utf-8?B?Z0lvNk8rbDFCSUlJR0xtSmVxUzc4QllTYzFhbXhka1h2Um80aEJXeCtUTXA2?=
 =?utf-8?B?WEw4bW5HNkZVSjR5cnBaTkpEQWVGaFJEemQ4REtsS2RVN1lMT2FSdFJzeEpr?=
 =?utf-8?B?cm1pL1ZrQWFjbU1BZEgrbWdwVys3MlRJVnEzRXhPci9XcHJxelJqUGFFU0VM?=
 =?utf-8?Q?oG6HuxJtT7LJToyHVgUKm8M=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e16f587-208e-4639-d1b2-08dc858564c9
X-MS-Exchange-CrossTenant-AuthSource: BYAPR01MB5463.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 17:31:54.8877
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UR2EE/xlfDuXBEbnT68Rw6AhY61+bCFpJY1pKEmti7QD3Ogu29pYEEk1CafD6y7uwv+kKfZllkgHmuDURHbUKb1tEoU+jktuMQlcI88zkBU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR01MB7748

>> @@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>>   		/* It was write fault */
>>   		vm_flags = VM_WRITE;
>>   		mm_flags |= FAULT_FLAG_WRITE;
>> +	} else if (is_el0_atomic_instr(regs)) {
>> +		/* Force write fault */
>> +		vm_flags = VM_WRITE;
>> +		mm_flags |= FAULT_FLAG_WRITE;
>> +		force_write = true;
>>   	} else {
>>   		/* It was read fault */
>>   		vm_flags = VM_READ;
>> @@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>>   	if (!vma)
>>   		goto lock_mmap;
>>   
>> +	/* vma flags don't allow write, undo force write */
>> +	if (force_write && !(vma->vm_flags & VM_WRITE)) {
>> +		vm_flags |= VM_READ;
>> +		if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
>> +			vm_flags |= VM_EXEC;
>> +		mm_flags &= ~FAULT_FLAG_WRITE;
>> +	}
> Ah, this revert to the non-write flags doesn't look great as we
> basically duplicate the 'else' block in the original check. So, it
> probably look better as per your earlier patch to just do the
> instruction read just before the !(vma->vm_flags & flags) check,
> something like:
>
> 	if ((vma->vm_flags & VM_WRITE) && is_el0_atomic_instr(regs)) {
> 		vm_flags = VM_WRITE;
> 		mm_flags |= FAULT_FLAG_WRITE;
> 	}
>
> This way we also only read the instruction if the vma is writeable. I
> think it's fine to do this under the vma lock since we have
> pagefault_disable() for the insn read.

I think we also need to skip this for write fault and instruction fault. 
Some something like:

@@ -529,6 +557,7 @@ static int __kprobes do_page_fault(unsigned long 
far, unsigned long esr,
         unsigned int mm_flags = FAULT_FLAG_DEFAULT;
         unsigned long addr = untagged_addr(far);
         struct vm_area_struct *vma;
+       bool may_force_write = false;

         if (kprobe_page_fault(regs, esr))
                 return 0;
@@ -565,6 +594,7 @@ static int __kprobes do_page_fault(unsigned long 
far, unsigned long esr,
                 /* If EPAN is absent then exec implies read */
                 if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
                         vm_flags |= VM_EXEC;
+               may_force_write = true;
         }

         if (is_ttbr0_addr(addr) && is_el1_permission_fault(addr, esr, 
regs)) {
@@ -586,6 +616,12 @@ static int __kprobes do_page_fault(unsigned long 
far, unsigned long esr,
         if (!vma)
                 goto lock_mmap;

+       if (may_force_write && (vma->vm_flags & VM_WRITE) &&
+           is_el0_atomic_instr(regs)) {
+               vm_flags = VM_WRITE;
+               mm_flags |= FAULT_FLAG_WRITE;
+       }
+
         if (!(vma->vm_flags & vm_flags)) {
                 vma_end_read(vma);
                 goto lock_mmap;

>