[PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Heiko Carstens]

Kernel user spaces accesses to not exported pages in atomic context
incorrectly try to resolve the page fault.
With debug options enabled call traces like this can be seen:

BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:1523
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 419074, name: qemu-system-s39
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
Preemption disabled at:
[<00000383ea47cfa2>] copy_page_from_iter_atomic+0xa2/0x8a0
CPU: 12 UID: 0 PID: 419074 Comm: qemu-system-s39
Tainted: G        W           6.16.0-20250531.rc0.git0.69b3a602feac.63.fc42.s390x+debug #1 PREEMPT
Tainted: [W]=WARN
Hardware name: IBM 3931 A01 703 (LPAR)
Call Trace:
 [<00000383e990d282>] dump_stack_lvl+0xa2/0xe8
 [<00000383e99bf152>] __might_resched+0x292/0x2d0
 [<00000383eaa7c374>] down_read+0x34/0x2d0
 [<00000383e99432f8>] do_secure_storage_access+0x108/0x360
 [<00000383eaa724b0>] __do_pgm_check+0x130/0x220
 [<00000383eaa842e4>] pgm_check_handler+0x114/0x160
 [<00000383ea47d028>] copy_page_from_iter_atomic+0x128/0x8a0
([<00000383ea47d016>] copy_page_from_iter_atomic+0x116/0x8a0)
 [<00000383e9c45eae>] generic_perform_write+0x16e/0x310
 [<00000383e9eb87f4>] ext4_buffered_write_iter+0x84/0x160
 [<00000383e9da0de4>] vfs_write+0x1c4/0x460
 [<00000383e9da123c>] ksys_write+0x7c/0x100
 [<00000383eaa7284e>] __do_syscall+0x15e/0x280
 [<00000383eaa8417e>] system_call+0x6e/0x90
INFO: lockdep is turned off.

It is not allowed to take the mmap_lock while in atomic context. Therefore
handle such a secure storage access fault as if the accessed page is not
mapped: the uaccess function will return -EFAULT, and the caller has to
deal with this. Usually this means that the access is retried in process
context, which allows to resolve the page fault (or in this case export the
page).

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/mm/fault.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 3829521450dd..e1ad05bfd28a 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
 		if (rc)
 			BUG();
 	} else {
+		if (faulthandler_disabled())
+			return handle_fault_error_nolock(regs, 0);
 		mm = current->mm;
 		mmap_read_lock(mm);
 		vma = find_vma(mm, addr);
-- 
2.45.2

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Claudio Imbrenda]

On Tue,  3 Jun 2025 15:49:36 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:

> Kernel user spaces accesses to not exported pages in atomic context
> incorrectly try to resolve the page fault.
> With debug options enabled call traces like this can be seen:
> 
> BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:1523
> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 419074, name: qemu-system-s39
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> INFO: lockdep is turned off.
> Preemption disabled at:
> [<00000383ea47cfa2>] copy_page_from_iter_atomic+0xa2/0x8a0
> CPU: 12 UID: 0 PID: 419074 Comm: qemu-system-s39
> Tainted: G        W           6.16.0-20250531.rc0.git0.69b3a602feac.63.fc42.s390x+debug #1 PREEMPT
> Tainted: [W]=WARN
> Hardware name: IBM 3931 A01 703 (LPAR)
> Call Trace:
>  [<00000383e990d282>] dump_stack_lvl+0xa2/0xe8
>  [<00000383e99bf152>] __might_resched+0x292/0x2d0
>  [<00000383eaa7c374>] down_read+0x34/0x2d0
>  [<00000383e99432f8>] do_secure_storage_access+0x108/0x360
>  [<00000383eaa724b0>] __do_pgm_check+0x130/0x220
>  [<00000383eaa842e4>] pgm_check_handler+0x114/0x160
>  [<00000383ea47d028>] copy_page_from_iter_atomic+0x128/0x8a0
> ([<00000383ea47d016>] copy_page_from_iter_atomic+0x116/0x8a0)
>  [<00000383e9c45eae>] generic_perform_write+0x16e/0x310
>  [<00000383e9eb87f4>] ext4_buffered_write_iter+0x84/0x160
>  [<00000383e9da0de4>] vfs_write+0x1c4/0x460
>  [<00000383e9da123c>] ksys_write+0x7c/0x100
>  [<00000383eaa7284e>] __do_syscall+0x15e/0x280
>  [<00000383eaa8417e>] system_call+0x6e/0x90
> INFO: lockdep is turned off.
> 
> It is not allowed to take the mmap_lock while in atomic context. Therefore
> handle such a secure storage access fault as if the accessed page is not
> mapped: the uaccess function will return -EFAULT, and the caller has to
> deal with this. Usually this means that the access is retried in process
> context, which allows to resolve the page fault (or in this case export the
> page).
> 
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

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

> ---
>  arch/s390/mm/fault.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 3829521450dd..e1ad05bfd28a 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		if (rc)
>  			BUG();
>  	} else {
> +		if (faulthandler_disabled())
> +			return handle_fault_error_nolock(regs, 0);
>  		mm = current->mm;
>  		mmap_read_lock(mm);
>  		vma = find_vma(mm, addr);

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Alexander Gordeev]

On Tue, Jun 03, 2025 at 03:49:36PM +0200, Heiko Carstens wrote:
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 3829521450dd..e1ad05bfd28a 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		if (rc)
>  			BUG();
>  	} else {
> +		if (faulthandler_disabled())
> +			return handle_fault_error_nolock(regs, 0);

This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():

		if (WARN_ON_ONCE(!si_code))
			si_code = SEGV_MAPERR;

Would this warning be justified in this case (aka user_mode(regs) == true)?

>  		mm = current->mm;
>  		mmap_read_lock(mm);
>  		vma = find_vma(mm, addr);

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Claudio Imbrenda]

On Wed, 4 Jun 2025 18:27:49 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> On Tue, Jun 03, 2025 at 03:49:36PM +0200, Heiko Carstens wrote:
> > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > index 3829521450dd..e1ad05bfd28a 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
> >  		if (rc)
> >  			BUG();
> >  	} else {
> > +		if (faulthandler_disabled())
> > +			return handle_fault_error_nolock(regs, 0);
> >  
> 
> This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
> 
> 		if (WARN_ON_ONCE(!si_code))
> 			si_code = SEGV_MAPERR;
> 
> Would this warning be justified in this case (aka user_mode(regs) ==
> true)?

I think so, because if we are in usermode, we should never trigger
faulthandler_disabled()

> 
> >  		mm = current->mm;
> >  		mmap_read_lock(mm);
> >  		vma = find_vma(mm, addr);  

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Alexander Gordeev]

On Wed, Jun 04, 2025 at 06:48:55PM +0200, Claudio Imbrenda wrote:
> > > @@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
> > >  		if (rc)
> > >  			BUG();
> > >  	} else {
> > > +		if (faulthandler_disabled())
> > > +			return handle_fault_error_nolock(regs, 0);
> > >  
> > 
> > This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
> > 
> > 		if (WARN_ON_ONCE(!si_code))
> > 			si_code = SEGV_MAPERR;
> > 
> > Would this warning be justified in this case (aka user_mode(regs) ==
> > true)?
> 
> I think so, because if we are in usermode, we should never trigger
> faulthandler_disabled()

I think I do not get you. We are in a system call and also in_atomic(),
so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
is called (above).

> 
> > 
> > >  		mm = current->mm;
> > >  		mmap_read_lock(mm);
> > >  		vma = find_vma(mm, addr);  

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Claudio Imbrenda]

On Wed, 4 Jun 2025 19:29:31 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> On Wed, Jun 04, 2025 at 06:48:55PM +0200, Claudio Imbrenda wrote:
> > > > @@ -441,6 +441,8 @@ void do_secure_storage_access(struct pt_regs *regs)
> > > >  		if (rc)
> > > >  			BUG();
> > > >  	} else {
> > > > +		if (faulthandler_disabled())
> > > > +			return handle_fault_error_nolock(regs, 0);
> > > >    
> > > 
> > > This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
> > > 
> > > 		if (WARN_ON_ONCE(!si_code))
> > > 			si_code = SEGV_MAPERR;
> > > 
> > > Would this warning be justified in this case (aka user_mode(regs) ==
> > > true)?  
> > 
> > I think so, because if we are in usermode, we should never trigger
> > faulthandler_disabled()  
> 
> I think I do not get you. We are in a system call and also in_atomic(),
> so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
> is called (above).

what is the psw in regs?
is it not the one that was being used when the exception was triggered?

> 
> >   
> > >   
> > > >  		mm = current->mm;
> > > >  		mmap_read_lock(mm);
> > > >  		vma = find_vma(mm, addr);    

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Alexander Gordeev]

On Wed, Jun 04, 2025 at 07:40:43PM +0200, Claudio Imbrenda wrote:
> > > > This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
> > > > 
> > > > 		if (WARN_ON_ONCE(!si_code))
> > > > 			si_code = SEGV_MAPERR;
> > > > 
> > > > Would this warning be justified in this case (aka user_mode(regs) ==
> > > > true)?  
> > > 
> > > I think so, because if we are in usermode, we should never trigger
> > > faulthandler_disabled()  
> > 
> > I think I do not get you. We are in a system call and also in_atomic(),
> > so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
> > is called (above).
> 
> what is the psw in regs?
> is it not the one that was being used when the exception was triggered?

Hmm, right. I assume is_kernel_fault() returns false not because 
user_mode(regs) is true, but because we access the secondary AS.

Still, to me it feels wrong to trigger that warning due to a user
process activity. But anyway:

Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Christian Borntraeger]

Am 05.06.25 um 11:04 schrieb Alexander Gordeev:
> On Wed, Jun 04, 2025 at 07:40:43PM +0200, Claudio Imbrenda wrote:
>>>>> This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
>>>>>
>>>>> 		if (WARN_ON_ONCE(!si_code))
>>>>> 			si_code = SEGV_MAPERR;
>>>>>
>>>>> Would this warning be justified in this case (aka user_mode(regs) ==
>>>>> true)?
>>>>
>>>> I think so, because if we are in usermode, we should never trigger
>>>> faulthandler_disabled()
>>>
>>> I think I do not get you. We are in a system call and also in_atomic(),
>>> so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
>>> is called (above).
>>
>> what is the psw in regs?
>> is it not the one that was being used when the exception was triggered?
> 
> Hmm, right. I assume is_kernel_fault() returns false not because
> user_mode(regs) is true, but because we access the secondary AS.
> 
> Still, to me it feels wrong to trigger that warning due to a user
> process activity. But anyway:
> 
> Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

Can we trigger a WARN from userspace?

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Heiko Carstens]

On Thu, Jun 05, 2025 at 11:06:29AM +0200, Christian Borntraeger wrote:
> Am 05.06.25 um 11:04 schrieb Alexander Gordeev:
> > On Wed, Jun 04, 2025 at 07:40:43PM +0200, Claudio Imbrenda wrote:
> > > > > > This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
> > > > > > 
> > > > > > 		if (WARN_ON_ONCE(!si_code))
> > > > > > 			si_code = SEGV_MAPERR;
> > > > > > 
> > > > > > Would this warning be justified in this case (aka user_mode(regs) ==
> > > > > > true)?
> > > > > 
> > > > > I think so, because if we are in usermode, we should never trigger
> > > > > faulthandler_disabled()
> > > > 
> > > > I think I do not get you. We are in a system call and also in_atomic(),
> > > > so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
> > > > is called (above).
> > > 
> > > what is the psw in regs?
> > > is it not the one that was being used when the exception was triggered?
> > 
> > Hmm, right. I assume is_kernel_fault() returns false not because
> > user_mode(regs) is true, but because we access the secondary AS.
> > 
> > Still, to me it feels wrong to trigger that warning due to a user
> > process activity. But anyway:
> > 
> > Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>
> 
> Can we trigger a WARN from userspace?

No. If the warning triggers, then this indicates a bug in the kernel (exit to
user with faulthandler_disabled() == true). I managed to screw up the kernel
exactly with such a bug. See commit 588a9836a4ef ("s390/stacktrace: Use break
instead of return statement"), which lead to random unexplainable user space
crashes.

Note that we have the identical check/code in do_exception().

Re: [PATCH] s390/mm: Fix in_atomic() handling in do_secure_storage_access()

[Christian Borntraeger]

Am 05.06.25 um 12:07 schrieb Heiko Carstens:
> On Thu, Jun 05, 2025 at 11:06:29AM +0200, Christian Borntraeger wrote:
>> Am 05.06.25 um 11:04 schrieb Alexander Gordeev:
>>> On Wed, Jun 04, 2025 at 07:40:43PM +0200, Claudio Imbrenda wrote:
>>>>>>> This could trigger WARN_ON_ONCE() in handle_fault_error_nolock():
>>>>>>>
>>>>>>> 		if (WARN_ON_ONCE(!si_code))
>>>>>>> 			si_code = SEGV_MAPERR;
>>>>>>>
>>>>>>> Would this warning be justified in this case (aka user_mode(regs) ==
>>>>>>> true)?
>>>>>>
>>>>>> I think so, because if we are in usermode, we should never trigger
>>>>>> faulthandler_disabled()
>>>>>
>>>>> I think I do not get you. We are in a system call and also in_atomic(),
>>>>> so faulthandler_disabled() is true and handle_fault_error_nolock(regs, 0)
>>>>> is called (above).
>>>>
>>>> what is the psw in regs?
>>>> is it not the one that was being used when the exception was triggered?
>>>
>>> Hmm, right. I assume is_kernel_fault() returns false not because
>>> user_mode(regs) is true, but because we access the secondary AS.
>>>
>>> Still, to me it feels wrong to trigger that warning due to a user
>>> process activity. But anyway:
>>>
>>> Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>
>>
>> Can we trigger a WARN from userspace?
> 
> No. If the warning triggers, then this indicates a bug in the kernel (exit to
> user with faulthandler_disabled() == true). I managed to screw up the kernel
> exactly with such a bug. See commit 588a9836a4ef ("s390/stacktrace: Use break
> instead of return statement"), which lead to random unexplainable user space
> crashes.

Ok, then this makes a lot of sense to WARN.

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>