Re: [PATCH v2 1/5] x86/vsyscall: Reorganize the page fault emulation code

[Sohil Mehta <sohil.mehta@intel.com>]

On 3/5/2026 2:36 PM, H. Peter Anvin wrote:
> On 2026-03-05 13:40, Sohil Mehta wrote:

>> +bool emulate_vsyscall_pf(unsigned long error_code, struct pt_regs *regs,
>> +			 unsigned long address)
>> +{
>> +	/* Write faults or kernel-privilege faults never get fixed up. */
>> +	if ((error_code & (X86_PF_WRITE | X86_PF_USER)) != X86_PF_USER)
>> +		return false;
> 
> 
> I think this can be tightened further.  If X86_PF_PK, X86_PF_SHSTK or
> X86_PF_RSVD are set we should definitely not try to do any emulation, and I
> believe the same is true for X86_PF_SGX or X86_PF_RMP; I'm not 100% as I don't
> have the semantics of those bits in my head at the moment.
> 

Could some of this already be (or might need to be) taken care of in the
calling function do_user_addr_fault(). For example, I see comments such as:

* PKRU never rejects instruction fetches, so we don't need
* to consider the PF_PK bit.

* Read-only permissions can not be expressed in shadow stack PTEs.
* Treat all shadow stack accesses as WRITE faults.

I would prefer to avoid changing any logic for the existing #PF
emulation handling in this patch. If it's okay, we could pursue these as
a follow-on to this series.


>> +	/*
>> +	 * Assume that faults at regs->ip are because of an instruction
>> +	 * fetch. Return early and avoid emulation for faults during
>> +	 * data accesses:
>> +	 */
>> +	if (address != regs->ip) {
>> +		 /* User code tried and failed to read the vsyscall page. */
>> +		if (vsyscall_mode != EMULATE)
>> +			warn_bad_vsyscall(KERN_INFO, regs,
>> +					  "vsyscall read attempt denied -- look up the vsyscall kernel parameter if you need a workaround");
>> +
>> +		return false;
>> +	}
>> +
> 
> I don't really like the reshuffling of the code here.
> 

Sure, I'll keep the flow same as the original code. Will change it in
the next revision.

>> +	/*
>> +	 * X86_PF_INSTR is only set when NX is supported.  When
>> +	 * available, use it to double-check that the emulation code
>> +	 * is only being used for instruction fetches:
>> +	 */
>> +	if (cpu_feature_enabled(X86_FEATURE_NX))
>> +		WARN_ON_ONCE(!(error_code & X86_PF_INSTR));
>> +
> 
> I realize this is the same as the previous code, but I really think this
> should have a "return false;" in it as well.
> 

Yes, returning early makes sense if the warning triggers. But we would
need to change the logic to:

	if (cpu_feature_enabled(X86_FEATURE_NX) &&
	    WARN_ON_ONCE(!(error_code & X86_PF_INSTR)))
		return false;

Again, I would like to avoid such a logic change in this patch as it
only focuses on code reorganization. Would it need to be backported?

Preferably I could send it out as a follow-on patch along with the other
tightening that you mentioned above. Your suggestions seem fine to me,
but I really want to understand and evaluate the changes before sending
them out.

>> +	return __emulate_vsyscall(regs, address);
>> +}
>> +
>>  /*

> 
> Other than the above minor nitpicks, looks good to me.
> 
> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> 

Thank you!