Re: [PATCH v2 1/9] KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Storage key reference and change bits are lost when executing SSKE with MR (Merge Reference) or MC (Merge Change) flags.
- [High] Race condition when updating hardware storage keys on huge pages (lockless read-modify-write).
--

--- Patch [1]: [PATCH v2 1/9] KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key() ---
commit 4e6ac296606202ca3143d3677ec8bf9361312ec5
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()

Some callers pass NULL as oldkey. This commit checks for NULL and assigns 
the pointer to a dummy local variable before calling 
page_cond_set_storage_key(), preventing a NULL pointer dereference.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 3f2d6e8902d76..165c704fcf299 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -722,9 +722,12 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
>  	if (rc)
>  		return rc;
>  
> -	if (!ptep)
> +	if (!ptep) {
> +		if (!oldkey)
> +			oldkey = &prev;
>  		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
>  						 nq, mr, mc);
> +	}

[Severity: High]
This is a pre-existing issue, but does calling page_cond_set_storage_key()
for huge pages lack synchronization?

For regular pages, this function acquires pgste_get_lock() below to
serialize updates. However, for huge pages, page_cond_set_storage_key() is
called directly without a per-page lock. 

Since page_cond_set_storage_key() performs a non-atomic read-modify-write by
executing page_get_storage_key() and later conditionally updating it with
page_set_storage_key(), can concurrent executions interleave and cause lost
storage key updates or lost change bits?

[Severity: High]
This is also a pre-existing issue, but do we lose the guest's reference and
change bits when the merge reference (mr) or merge change (mc) flags are
true?

In page_cond_set_storage_key(), if the new key differs from the old key, it
calls:

    page_set_storage_key(paddr, skey.skey, !nq);

The skey parameter does not logically OR the old reference and change bits
from oldkey when mr or mc are true. Since this writes directly to hardware,
won't the old hardware bits be permanently overwritten and lost?

Additionally, lower down in dat_cond_set_storage_key(), the software
tracking bits are unconditionally overwritten:

    pgste.gc = skey.c;
    pgste.gr = skey.r;

Could these assignments also drop the old.gc and old.gr bits when the merge
flags are set, potentially leading to a situation where a guest OS discards
a dirty page believing it to be clean?

>  
>  	old = pgste_get_lock(ptep);
>  	pgste = old;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1