[PATCH 0/2] s390: uv: small UV fixes

[PATCH 0/2] s390: uv: small UV fixes

[Janosch Frank]

Two small fixes:
    * Handle 3d PGMs where the address space id is not known
    * Clean up the UV query VCPU number field naming (it's N-1 not N as number in this context means ID)

Janosch Frank (2):
  s390: uv: Fix sysfs max number of VCPUs reporting
  s390: mm: Fix secure storage access exception handling

 arch/s390/boot/uv.c        |  2 +-
 arch/s390/include/asm/uv.h |  4 ++--
 arch/s390/kernel/uv.c      |  2 +-
 arch/s390/mm/fault.c       | 14 ++++++++++++++
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Janosch Frank]

The number reported by the query is N-1 and I think people reading the
sysfs file would expect N instead. For users creating VMs there's no
actual difference because KVM's limit is currently below the UV's
limit.

The naming of the field is a bit misleading. Number in this context is
used like ID and starts at 0. The query field denotes the maximum
number that can be put into the VCPU number field in the "create
secure CPU" UV call.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
Cc: stable@vger.kernel.org
---
 arch/s390/boot/uv.c        | 2 +-
 arch/s390/include/asm/uv.h | 4 ++--
 arch/s390/kernel/uv.c      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
index a15c033f53ca..afb721082989 100644
--- a/arch/s390/boot/uv.c
+++ b/arch/s390/boot/uv.c
@@ -35,7 +35,7 @@ void uv_query_info(void)
 		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
 		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
 		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
-		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
+		uv_info.max_guest_cpu_id = uvcb.max_guest_cpu_num;
 	}
 
 #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index 0325fc0469b7..c484c95ea142 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -96,7 +96,7 @@ struct uv_cb_qui {
 	u32 max_num_sec_conf;
 	u64 max_guest_stor_addr;
 	u8  reserved88[158 - 136];
-	u16 max_guest_cpus;
+	u16 max_guest_cpu_num;
 	u8  reserveda0[200 - 160];
 } __packed __aligned(8);
 
@@ -273,7 +273,7 @@ struct uv_info {
 	unsigned long guest_cpu_stor_len;
 	unsigned long max_sec_stor_addr;
 	unsigned int max_num_sec_conf;
-	unsigned short max_guest_cpus;
+	unsigned short max_guest_cpu_id;
 };
 
 extern struct uv_info uv_info;
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 883bfed9f5c2..b2d2ad153067 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -368,7 +368,7 @@ static ssize_t uv_query_max_guest_cpus(struct kobject *kobj,
 				       struct kobj_attribute *attr, char *page)
 {
 	return scnprintf(page, PAGE_SIZE, "%d\n",
-			uv_info.max_guest_cpus);
+			uv_info.max_guest_cpu_id + 1);
 }
 
 static struct kobj_attribute uv_query_max_guest_cpus_attr =
-- 
2.25.1

Re: [PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Christian Borntraeger]

On 19.01.21 11:04, Janosch Frank wrote:
> The number reported by the query is N-1 and I think people reading the
> sysfs file would expect N instead. For users creating VMs there's no
> actual difference because KVM's limit is currently below the UV's
> limit.
> 
> The naming of the field is a bit misleading. Number in this context is
> used like ID and starts at 0. The query field denotes the maximum
> number that can be put into the VCPU number field in the "create
> secure CPU" UV call.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
> Cc: stable@vger.kernel.org
> ---
>  arch/s390/boot/uv.c        | 2 +-
>  arch/s390/include/asm/uv.h | 4 ++--
>  arch/s390/kernel/uv.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> index a15c033f53ca..afb721082989 100644
> --- a/arch/s390/boot/uv.c
> +++ b/arch/s390/boot/uv.c
> @@ -35,7 +35,7 @@ void uv_query_info(void)
>  		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
>  		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
>  		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
> -		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> +		uv_info.max_guest_cpu_id = uvcb.max_guest_cpu_num;
>  	}
>  
>  #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 0325fc0469b7..c484c95ea142 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -96,7 +96,7 @@ struct uv_cb_qui {
>  	u32 max_num_sec_conf;
>  	u64 max_guest_stor_addr;
>  	u8  reserved88[158 - 136];
> -	u16 max_guest_cpus;
> +	u16 max_guest_cpu_num;

I think it would read better if we name this also max_guest_cpu_id.
Otherwise this looks good.

Re: [PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Janosch Frank]

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

On 1/19/21 11:11 AM, Christian Borntraeger wrote:
> 
> 
> On 19.01.21 11:04, Janosch Frank wrote:
>> The number reported by the query is N-1 and I think people reading the
>> sysfs file would expect N instead. For users creating VMs there's no
>> actual difference because KVM's limit is currently below the UV's
>> limit.
>>
>> The naming of the field is a bit misleading. Number in this context is
>> used like ID and starts at 0. The query field denotes the maximum
>> number that can be put into the VCPU number field in the "create
>> secure CPU" UV call.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
>> Cc: stable@vger.kernel.org
>> ---
>>  arch/s390/boot/uv.c        | 2 +-
>>  arch/s390/include/asm/uv.h | 4 ++--
>>  arch/s390/kernel/uv.c      | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
>> index a15c033f53ca..afb721082989 100644
>> --- a/arch/s390/boot/uv.c
>> +++ b/arch/s390/boot/uv.c
>> @@ -35,7 +35,7 @@ void uv_query_info(void)
>>  		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
>>  		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
>>  		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
>> -		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
>> +		uv_info.max_guest_cpu_id = uvcb.max_guest_cpu_num;
>>  	}
>>  
>>  #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>> index 0325fc0469b7..c484c95ea142 100644
>> --- a/arch/s390/include/asm/uv.h
>> +++ b/arch/s390/include/asm/uv.h
>> @@ -96,7 +96,7 @@ struct uv_cb_qui {
>>  	u32 max_num_sec_conf;
>>  	u64 max_guest_stor_addr;
>>  	u8  reserved88[158 - 136];
>> -	u16 max_guest_cpus;
>> +	u16 max_guest_cpu_num;
> 
> I think it would read better if we name this also max_guest_cpu_id.
> Otherwise this looks good.
> 

Yes, but I wanted to have the same name as in the specification.
So, what do we value more?


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

Re: [PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Christian Borntraeger]

On 19.01.21 11:15, Janosch Frank wrote:
> On 1/19/21 11:11 AM, Christian Borntraeger wrote:
>>
>>
>> On 19.01.21 11:04, Janosch Frank wrote:
>>> The number reported by the query is N-1 and I think people reading the
>>> sysfs file would expect N instead. For users creating VMs there's no
>>> actual difference because KVM's limit is currently below the UV's
>>> limit.
>>>
>>> The naming of the field is a bit misleading. Number in this context is
>>> used like ID and starts at 0. The query field denotes the maximum
>>> number that can be put into the VCPU number field in the "create
>>> secure CPU" UV call.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  arch/s390/boot/uv.c        | 2 +-
>>>  arch/s390/include/asm/uv.h | 4 ++--
>>>  arch/s390/kernel/uv.c      | 2 +-
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
>>> index a15c033f53ca..afb721082989 100644
>>> --- a/arch/s390/boot/uv.c
>>> +++ b/arch/s390/boot/uv.c
>>> @@ -35,7 +35,7 @@ void uv_query_info(void)
>>>  		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
>>>  		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
>>>  		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
>>> -		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
>>> +		uv_info.max_guest_cpu_id = uvcb.max_guest_cpu_num;
>>>  	}
>>>  
>>>  #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>>> index 0325fc0469b7..c484c95ea142 100644
>>> --- a/arch/s390/include/asm/uv.h
>>> +++ b/arch/s390/include/asm/uv.h
>>> @@ -96,7 +96,7 @@ struct uv_cb_qui {
>>>  	u32 max_num_sec_conf;
>>>  	u64 max_guest_stor_addr;
>>>  	u8  reserved88[158 - 136];
>>> -	u16 max_guest_cpus;
>>> +	u16 max_guest_cpu_num;
>>
>> I think it would read better if we name this also max_guest_cpu_id.
>> Otherwise this looks good.
>>
> 
> Yes, but I wanted to have the same name as in the specification.
> So, what do we value more?

I think readability is more important. Maybe add a comment in the structure definition that
explains it?

Re: [PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Claudio Imbrenda]

On Tue, 19 Jan 2021 05:04:01 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> The number reported by the query is N-1 and I think people reading the
> sysfs file would expect N instead. For users creating VMs there's no
> actual difference because KVM's limit is currently below the UV's
> limit.
> 
> The naming of the field is a bit misleading. Number in this context is
> used like ID and starts at 0. The query field denotes the maximum
> number that can be put into the VCPU number field in the "create
> secure CPU" UV call.

once you address Christian's comments:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface
> for Ultravisor information") Cc: stable@vger.kernel.org
> ---
>  arch/s390/boot/uv.c        | 2 +-
>  arch/s390/include/asm/uv.h | 4 ++--
>  arch/s390/kernel/uv.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> index a15c033f53ca..afb721082989 100644
> --- a/arch/s390/boot/uv.c
> +++ b/arch/s390/boot/uv.c
> @@ -35,7 +35,7 @@ void uv_query_info(void)
>  		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
>  		uv_info.max_sec_stor_addr =
> ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE); uv_info.max_num_sec_conf
> = uvcb.max_num_sec_conf;
> -		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> +		uv_info.max_guest_cpu_id = uvcb.max_guest_cpu_num;
>  	}
>  
>  #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 0325fc0469b7..c484c95ea142 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -96,7 +96,7 @@ struct uv_cb_qui {
>  	u32 max_num_sec_conf;
>  	u64 max_guest_stor_addr;
>  	u8  reserved88[158 - 136];
> -	u16 max_guest_cpus;
> +	u16 max_guest_cpu_num;
>  	u8  reserveda0[200 - 160];
>  } __packed __aligned(8);
>  
> @@ -273,7 +273,7 @@ struct uv_info {
>  	unsigned long guest_cpu_stor_len;
>  	unsigned long max_sec_stor_addr;
>  	unsigned int max_num_sec_conf;
> -	unsigned short max_guest_cpus;
> +	unsigned short max_guest_cpu_id;
>  };
>  
>  extern struct uv_info uv_info;
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 883bfed9f5c2..b2d2ad153067 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -368,7 +368,7 @@ static ssize_t uv_query_max_guest_cpus(struct
> kobject *kobj, struct kobj_attribute *attr, char *page)
>  {
>  	return scnprintf(page, PAGE_SIZE, "%d\n",
> -			uv_info.max_guest_cpus);
> +			uv_info.max_guest_cpu_id + 1);
>  }
>  
>  static struct kobj_attribute uv_query_max_guest_cpus_attr =

Re: [PATCH 1/2] s390: uv: Fix sysfs max number of VCPUs reporting

[Cornelia Huck]

On Tue, 19 Jan 2021 05:04:01 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> The number reported by the query is N-1 and I think people reading the
> sysfs file would expect N instead. For users creating VMs there's no
> actual difference because KVM's limit is currently below the UV's
> limit.
> 
> The naming of the field is a bit misleading. Number in this context is
> used like ID and starts at 0. The query field denotes the maximum
> number that can be put into the VCPU number field in the "create
> secure CPU" UV call.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: a0f60f8431999 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
> Cc: stable@vger.kernel.org
> ---
>  arch/s390/boot/uv.c        | 2 +-
>  arch/s390/include/asm/uv.h | 4 ++--
>  arch/s390/kernel/uv.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

[PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Janosch Frank]

Turns out that the bit 61 in the TEID is not always 1 and if that's
the case the address space ID and the address are
unpredictable. Without an address and it's address space ID we can't
export memory and hence we can only send a SIGSEGV to the process or
panic the kernel depending on who caused the exception.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
Cc: stable@vger.kernel.org
---
 arch/s390/mm/fault.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index e30c7c781172..5442937e5b4b 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -791,6 +791,20 @@ void do_secure_storage_access(struct pt_regs *regs)
 	struct page *page;
 	int rc;
 
+	/* There are cases where we don't have a TEID. */
+	if (!(regs->int_parm_long & 0x4)) {
+		/*
+		 * Userspace could for example try to execute secure
+		 * storage and trigger this. We should tell it that it
+		 * shouldn't do that.
+		 */
+		if (user_mode(regs)) {
+			send_sig(SIGSEGV, current, 0);
+			return;
+		} else
+			panic("Unexpected PGM 0x3d with TEID bit 61=0");
+	}
+
 	switch (get_fault_type(regs)) {
 	case USER_FAULT:
 		mm = current->mm;
-- 
2.25.1

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Christian Borntraeger]

On 19.01.21 11:04, Janosch Frank wrote:
> Turns out that the bit 61 in the TEID is not always 1 and if that's
> the case the address space ID and the address are
> unpredictable. Without an address and it's address space ID we can't
> export memory and hence we can only send a SIGSEGV to the process or
> panic the kernel depending on who caused the exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
> Cc: stable@vger.kernel.org

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

some small things to consider (or to reject)

> ---
>  arch/s390/mm/fault.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index e30c7c781172..5442937e5b4b 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -791,6 +791,20 @@ void do_secure_storage_access(struct pt_regs *regs)
>  	struct page *page;
>  	int rc;
>  
> +	/* There are cases where we don't have a TEID. */
> +	if (!(regs->int_parm_long & 0x4)) {
> +		/*
> +		 * Userspace could for example try to execute secure
> +		 * storage and trigger this. We should tell it that it
> +		 * shouldn't do that.

Maybe something like
		/*
		 * when this happens, userspace did something that it
		 * was not supposed to do, e.g. branching into secure
		 * secure memory. Trigger a segmentation fault.
> +		 */
> +		if (user_mode(regs)) {
> +			send_sig(SIGSEGV, current, 0);
> +			return;
> +		} else
> +			panic("Unexpected PGM 0x3d with TEID bit 61=0");

use BUG instead of panic? That would kill this process, but it allows
people to maybe save unaffected data.

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Janosch Frank]

[-- Attachment #1.1.1: Type: text/plain, Size: 1935 bytes --]

On 1/19/21 11:25 AM, Christian Borntraeger wrote:
> 
> 
> On 19.01.21 11:04, Janosch Frank wrote:
>> Turns out that the bit 61 in the TEID is not always 1 and if that's
>> the case the address space ID and the address are
>> unpredictable. Without an address and it's address space ID we can't
>> export memory and hence we can only send a SIGSEGV to the process or
>> panic the kernel depending on who caused the exception.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
>> Cc: stable@vger.kernel.org
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks!

> 
> some small things to consider (or to reject)
> 
>> ---
>>  arch/s390/mm/fault.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
>> index e30c7c781172..5442937e5b4b 100644
>> --- a/arch/s390/mm/fault.c
>> +++ b/arch/s390/mm/fault.c
>> @@ -791,6 +791,20 @@ void do_secure_storage_access(struct pt_regs *regs)
>>  	struct page *page;
>>  	int rc;
>>  
>> +	/* There are cases where we don't have a TEID. */
>> +	if (!(regs->int_parm_long & 0x4)) {
>> +		/*
>> +		 * Userspace could for example try to execute secure
>> +		 * storage and trigger this. We should tell it that it
>> +		 * shouldn't do that.
> 
> Maybe something like
> 		/*
> 		 * when this happens, userspace did something that it
> 		 * was not supposed to do, e.g. branching into secure
> 		 * secure memory. Trigger a segmentation fault.
>> +		 */

Sounds good

>> +		if (user_mode(regs)) {
>> +			send_sig(SIGSEGV, current, 0);
>> +			return;
>> +		} else
>> +			panic("Unexpected PGM 0x3d with TEID bit 61=0");
> 
> use BUG instead of panic? That would kill this process, but it allows
> people to maybe save unaffected data.

That would make sense, will do

[-- Attachment #1.1.2: OpenPGP_0xE354E6B8E238B9F8.asc --]
[-- Type: application/pgp-keys, Size: 7995 bytes --]

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

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

On Tue, 19 Jan 2021 11:38:10 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/19/21 11:25 AM, Christian Borntraeger wrote:
> > 
> > 
> > On 19.01.21 11:04, Janosch Frank wrote:  
> >> Turns out that the bit 61 in the TEID is not always 1 and if that's
> >> the case the address space ID and the address are
> >> unpredictable. Without an address and it's address space ID we can't
> >> export memory and hence we can only send a SIGSEGV to the process or
> >> panic the kernel depending on who caused the exception.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
> >> Cc: stable@vger.kernel.org  
> > 
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>  
> 
> Thanks!
> 
> > 
> > some small things to consider (or to reject)
> >   
> >> ---
> >>  arch/s390/mm/fault.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> >> index e30c7c781172..5442937e5b4b 100644
> >> --- a/arch/s390/mm/fault.c
> >> +++ b/arch/s390/mm/fault.c
> >> @@ -791,6 +791,20 @@ void do_secure_storage_access(struct pt_regs *regs)
> >>  	struct page *page;
> >>  	int rc;
> >>  
> >> +	/* There are cases where we don't have a TEID. */
> >> +	if (!(regs->int_parm_long & 0x4)) {
> >> +		/*
> >> +		 * Userspace could for example try to execute secure
> >> +		 * storage and trigger this. We should tell it that it
> >> +		 * shouldn't do that.  
> > 
> > Maybe something like
> > 		/*
> > 		 * when this happens, userspace did something that it

s/when/When/ :)

> > 		 * was not supposed to do, e.g. branching into secure
> > 		 * secure memory. Trigger a segmentation fault.  
> >> +		 */  
> 
> Sounds good
> 
> >> +		if (user_mode(regs)) {
> >> +			send_sig(SIGSEGV, current, 0);
> >> +			return;
> >> +		} else
> >> +			panic("Unexpected PGM 0x3d with TEID bit 61=0");  
> > 
> > use BUG instead of panic? That would kill this process, but it allows
> > people to maybe save unaffected data.  
> 
> That would make sense, will do

With BUG():

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Heiko Carstens]

On Tue, Jan 19, 2021 at 11:25:01AM +0100, Christian Borntraeger wrote:
> > +		if (user_mode(regs)) {
> > +			send_sig(SIGSEGV, current, 0);
> > +			return;
> > +		} else
> > +			panic("Unexpected PGM 0x3d with TEID bit 61=0");
> 
> use BUG instead of panic? That would kill this process, but it allows
> people to maybe save unaffected data.

It would kill the process, and most likely lead to deadlock'ed
system. But with all the "good" debug information being lost, which
wouldn't be the case with panic().
I really don't think this is a good idea.

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Christian Borntraeger]

On 20.01.21 14:42, Heiko Carstens wrote:
> On Tue, Jan 19, 2021 at 11:25:01AM +0100, Christian Borntraeger wrote:
>>> +		if (user_mode(regs)) {
>>> +			send_sig(SIGSEGV, current, 0);
>>> +			return;
>>> +		} else
>>> +			panic("Unexpected PGM 0x3d with TEID bit 61=0");
>>
>> use BUG instead of panic? That would kill this process, but it allows
>> people to maybe save unaffected data.
> 
> It would kill the process, and most likely lead to deadlock'ed
> system. But with all the "good" debug information being lost, which
> wouldn't be the case with panic().
> I really don't think this is a good idea.
> 

My understanding is that Linus hates panic for anything that might be able
to continue to run. With BUG the admin can decide via panic_on_oops if
debugging data or runtime data is more important. But mm is more on your
side, so if you insist on panic we can keep it.

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Heiko Carstens]

On Wed, Jan 20, 2021 at 03:39:14PM +0100, Christian Borntraeger wrote:
> On 20.01.21 14:42, Heiko Carstens wrote:
> > On Tue, Jan 19, 2021 at 11:25:01AM +0100, Christian Borntraeger wrote:
> >>> +		if (user_mode(regs)) {
> >>> +			send_sig(SIGSEGV, current, 0);
> >>> +			return;
> >>> +		} else
> >>> +			panic("Unexpected PGM 0x3d with TEID bit 61=0");
> >>
> >> use BUG instead of panic? That would kill this process, but it allows
> >> people to maybe save unaffected data.
> > 
> > It would kill the process, and most likely lead to deadlock'ed
> > system. But with all the "good" debug information being lost, which
> > wouldn't be the case with panic().
> > I really don't think this is a good idea.
> > 
> 
> My understanding is that Linus hates panic for anything that might be able
> to continue to run. With BUG the admin can decide via panic_on_oops if
> debugging data or runtime data is more important. But mm is more on your
> side, so if you insist on panic we can keep it.

I prefer to have good debug data - and when we are reaching this
panic, then we _most_ likely have data corruption anywhere (wrong
pointer?). So it seems to be best to me to shutdown the machine
immediately in order to avoid any further corruption instead of hoping
that the system stays somehow alive.

Furthermore a panic is easily detectable by a watchdog, while a BUG
may put the system into a limbo state where the real workload doesn't
work anymore, but the keepalive process does. I don't think this is
desirable.

Re: [PATCH 2/2] s390: mm: Fix secure storage access exception handling

[Claudio Imbrenda]

On Tue, 19 Jan 2021 05:04:02 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Turns out that the bit 61 in the TEID is not always 1 and if that's
> the case the address space ID and the address are
> unpredictable. Without an address and it's address space ID we can't

*its

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> export memory and hence we can only send a SIGSEGV to the process or
> panic the kernel depending on who caused the exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access
> exceptions handlers") Cc: stable@vger.kernel.org
> ---
>  arch/s390/mm/fault.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index e30c7c781172..5442937e5b4b 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -791,6 +791,20 @@ void do_secure_storage_access(struct pt_regs
> *regs) struct page *page;
>  	int rc;
>  
> +	/* There are cases where we don't have a TEID. */
> +	if (!(regs->int_parm_long & 0x4)) {
> +		/*
> +		 * Userspace could for example try to execute secure
> +		 * storage and trigger this. We should tell it that
> it
> +		 * shouldn't do that.
> +		 */
> +		if (user_mode(regs)) {
> +			send_sig(SIGSEGV, current, 0);
> +			return;
> +		} else
> +			panic("Unexpected PGM 0x3d with TEID bit
> 61=0");
> +	}
> +
>  	switch (get_fault_type(regs)) {
>  	case USER_FAULT:
>  		mm = current->mm;