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@xxxxxxxxxxxxx>
> > Suggested-by: Heiko Carstens <hca@xxxxxxxxxxxxx>
> > Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
> > ---
> >
> > 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.