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
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 <[email protected]>
Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
Cc: [email protected]
---
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
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 <[email protected]>
> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
> Cc: [email protected]
Reviewed-by: Christian Borntraeger <[email protected]>
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.
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 <[email protected]>
>> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
>> Cc: [email protected]
>
> Reviewed-by: Christian Borntraeger <[email protected]>
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
On Tue, 19 Jan 2021 11:38:10 +0100
Janosch Frank <[email protected]> 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 <[email protected]>
> >> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access exceptions handlers")
> >> Cc: [email protected]
> >
> > Reviewed-by: Christian Borntraeger <[email protected]>
>
> 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 <[email protected]>
On Tue, 19 Jan 2021 05:04:02 -0500
Janosch Frank <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Fixes: 084ea4d611a3d ("s390/mm: add (non)secure page access
> exceptions handlers") Cc: [email protected]
> ---
> 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;
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.
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.
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.