[PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Janosch Frank]

There are special cases where secure storage access exceptions happen
in a kernel context for pages that don't have the PG_arch_1 bit
set. That bit is set for non-exported guest secure storage (memory)
but is absent on storage donated to the Ultravisor since the kernel
isn't allowed to export donated pages.

Prior to this patch we would try to export the page by calling
arch_make_folio_accessible() which would instantly return since the
arch bit is absent signifying that the page was already exported and
no further action is necessary. This leads to secure storage access
exception loops which can never be resolved.

With this patch we unconditionally try to export and if that fails we
fixup.

Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
Reported-by: Heiko Carstens <hca@linux.ibm.com>
Suggested-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---

Changed fault error handling to nolock. (Heiko)
Added PG_arch_1 cleanup requested off-list. (Claudio)

---
 arch/s390/mm/fault.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index a52aa7a99b6b..191cc53caead 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -441,10 +441,17 @@ void do_secure_storage_access(struct pt_regs *regs)
 		folio = phys_to_folio(addr);
 		if (unlikely(!folio_try_get(folio)))
 			return;
-		rc = arch_make_folio_accessible(folio);
+		rc = uv_convert_from_secure(folio_to_phys(folio));
+		if (!rc)
+			clear_bit(PG_arch_1, &folio->flags.f);
 		folio_put(folio);
+		/*
+		 * There are some valid fixup types for kernel
+		 * accesses to donated secure memory. zeropad is one
+		 * of them.
+		 */
 		if (rc)
-			BUG();
+			return handle_fault_error_nolock(regs, 0);
 	} else {
 		if (faulthandler_disabled())
 			return handle_fault_error_nolock(regs, 0);
-- 
2.51.0

Re: [PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Christian Borntraeger]

Am 10.03.26 um 16:02 schrieb Janosch Frank:
> There are special cases where secure storage access exceptions happen
> in a kernel context for pages that don't have the PG_arch_1 bit
> set. That bit is set for non-exported guest secure storage (memory)
> but is absent on storage donated to the Ultravisor since the kernel
> isn't allowed to export donated pages.
> 
> Prior to this patch we would try to export the page by calling
> arch_make_folio_accessible() which would instantly return since the
> arch bit is absent signifying that the page was already exported and
> no further action is necessary. This leads to secure storage access
> exception loops which can never be resolved.
> 
> With this patch we unconditionally try to export and if that fails we
> fixup.
> 
> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

Re: [PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Claudio Imbrenda]

On Tue, 10 Mar 2026 15:02:42 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> There are special cases where secure storage access exceptions happen
> in a kernel context for pages that don't have the PG_arch_1 bit
> set. That bit is set for non-exported guest secure storage (memory)
> but is absent on storage donated to the Ultravisor since the kernel
> isn't allowed to export donated pages.
> 
> Prior to this patch we would try to export the page by calling
> arch_make_folio_accessible() which would instantly return since the
> arch bit is absent signifying that the page was already exported and
> no further action is necessary. This leads to secure storage access
> exception loops which can never be resolved.
> 
> With this patch we unconditionally try to export and if that fails we
> fixup.
> 
> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
> 
> Changed fault error handling to nolock. (Heiko)
> Added PG_arch_1 cleanup requested off-list. (Claudio)
> 
> ---
>  arch/s390/mm/fault.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index a52aa7a99b6b..191cc53caead 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -441,10 +441,17 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		folio = phys_to_folio(addr);
>  		if (unlikely(!folio_try_get(folio)))
>  			return;
> -		rc = arch_make_folio_accessible(folio);
> +		rc = uv_convert_from_secure(folio_to_phys(folio));
> +		if (!rc)
> +			clear_bit(PG_arch_1, &folio->flags.f);
>  		folio_put(folio);
> +		/*
> +		 * There are some valid fixup types for kernel
> +		 * accesses to donated secure memory. zeropad is one
> +		 * of them.
> +		 */
>  		if (rc)
> -			BUG();
> +			return handle_fault_error_nolock(regs, 0);
>  	} else {
>  		if (faulthandler_disabled())
>  			return handle_fault_error_nolock(regs, 0);

Re: [PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Heiko Carstens]

On Tue, Mar 10, 2026 at 03:02:42PM +0000, Janosch Frank wrote:
> There are special cases where secure storage access exceptions happen
> in a kernel context for pages that don't have the PG_arch_1 bit
> set. That bit is set for non-exported guest secure storage (memory)
> but is absent on storage donated to the Ultravisor since the kernel
> isn't allowed to export donated pages.
> 
> Prior to this patch we would try to export the page by calling
> arch_make_folio_accessible() which would instantly return since the
> arch bit is absent signifying that the page was already exported and
> no further action is necessary. This leads to secure storage access
> exception loops which can never be resolved.
> 
> With this patch we unconditionally try to export and if that fails we
> fixup.
> 
> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> 
> Changed fault error handling to nolock. (Heiko)
> Added PG_arch_1 cleanup requested off-list. (Claudio)
> 
> ---
>  arch/s390/mm/fault.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index a52aa7a99b6b..191cc53caead 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -441,10 +441,17 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		folio = phys_to_folio(addr);
>  		if (unlikely(!folio_try_get(folio)))
>  			return;
> -		rc = arch_make_folio_accessible(folio);
> +		rc = uv_convert_from_secure(folio_to_phys(folio));
> +		if (!rc)
> +			clear_bit(PG_arch_1, &folio->flags.f);
>  		folio_put(folio);

Isn't the clear_bit() racy? That is: another CPU could make the page secure
again, set (the still set) PG_arch_1, and then clear_bit() removes the bit,
and we end up with a secure page where PG_arch_1 is not set?
Which in turn would arch_make_folio_accessible() al

Or is that not possible?

Just wondering, since __make_folio_secure() requires the folio to be locked
when setting PG_arch_1, while clearing happens unlocked. But chances are high
that I don't understand the code.

Re: [PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Janosch Frank]

On 3/11/26 08:00, Heiko Carstens wrote:
> On Tue, Mar 10, 2026 at 03:02:42PM +0000, Janosch Frank wrote:
>> There are special cases where secure storage access exceptions happen
>> in a kernel context for pages that don't have the PG_arch_1 bit
>> set. That bit is set for non-exported guest secure storage (memory)
>> but is absent on storage donated to the Ultravisor since the kernel
>> isn't allowed to export donated pages.
>>
>> Prior to this patch we would try to export the page by calling
>> arch_make_folio_accessible() which would instantly return since the
>> arch bit is absent signifying that the page was already exported and
>> no further action is necessary. This leads to secure storage access
>> exception loops which can never be resolved.
>>
>> With this patch we unconditionally try to export and if that fails we
>> fixup.
>>
>> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
>> Reported-by: Heiko Carstens <hca@linux.ibm.com>
>> Suggested-by: Heiko Carstens <hca@linux.ibm.com>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>
>> Changed fault error handling to nolock. (Heiko)
>> Added PG_arch_1 cleanup requested off-list. (Claudio)
>>
>> ---
>>   arch/s390/mm/fault.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
>> index a52aa7a99b6b..191cc53caead 100644
>> --- a/arch/s390/mm/fault.c
>> +++ b/arch/s390/mm/fault.c
>> @@ -441,10 +441,17 @@ void do_secure_storage_access(struct pt_regs *regs)
>>   		folio = phys_to_folio(addr);
>>   		if (unlikely(!folio_try_get(folio)))
>>   			return;
>> -		rc = arch_make_folio_accessible(folio);
>> +		rc = uv_convert_from_secure(folio_to_phys(folio));
>> +		if (!rc)
>> +			clear_bit(PG_arch_1, &folio->flags.f);
>>   		folio_put(folio);
> 
> Isn't the clear_bit() racy? That is: another CPU could make the page secure
> again, set (the still set) PG_arch_1, and then clear_bit() removes the bit,
> and we end up with a secure page where PG_arch_1 is not set?
> Which in turn would arch_make_folio_accessible() al
> 
> Or is that not possible?
> 
> Just wondering, since __make_folio_secure() requires the folio to be locked
> when setting PG_arch_1, while clearing happens unlocked. But chances are high
> that I don't understand the code.
> 

__make_folio_secure() checks the refcount and if the comments hold true, 
it should protect us from a flag being set as long as we have the extra 
reference which we should have gotten via folio_try_get().

It does not protect us from a double clear.

Re: [PATCH v2] s390/mm: add missing secure storage access fixups for donated memory

[Heiko Carstens]

On Wed, Mar 11, 2026 at 03:17:22PM +0100, Janosch Frank wrote:
> > > -		rc = arch_make_folio_accessible(folio);
> > > +		rc = uv_convert_from_secure(folio_to_phys(folio));
> > > +		if (!rc)
> > > +			clear_bit(PG_arch_1, &folio->flags.f);
> > >   		folio_put(folio);
> > 
> > Isn't the clear_bit() racy? That is: another CPU could make the page secure
> > again, set (the still set) PG_arch_1, and then clear_bit() removes the bit,
> > and we end up with a secure page where PG_arch_1 is not set?
> > Which in turn would arch_make_folio_accessible() al
> > 
> > Or is that not possible?
> > 
> > Just wondering, since __make_folio_secure() requires the folio to be locked
> > when setting PG_arch_1, while clearing happens unlocked. But chances are high
> > that I don't understand the code.
> > 
> 
> __make_folio_secure() checks the refcount and if the comments hold true, it
> should protect us from a flag being set as long as we have the extra
> reference which we should have gotten via folio_try_get().
> 
> It does not protect us from a double clear.

Ok, then it should work. Thanks for explaining!