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

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

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.