Re: [PATCH 2/2] s390x: Add 3f program exception handler

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3127 bytes --]

On 9/4/20 12:35 PM, Heiko Carstens wrote:
> On Thu, Sep 03, 2020 at 09:14:35AM -0400, Janosch Frank wrote:
>> Program exception 3f (secure storage violation) can only be detected
>> when the CPU is running in SIE with a format 4 state description,
>> e.g. running a protected guest. Because of this and because user
>> space partly controls the guest memory mapping and can trigger this
>> exception, we want to send a SIGSEGV to the process running the guest
>> and not panic the kernel.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> CC: <stable@vger.kernel.org> # 5.7+
>> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>  arch/s390/kernel/pgm_check.S |  2 +-
>>  arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S
>> index 2c27907a5ffc..9a92638360ee 100644
>> --- a/arch/s390/kernel/pgm_check.S
>> +++ b/arch/s390/kernel/pgm_check.S
>> @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception)		/* 3b */
>>  PGM_CHECK_DEFAULT			/* 3c */
>>  PGM_CHECK(do_secure_storage_access)	/* 3d */
>>  PGM_CHECK(do_non_secure_storage_access)	/* 3e */
>> -PGM_CHECK_DEFAULT			/* 3f */
>> +PGM_CHECK(do_secure_storage_violation)	/* 3f */
>>  PGM_CHECK(monitor_event_exception)	/* 40 */
>>  PGM_CHECK_DEFAULT			/* 41 */
>>  PGM_CHECK_DEFAULT			/* 42 */
>> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
>> index 4c8c063bce5b..20abb7c5c540 100644
>> --- a/arch/s390/mm/fault.c
>> +++ b/arch/s390/mm/fault.c
>> @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs)
>>  }
>>  NOKPROBE_SYMBOL(do_non_secure_storage_access);
>>  
>> +void do_secure_storage_violation(struct pt_regs *regs)
>> +{
>> +	char buf[TASK_COMM_LEN];
>> +
>> +	/*
>> +	 * Either KVM messed up the secure guest mapping or the same
>> +	 * page is mapped into multiple secure guests.
>> +	 *
>> +	 * This exception is only triggered when a guest 2 is running
>> +	 * and can therefore never occur in kernel context.
>> +	 */
>> +	printk_ratelimited(KERN_WARNING
>> +			   "Secure storage violation in task: %s, pid %d\n",
>> +			   get_task_comm(buf, current), task_pid_nr(current));
> 
> Why get_task_comm() and task_pid_nr() instead of simply current->comm
> and current->pid?

Normally if there are functions to get data I assume those should be used.

> Also: is the dmesg message of any value?

Yes, it's import for administrators to know that an exception caused
this segfault and not some memory shenanigans.

As the exception only occurs if a guest runs in unsupported modes like
sharing the memory between two secure guests it's a good first
indication what went wrong.

> 
>> +	send_sig(SIGSEGV, current, 0);
>> +}
>> +NOKPROBE_SYMBOL(do_secure_storage_violation);>
> Why is this NOKPROBE? Can this deadlock?
Right, we don't need to export this at all, this was copied over from
the export function above.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]