s390: Reintroduce support for DCACHE_WORD_ACCESS
Heiko Carstens
maintainerv4 cover s390: Reintroduce support for DCACHE_WORD_ACCESS
v4:
- Add two more cleanup patches based on Sashiko feedback, however I don't consider them as bug fixes. This is more cosmetics:
- Use handle_fault_error() in do_secure_storage_access()
- Use goto statement in do_secure_storage_access()
v3:
- Fix two more pre-existing bugs reported by Sashiko
- Add Christian's Tested-by tag to four of the six patches
v2:
- Explicitely PAGE_ALIGN size in uv_alloc_stor_var() [Sashiko]
- Document that lazy PTE update / TLB flushing is intended [Sashiko]
v1:
Support for DCACHE_WORD_ACCESS was recently removed [1] since it caused problems with the incomplete handling of secure storage access exceptions. It looked like fixing the exception handler would be a larger effort; therefore support for DCACHE_WORD_ACCESS was removed as a work around.
Address the potential problems that exist with secure storage access exceptions and add support for DCACHE_WORD_ACCESS again.
In particular address the following problems:
- Reading the guest variable storage area via the /proc/kcore interface results in short reads. Address this by using a VM_SPARSE area for the guest variable storage area. VM_SPARSE areas will be handled like memory holes (zeros will be read).
- Fix handling of secure storage access exceptions in vmalloc area.
- Remove folio handling for secure storage access exceptions to avoid potential data corruption.
[1] 37540b8c287f ("s390: Revert support for DCACHE_WORD_ACCESS")
Heiko Carstens (8):
KVM: s390: pv: Use VM_SPARSE area for guest variable storage area s390/mm: Add missing mm check to do_secure_storage_access() s390/mm: Use lock_mm_and_find_vma() in do_secure_storage_access() s390/mm: Fix handling of vmalloc area in do_secure_storage_access() s390/mm: Remove folio handling for kernel faults in do_secure_storage_access() s390/mm: Use handle_fault_error() in do_secure_storage_access() s390/mm: Use goto statement in do_secure_storage_access()
s390: Add support for DCACHE_WORD_ACCESS (again)
arch/s390/Kconfig | 1 + arch/s390/include/asm/asm-extable.h | 4 ++ arch/s390/include/asm/uv.h | 2 + arch/s390/include/asm/word-at-a-time.h | 22 +++++++++ arch/s390/kernel/uv.c | 65 ++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 6 +-- arch/s390/mm/extable.c | 18 +++++++ arch/s390/mm/fault.c | 35 ++++++-------- 8 files changed, 128 insertions(+), 25 deletions(-)
Signature · 1 line
2.53.0
Heiko Carstens
maintainerPATCH v4 1/8 KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
The guest variable storage area is allocated with vmalloc and then donated to the ultravisor. Any kernel access to that area will result in a secure storage access exception (aka fault).
This is a problem if such a memory area is read via /proc/kcore. This causes an exception via vread_iter() and results in an unexpected short read. Avoid this by allocating a custom VM_SPARSE area. If such an area is read, vread_iter() returns zeroes for the entire area.
Note that the function which frees the area does not update ptes. This is intentional to allow for deferred / lazy pte updates and TLB flushing like the generic vfree() code is doing that. See vunmap_pte_range().
This assumes that s390 will gain full support for lazy_mmu_mode_enable() and lazy_mmu_mode_disable() in the future, since as of now the used ptep_get_and_clear() in vunmap_pte_range() does indeed invalidate and flush every single pte entry, but only for s390.
--- arch/s390/include/asm/uv.h | 2 ++ arch/s390/kernel/uv.c | 65 ++++++++++++++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 6 ++-- 3 files changed, 70 insertions(+), 3 deletions(-)
arch/s390/include/asm/uv.h +2 -0
@@ -635,6 +635,8 @@ int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio);int __make_folio_secure (struct folio *folio,struct uv_cb_header *uvcb);int uv_convert_from_secure (unsigned long paddr);int uv_convert_from_secure_folio (struct folio *folio); +void *uv_alloc_stor_var (unsigned long size); +void uv_free_stor_var (void *stor_var);void setup_uv (void );
arch/s390/kernel/uv.c +65 -0
77 lines not on this page · raw message
arch/s390/kvm/pv.c +3 -3
@@ -337,7 +337,7 @@ int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)/* only free resources when the destroy was successful */ static void kvm_s390_pv_dealloc_vm (struct kvm *kvm) { -vfree (kvm->arch.pv.stor_var); +uv_free_stor_var (kvm->arch.pv.stor_var);free_pages (kvm->arch.pv.stor_base,get_order (uv_info.guest_base_stor_len));kvm_s390_clear_pv_state (kvm);
@@ -369,7 +369,7 @@ static int kvm_s390_pv_alloc_vm(struct kvm *kvm)/* Allocate variable storage */ vlen =ALIGN (virt * ((npages *PAGE_SIZE ) /HPAGE_SIZE ),PAGE_SIZE ); vlen += uv_info.guest_virt_base_stor_len; - kvm->arch.pv.stor_var =vzalloc (vlen); + kvm->arch.pv.stor_var =uv_alloc_stor_var (vlen);if (!kvm->arch.pv.stor_var) goto out_err;return 0 ;
@@ -414,7 +414,7 @@ static int kvm_s390_pv_dispose_one_leftover(struct kvm *kvm,*/ free_pages (leftover->stor_base,get_order (uv_info.guest_base_stor_len));free_pages (leftover->old_gmap_table,CRST_ALLOC_ORDER ); -vfree (leftover->stor_var); +uv_free_stor_var (leftover->stor_var); done_fast:atomic_dec (&kvm->mm->context.protected_count);return 0 ;
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Physical pages are returned to the buddy allocator while their page table entries (PTEs) are left…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Physical pages are returned to the buddy allocator while their page table entries (PTEs) are left valid, creating a Use-After-Free / Map-After-Free window.
- [Medium] Sleeping memory allocation within a pte_fn_t callback violates the lazy_mmu_mode API contract.
--
--- Patch [1]: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area --- commit 66226034e9e83eee2fe85a1e6e86baa666a0331b
Author: Heiko Carstens <[email protected]>
KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
The patch replaces a standard vmalloc allocation for the guest variable storage area with a custom VM_SPARSE area allocated via get_vm_area(). It populates and tears down this memory using apply_to_page_range() and apply_to_existing_page_range() callbacks.
Quoted 3 lines
arch/s390/kernel/uv.c +0
-0
[ ... ]
arch/s390/kernel/uv.c int uv_convert_from_secure_pte(pte_t pte) · line 224
Quoted 12 lines
arch/s390/kernel/uv.c +12
-0+static int uv_free_range_cb (pte_t *ptep,unsigned long addr,void *data) +{ +pte_t pte =ptep_get (ptep); + +if (!pte_present (pte)) +return 0 ; +/* +* Note: do not update the pte here, since there is no code which +* accesses the memory range, besides bugs. The invalidation of ptes +* and TLB flushing is deferred like for regular vfree() calls. +*/ +__free_page (pte_page (pte));
[Severity: High]
Does freeing the physical page without clearing the page table entry introduce a regression where the memory could be reallocated while the kernel retains a valid mapping to it?
The comment mentions that vfree() defers PTE invalidation, but standard vfree() clears PTEs synchronously and only defers the TLB flush.
Because apply_to_existing_page_range() returns before free_vm_area() is called to unmap the area, could speculative execution or a hardware page table walk access the new owner's memory after it has been returned to the buddy allocator?
Quoted 2 lines
arch/s390/kernel/uv.c +2
-0+return 0 ; +}
[ ... ]
arch/s390/kernel/uv.c int uv_convert_from_secure_pte(pte_t pte) · line 250
Quoted 6 lines
arch/s390/kernel/uv.c +6
-0+static int uv_alloc_range_cb (pte_t *ptep,unsigned long addr,void *data) +{ +struct page *page; +pte_t pte; + + page =alloc_page (GFP_KERNEL_ACCOUNT | __GFP_ZERO);
[Severity: Medium]
Is it safe to use sleeping allocation flags during this page table walk?
The apply_to_page_range() function wraps iteration in lazy_mmu_mode_enable() and lazy_mmu_mode_disable(). The MM page table guidelines state that lazy MMU mode implies a possible atomic context because it disables preemption on some architectures, meaning pte_fn_t callbacks must not sleep.
Could this cause a regression if the callback blocks, and would it be better to use a non-sleeping allocation flag to conform to the apply_to_page_range() contract?
Quoted 6 lines
arch/s390/kernel/uv.c +6
-0+if (!page) +return -ENOMEM ; + pte =__pte (page_to_phys (page) |pgprot_val (PAGE_KERNEL )); +set_pte (ptep, pte); +return 0 ; +}
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 13 lines
The guest variable storage area is allocated with vmalloc and then donated to the ultravisor. Any kernel access to that area will result in a secure storage access exception (aka fault).
This is a problem if such a memory area is read via
/proc/kcore. This causes an exception viavread_iter()and results in an unexpected short read. Avoid this by allocating a customVM_SPARSEarea. If such an area is read,vread_iter()returns zeroes for the entire area.Note that the function which frees the area does not update ptes. This is intentional to allow for deferred / lazy pte updates and TLB flushing like the generic
vfree()code is doing that. Seevunmap_pte_range().
Quoted 7 lines
This assumes that s390 will gain full support for
lazy_mmu_mode_enable()andlazy_mmu_mode_disable()in the future, since as of now the usedptep_get_and_clear()invunmap_pte_range()does indeed invalidate and flush every single pte entry, but only for s390.Signed-off-by Heiko Carstens <[email protected]>
[..]> +static int uv_free_range_cb(pte_t *ptep, unsigned long addr, void *data)
arch/s390/kernel/uv.c int uv_convert_from_secure_pte(pte_t pte) · line 218
Quoted 5 lines
arch/s390/kernel/uv.c +5
-0+{ +pte_t pte =ptep_get (ptep); + +if (!pte_present (pte)) +return 0 ;
I guess we do not need it today, but it certainly does not hurt?
Heiko Carstens
maintainerRe: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
Quoted 7 lines · Christian Borntraeger
[..]> +static int
uv_free_range_cb(pte_t *ptep, unsigned long addr, void *data)arch/s390/kernel/uv.c int uv_convert_from_secure_pte(pte_t pte) · line 218
Quoted 5 lines
arch/s390/kernel/uv.c +5
-0+{ +pte_t pte =ptep_get (ptep); + +if (!pte_present (pte)) +return 0 ;I guess we do not need it today, but it certainly does not hurt?
It is not needed, just a sanity check.
Alexander Gordeev
maintainerRe: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
In reply to Heiko CarstensOn Mon, Jul 20, 2026 at 10:58:27AM +0200, Heiko Carstens wrote:
...
Quoted 4 lines
This assumes that s390 will gain full support for
lazy_mmu_mode_enable()andlazy_mmu_mode_disable()in the future, since as of now the usedptep_get_and_clear()invunmap_pte_range()does indeed invalidate and flush every single pte entry, but only for s390.
...
Review
Quoted 6 lines
(quoted diff) +6
-0+static int uv_alloc_range_cb(pte_t *ptep, unsigned long addr, void *data) +{ + struct page *page; + pte_t pte; + + page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
In lazy mode this callback is called with preemption disabled, so it leads to:
[ 138.709287] BUG: sleeping function called from invalid context at ./include/linux/sched/mm.h:322
[ 138.709535] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 6092, name: qemu-kvm
[ 138.709540] preempt_count: 1, expected: 0
[ 138.709545] RCU nest depth: 0, expected: 0
[ 138.709549] locks held by qemu-kvm/6092: 1, last CPU#1:
[ 138.709554] #0: 00000162368b8ba8 (&kvm->lock){+.+.}-{3:3}, at: kvm_s390_handle_pv+0x7c/0xf70 [kvm]
[ 138.709612] Preemption disabled at:
[ 138.709614] [<000002690b609a76>] enter_ipte_range.part.0+0x36/0xb0
[ 138.709627] CPU: 1 UID: 107 PID: 6092 Comm: qemu-kvm Not tainted 7.3.0-20260901.rc1.git22.7cbe5e7f09fc.300.mm.fc44.s390x+debug #1 PREEMPT
[ 138.709629] Hardware name: IBM 3931 A01 701 (LPAR)
[ 138.709630] Call Trace:
[ 138.709631] [<000002690b5d0d4e>] dump_stack_lvl+0xae/0x108
[ 138.709634] [<000002690b67bbde>] __might_resched+0x1de/0x2f0
[ 138.709637] [<000002690ba1d23a>] prepare_alloc_pages+0x1ba/0x220
[ 138.709641] [<000002690ba1f936>] __alloc_frozen_pages_noprof+0xc6/0x390
[ 138.709643] [<000002690ba6e9d6>] alloc_pages_mpol+0xe6/0x220
[ 138.709647] [<000002690ba6f1dc>] alloc_frozen_pages_noprof+0x5c/0x80
[ 138.709649] [<000002690ba6f222>] alloc_pages_noprof+0x22/0x80
[ 138.709652] [<000002690b5ef6a0>] uv_alloc_range_cb+0x30/0x300
[ 138.709655] [<000002690b9d22fa>] apply_to_pte_range+0x11a/0x3b0
[ 138.709656] [<000002690b9dbf5a>] apply_to_pmd_range+0x13a/0x250
[ 138.709658] [<000002690b9dcef2>] __apply_to_page_range+0x232/0x4d0
[ 138.709660] [<000002690b9dd1b8>] apply_to_page_range+0x28/0x40
[ 138.709662] [<000002690b5eebfe>] uv_alloc_stor_var+0x5e/0x90
[ 138.709664] [<000002688b73ffe8>] kvm_s390_pv_alloc_vm+0x118/0x1e0 [kvm]
[ 138.709685] [<000002688b741274>] kvm_s390_pv_init_vm+0x84/0x2e0 [kvm]
[ 138.709705] [<000002688b71cf92>] kvm_s390_handle_pv+0x502/0xf70 [kvm]
[ 138.709726] [<000002688b71fd44>] kvm_arch_vm_ioctl+0x234/0xdd0 [kvm]
[ 138.709747] [<000002688b7070fa>] kvm_vm_ioctl+0x33a/0x890 [kvm]
[ 138.709765] [<000002690bb046ca>] __s390x_sys_ioctl+0xfa/0x130
[ 138.709767] [<000002690c78cffc>] __do_syscall+0x1fc/0x700
[ 138.709772] [<000002690c7a21e2>] system_call+0x72/0x90
[ 138.709774] locks held by qemu-kvm/6092: 1, last CPU#1:
[ 138.709775] #0: 00000162368b8ba8 (&kvm->lock){+.+.}-{3:3}, at: kvm_s390_handle_pv+0x7c/0xf70 [kvm]This could be solved using the below fixup:
arch/s390/kernel/uv.c +2 -0
@@ -247,7 +247,9 @@ static int uv_alloc_range_cb(pte_t *ptep, unsigned long addr, void *data)struct page *page;pte_t pte; +lazy_mmu_mode_pause (); page =alloc_page (GFP_KERNEL_ACCOUNT | __GFP_ZERO); +lazy_mmu_mode_resume ();if (!page)return -ENOMEM ; pte =__pte (page_to_phys (page) |pgprot_val (PAGE_KERNEL ));
The downside is lazy_mmu_mode_resume() does not really re-enable the caching and the performance will stay the same even when the lazy mode is supported on s390.
This is the same pattern as kasan_populate_vmalloc_pte() - which was the only occurrence so far.
Alternatively, the page could be allocated atomically, but I think that is less preferrable.
Quoted 8 lines
(quoted diff) +8
-0+ if (!page) + return -ENOMEM; + pte = __pte(page_to_phys(page) | pgprot_val(PAGE_KERNEL)); + set_pte(ptep, pte); + return 0; +} + +void *uv_alloc_stor_var(unsigned long size)
Review
Quoted 10 lines
(quoted diff) +10
-0+{ + struct vm_struct *area; + unsigned long addr; + + size = PAGE_ALIGN(size); + area = get_vm_area(size, VM_SPARSE); + if (!area) + return NULL; + addr = (unsigned long)area->addr; + if (apply_to_page_range(&init_mm, addr, size, uv_alloc_range_cb, NULL))
lazy_mmu_mode_enable_with_ptes() called from apply_to_pte_range() disables preemption.
Review
Quoted 7 lines
(quoted diff) +7
-0+ goto out; + return area->addr; +out: + uv_free_stor_var(area->addr); + return NULL; +} +EXPORT_SYMBOL_FOR_MODULES(uv_alloc_stor_var, "kvm");
Thanks!
Heiko Carstens
maintainerRe: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
Quoted 16 lines · Alexander Gordeev
On Mon, Jul 20, 2026 at 10:58:27AM +0200, Heiko Carstens wrote:
...Quoted 4 lines
This assumes that s390 will gain full support for
lazy_mmu_mode_enable()andlazy_mmu_mode_disable()in the future, since as of now the usedptep_get_and_clear()invunmap_pte_range()does indeed invalidate and flush every single pte entry, but only for s390....
Review
Quoted 6 lines
(quoted diff) +6
-0+static int uv_alloc_range_cb(pte_t *ptep, unsigned long addr, void *data) +{ + struct page *page; + pte_t pte; + + page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);In lazy mode this callback is called with preemption disabled, so it leads to:
Quoted 2 lines
[ 138.709287] BUG: sleeping function called from invalid context at ./include/linux/sched/mm.h:322
[ 138.709535] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 6092, name: qemu-kvm...
Any chance you missed to provide an important detail: this can only happen with your not-yet upstream series, but as of now there is no upstream problem?
Is this correct?
Quoted 17 lines
This could be solved using the below fixup:
arch/s390/kernel/uv.c +2
-0@@ -247,7 +247,9 @@ static int uv_alloc_range_cb(pte_t *ptep, unsigned long addr, void *data)struct page *page;pte_t pte; +lazy_mmu_mode_pause (); page =alloc_page (GFP_KERNEL_ACCOUNT | __GFP_ZERO); +lazy_mmu_mode_resume ();if (!page)return -ENOMEM ; pte =__pte (page_to_phys (page) |pgprot_val (PAGE_KERNEL ));The downside is
lazy_mmu_mode_resume()does not really re-enable the caching and the performance will stay the same even when the lazy mode is supported on s390.
Quoted 5 lines
This is the same pattern as
kasan_populate_vmalloc_pte()- which was the only occurrence so far.Alternatively, the page could be allocated atomically, but I think that is less preferrable.
I guess the real fix is to pre-allocate all pages, keep the pointers to the struct pages in an array, and populate with a different mechanism. Similar like the vmalloc code is doing.
I wanted to keep this code as simple as possible, but...
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ if (apply_to_page_range(&init_mm, addr, size, uv_alloc_range_cb, NULL))
lazy_mmu_mode_enable_with_ptes()called fromapply_to_pte_range()disables preemption.
There is no lazy_mmu_mode_enable_with_ptes() upstream, nor does s390 select ARCH_HAS_LAZY_MMU_MODE until now. Therefore my question above.
I guess my assumption above is correct and there is real urgency to fix this.
Heiko Carstens
maintainerRe: [PATCH v4 1/8] KVM: s390: pv: Use VM_SPARSE area for guest variable storage area
Quoted 22 lines · Heiko Carstens
Quoted 21 lines · Alexander Gordeev
On Mon, Jul 20, 2026 at 10:58:27AM +0200, Heiko Carstens wrote:
This could be solved using the below fixup:(quoted diff) +2
-0@@ -247,7 +247,9 @@ static int uv_alloc_range_cb(pte_t *ptep, unsigned long addr, void *data) struct page *page; pte_t pte; + lazy_mmu_mode_pause(); page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO); + lazy_mmu_mode_resume(); if (!page) return -ENOMEM; pte = __pte(page_to_phys(page) | pgprot_val(PAGE_KERNEL));The downside is
lazy_mmu_mode_resume()does not really re-enable the caching and the performance will stay the same even when the lazy mode is supported on s390.This is the same pattern as
kasan_populate_vmalloc_pte()- which was the only occurrence so far.
Quoted 7 lines
Quoted 2 lines
Alternatively, the page could be allocated atomically, but I think that is less preferrable.
I guess the real fix is to pre-allocate all pages, keep the pointers to the struct pages in an array, and populate with a different mechanism. Similar like the vmalloc code is doing.
I wanted to keep this code as simple as possible, but...
Actually addressing this makes the code even simpler. Something like the below should do (untested).
--- arch/s390/kernel/uv.c | 59 ++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 35 deletions(-)
arch/s390/kernel/uv.c +24 -35
@@ -14,8 +14,8 @@#include <linux/memblock.h> #include <linux/pagemap.h> #include <linux/swap.h> -#include <linux/pagewalk.h> #include <linux/backing-dev.h> +#include <linux/slab.h> #include <linux/vmalloc.h> #include <asm/facility.h> #include <asm/sections.h>
@@ -210,62 +210,51 @@ int uv_convert_from_secure_pte(pte_t pte)return uv_convert_from_secure_folio (pfn_folio (pte_pfn (pte))); } -static int uv_free_range_cb (pte_t *ptep,unsigned long addr,void *data) -{ -pte_t pte =ptep_get (ptep); - -if (!pte_present (pte)) -return 0 ; -/* -* Note: do not update the pte here, since there is no code which -* accesses the memory range, besides bugs. The invalidation of ptes -* and TLB flushing is deferred like for regular vfree() calls. -*/ -__free_page (pte_page (pte)); -return 0 ; -} -void uv_free_stor_var (void *stor_var) { -unsigned long addr, size;struct vm_struct *area; +unsigned long i;if (!stor_var)return ; area =find_vm_area (stor_var);if (WARN_ON_ONCE (!area || !(area->flags &VM_SPARSE )))return ; - size =get_vm_area_size (area); - addr = (unsigned long )area->addr; -apply_to_existing_page_range (&init_mm, addr, size, uv_free_range_cb,NULL ); +/* +* Do not update PTEs. There is no code which accesses the memory +* range, besides bugs. The invalidation of PTEs and TLB flushing +* is deferred like for regular vfree() calls. +*/ +for (i =0 ; i < area->nr_pages; i++) +__free_page (area->pages[i]); +kvfree (area->pages); + area->pages =NULL ;free_vm_area (area); }EXPORT_SYMBOL_FOR_MODULES (uv_free_stor_var,"kvm" ); -static int uv_alloc_range_cb (pte_t *ptep,unsigned long addr,void *data) -{ -struct page *page; -pte_t pte; - - page =alloc_page (GFP_KERNEL_ACCOUNT | __GFP_ZERO); -if (!page) -return -ENOMEM ; - pte =__pte (page_to_phys (page) |pgprot_val (PAGE_KERNEL )); -set_pte (ptep, pte); -return 0 ; -} -void *uv_alloc_stor_var (unsigned long size) { +unsigned long i, nr_pages, addr;struct vm_struct *area; -unsigned long addr; size =PAGE_ALIGN (size); + nr_pages = size >>PAGE_SHIFT ; area =get_vm_area (size,VM_SPARSE );if (!area)return NULL ; addr = (unsigned long )area->addr; -if (apply_to_page_range (&init_mm, addr, size, uv_alloc_range_cb,NULL )) + area->pages =kvcalloc (nr_pages,sizeof (struct page *),GFP_KERNEL_ACCOUNT ); +if (!area->pages) + goto out; +for (i =0 ; i < nr_pages; i++) { + area->nr_pages = i; + area->pages[i] =alloc_page (GFP_KERNEL_ACCOUNT | __GFP_ZERO); +if (!area->pages[i]) + goto out; + } + area->nr_pages = nr_pages; +if (vm_area_map_pages (area, addr, addr + size, area->pages)) goto out;return area->addr; out:
Signature · 1 line
2.53.0
Heiko Carstens
maintainerPATCH v4 2/8 s390/mm: Add missing mm check to do_secure_storage_access()
In reply to Heiko CarstensSimilar like the normal exception handler the secure storage access exception handler needs to verify if mm is not NULL, which may happen with kernel threads.
--- arch/s390/mm/fault.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
arch/s390/mm/fault.c +2 -3
@@ -409,9 +409,9 @@ void do_secure_storage_access(struct pt_regs *regs) {union teid teid = { .val = regs->int_parm_long };unsigned long addr =get_fault_address (regs); +struct mm_struct *mm = current->mm;struct vm_area_struct *vma;struct folio_walk fw; -struct mm_struct *mm;struct folio *folio;int rc;
@@ -453,9 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)if (rc)return handle_fault_error_nolock (regs,0 ); }else { -if (faulthandler_disabled ()) +if (faulthandler_disabled () || !mm)return handle_fault_error_nolock (regs,0 ); - mm = current->mm;mmap_read_lock (mm); vma =find_vma (mm, addr);if (!vma)
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 2/8] s390/mm: Add missing mm check to do_secure_storage_access()
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing VMA boundary check in user space fault path can trigger a kernel WARN_ON_ONCE due to a race condition with concurrent munmap.
- [High] Using virt_to_folio() on arbitrary kernel fault addresses can cause a kernel panic if the address is in vmalloc space.
- [High] Failing to run exception fixups for kernel-mode faults in do_secure_storage_access() causes an infinite loop. --
--- Patch [2]: [PATCH v4 2/8] s390/mm: Add missing mm check to do_secure_storage_access() --- commit 61b1851bdfb6749b6398a064bf9e76283ed6b1e3
Author: Heiko Carstens <[email protected]>
s390/mm: Add missing mm check to do_secure_storage_access()
Similar like the normal exception handler the secure storage access exception handler needs to verify if mm is not NULL, which may happen with kernel threads.
Quoted 15 lines
arch/s390/mm/fault.c +1
-1@@ -409,9 +409,9 @@ void do_secure_storage_access(struct pt_regs *regs) {union teid teid = { .val = regs->int_parm_long };unsigned long addr =get_fault_address (regs); +struct mm_struct *mm = current->mm;struct vm_area_struct *vma;struct folio_walk fw; -struct mm_struct *mm;struct folio *folio;int rc;
[ ... ]
arch/s390/mm/fault.c
Quoted 4 lines
arch/s390/mm/fault.c +0
-0panic ("Unexpected PGM 0x3d with TEID bit 61=0" ); }if (is_kernel_fault (regs)) { folio =virt_to_folio ((void *)addr);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This isn't a bug introduced by this patch, but does using virt_to_folio() unconditionally here cause a kernel panic if the fault address is in vmalloc space?
If the kernel accesses donated secure memory that was allocated via vmalloc, virt_to_folio() on s390 uses __pa(), which only works for direct-mapped addresses. This would produce a garbage physical address, leading to an out-of-bounds pointer dereference in the subsequent folio_try_get() check.
Quoted 3 lines
arch/s390/mm/fault.c +0
-0if (unlikely (!folio_try_get (folio)))return ; rc =uv_convert_from_secure (folio_to_phys (folio));
[ ... ]
arch/s390/mm/fault.c void do_secure_storage_access(struct pt_regs *regs) · line 459
Quoted 9 lines
arch/s390/mm/fault.c +1
-2if (rc)return handle_fault_error_nolock (regs,0 ); }else { -if (faulthandler_disabled ()) +if (faulthandler_disabled () || !mm)return handle_fault_error_nolock (regs,0 ); - mm = current->mm;mmap_read_lock (mm); vma =find_vma (mm, addr);
^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This is a pre-existing issue, but does find_vma() properly check that the address is within the VMA's boundaries before we call folio_walk_start()?
find_vma() returns the first VMA with vm_end greater than addr, but its vm_start could also be greater than addr if the faulting address was just unmapped by a concurrent munmap(). If this happens, we would pass a VMA to folio_walk_start() that doesn't actually contain the address, which triggers its strict WARN_ON_ONCE assertion. Should this be using vma_lookup() instead?
Quoted 4 lines
arch/s390/mm/fault.c +0
-0if (!vma)return handle_fault_error (regs,SEGV_MAPERR ); folio =folio_walk_start (&fw, vma, addr,0 );if (!folio) {
arch/s390/mm/fault.c
Quoted 10 lines
arch/s390/mm/fault.c +0
-0mmap_read_unlock (mm);return ; }/* arch_make_folio_accessible() needs a raised refcount. */ folio_get (folio); rc =arch_make_folio_accessible (folio);folio_put (folio);folio_walk_end (&fw, vma);if (rc)send_sig (SIGSEGV , current,0 );
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This isn't a bug introduced by this patch, but does this error path cause an infinite loop if the fault was triggered by a kernel-mode access like copy_to_user()?
For kernel faults that happen in the user address space, is_kernel_fault() is false. If arch_make_folio_accessible() fails here, we send a SIGSEGV and return without calling fixup_exception(). Because fixup_exception() is not run, the kernel will return to the faulting instruction rather than an error path, causing the instruction to endlessly re-execute and lock up the CPU.
Quoted 3 lines
arch/s390/mm/fault.c +0
-0mmap_read_unlock (mm); } }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v4 2/8] s390/mm: Add missing mm check to do_secure_storage_access()
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 7 lines
Similar like the normal exception handler the secure storage access exception handler needs to verify if mm is not NULL, which may happen with kernel threads.
Reported-by sashiko-bot <[email protected]>Signed-off-by Heiko Carstens <[email protected]>
Quoted 29 lines
arch/s390/mm/fault.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)arch/s390/mm/fault.c +2
-3@@ -409,9 +409,9 @@ void do_secure_storage_access(struct pt_regs *regs) {union teid teid = { .val = regs->int_parm_long };unsigned long addr =get_fault_address (regs); +struct mm_struct *mm = current->mm;struct vm_area_struct *vma;struct folio_walk fw; -struct mm_struct *mm;struct folio *folio;int rc;@@ -453,9 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)if (rc)return handle_fault_error_nolock (regs,0 ); }else { -if (faulthandler_disabled ()) +if (faulthandler_disabled () || !mm)return handle_fault_error_nolock (regs,0 ); - mm = current->mm;mmap_read_lock (mm); vma =find_vma (mm, addr);if (!vma)
Heiko Carstens
maintainerPATCH v4 3/8 s390/mm: Use lock_mm_and_find_vma() in do_secure_storage_access()
In reply to Heiko Carstensdo_secure_storage_access() uses find_vma() without verifying that the faulting address is within the returned vma. Add this missing check by converting to lock_mm_and_find_vma().
This is not a critical fix, since the worst that could happen is a WARN_ON_ONCE() in folio_walk_start().
--- arch/s390/mm/fault.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
arch/s390/mm/fault.c +2 -3
@@ -455,10 +455,9 @@ void do_secure_storage_access(struct pt_regs *regs) }else {if (faulthandler_disabled () || !mm)return handle_fault_error_nolock (regs,0 ); -mmap_read_lock (mm); - vma =find_vma (mm, addr); + vma =lock_mm_and_find_vma (mm, addr, regs);if (!vma) -return handle_fault_error (regs,SEGV_MAPERR ); +return handle_fault_error_nolock (regs,SEGV_MAPERR ); folio =folio_walk_start (&fw, vma, addr,0 );if (!folio) {mmap_read_unlock (mm);
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 3/8] s390/mm: Use lock_mm_and_find_vma() in do_secure_storage_access()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 11 lines
do_secure_storage_access()usesfind_vma()without verifying that the faulting address is within the returned vma. Add this missing check by converting tolock_mm_and_find_vma().This is not a critical fix, since the worst that could happen is a
WARN_ON_ONCE()infolio_walk_start().
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH v4 3/8] s390/mm: Use lock_mm_and_find_vma() in do_secure_storage_access()
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 11 lines
do_secure_storage_access()usesfind_vma()without verifying that the faulting address is within the returned vma. Add this missing check by converting tolock_mm_and_find_vma().This is not a critical fix, since the worst that could happen is a
WARN_ON_ONCE()infolio_walk_start().
Quoted 21 lines
--- arch/s390/mm/fault.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)arch/s390/mm/fault.c +2
-3@@ -455,10 +455,9 @@ void do_secure_storage_access(struct pt_regs *regs) }else {if (faulthandler_disabled () || !mm)return handle_fault_error_nolock (regs,0 ); -mmap_read_lock (mm); - vma =find_vma (mm, addr); + vma =lock_mm_and_find_vma (mm, addr, regs);if (!vma) -return handle_fault_error (regs,SEGV_MAPERR ); +return handle_fault_error_nolock (regs,SEGV_MAPERR ); folio =folio_walk_start (&fw, vma, addr,0 );if (!folio) {mmap_read_unlock (mm);
Heiko Carstens
maintainerPATCH v4 4/8 s390/mm: Fix handling of vmalloc area in do_secure_storage_access()
In reply to Heiko CarstensSecure storage access exceptions in vmalloc area need to be handled separately since folio_try_get() or virt_to_phys() do not work on such addresses. Given that there cannot be a valid access let such exceptions fail immediately. The kernel will either fixup or crash.
--- arch/s390/mm/fault.c | 2 ++ 1 file changed, 2 insertions(+)
arch/s390/mm/fault.c +2 -0
@@ -438,6 +438,8 @@ void do_secure_storage_access(struct pt_regs *regs)panic ("Unexpected PGM 0x3d with TEID bit 61=0" ); }if (is_kernel_fault (regs)) { +if (is_vmalloc_addr ((void *)addr)) +return handle_fault_error_nolock (regs,0 ); folio =virt_to_folio ((void *)addr);if (unlikely (!folio_try_get (folio)))return ;
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 4/8] s390/mm: Fix handling of vmalloc area in do_secure_storage_access()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 7 lines
Secure storage access exceptions in vmalloc area need to be handled separately since
folio_try_get()orvirt_to_phys()do not work on such addresses. Given that there cannot be a valid access let such exceptions fail immediately. The kernel will either fixup or crash.Signed-off-by Heiko Carstens <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v4 4/8] s390/mm: Fix handling of vmalloc area in do_secure_storage_access()
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 7 lines
Secure storage access exceptions in vmalloc area need to be handled separately since
folio_try_get()orvirt_to_phys()do not work on such addresses. Given that there cannot be a valid access let such exceptions fail immediately. The kernel will either fixup or crash.Signed-off-by Heiko Carstens <[email protected]>
Quoted 16 lines
arch/s390/mm/fault.c | 2 ++ 1 file changed, 2 insertions(+)arch/s390/mm/fault.c +2
-0@@ -438,6 +438,8 @@ void do_secure_storage_access(struct pt_regs *regs)panic ("Unexpected PGM 0x3d with TEID bit 61=0" ); }if (is_kernel_fault (regs)) { +if (is_vmalloc_addr ((void *)addr)) +return handle_fault_error_nolock (regs,0 ); folio =virt_to_folio ((void *)addr);if (unlikely (!folio_try_get (folio)))return ;
Heiko Carstens
maintainerPATCH v4 5/8 s390/mm: Remove folio handling for kernel faults in do_secure_storage_access()
In reply to Heiko CarstensRemove folio handling for secure storage access exceptions that happen via the kernel mapping. Using folio_try_get() for exceptions via the kernel mapping is racy: for example such an exception may happen on one CPU, while a different CPU frees the corresponding page. Subsequently the page can be part of a folio of different size, before the faulting CPU executes folio_try_get(). This may lead to data corruption.
As of now this scenario does not seem to be possible, however with the upcoming load_unaligned_zeropad() this is possible.
As a side effect PG_arch_1 is not cleared anymore for such faults via the kernel mapping. However given that this bit is over-indicating it shouldn't matter.
--- arch/s390/mm/fault.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
arch/s390/mm/fault.c +1 -7
@@ -440,13 +440,7 @@ void do_secure_storage_access(struct pt_regs *regs)if (is_kernel_fault (regs)) {if (is_vmalloc_addr ((void *)addr))return handle_fault_error_nolock (regs,0 ); - folio =virt_to_folio ((void *)addr); -if (unlikely (!folio_try_get (folio))) -return ; - rc =uv_convert_from_secure (folio_to_phys (folio)); -if (!rc) -clear_bit (PG_arch_1, &folio->flags.f); -folio_put (folio); + rc =uv_convert_from_secure (__pa (addr));/* * There are some valid fixup types for kernel * accesses to donated secure memory. zeropad is one
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 5/8] s390/mm: Remove folio handling for kernel faults in do_secure_storage_access()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 10 lines
Remove folio handling for secure storage access exceptions that happen via the kernel mapping. Using
folio_try_get()for exceptions via the kernel mapping is racy: for example such an exception may happen on one CPU, while a different CPU frees the corresponding page. Subsequently the page can be part of a folio of different size, before the faulting CPU executesfolio_try_get(). This may lead to data corruption.As of now this scenario does not seem to be possible, however with the upcoming
load_unaligned_zeropad()this is possible.
Quoted 6 lines
As a side effect
PG_arch_1is not cleared anymore for such faults via the kernel mapping. However given that this bit is over-indicating it shouldn't matter.Signed-off-by Heiko Carstens <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Re: [PATCH v4 5/8] s390/mm: Remove folio handling for kernel faults in do_secure_storage_access()
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 10 lines
Remove folio handling for secure storage access exceptions that happen via the kernel mapping. Using
folio_try_get()for exceptions via the kernel mapping is racy: for example such an exception may happen on one CPU, while a different CPU frees the corresponding page. Subsequently the page can be part of a folio of different size, before the faulting CPU executesfolio_try_get(). This may lead to data corruption.As of now this scenario does not seem to be possible, however with the upcoming
load_unaligned_zeropad()this is possible.
Quoted 6 lines
As a side effect
PG_arch_1is not cleared anymore for such faults via the kernel mapping. However given that this bit is over-indicating it shouldn't matter.Signed-off-by Heiko Carstens <[email protected]>
Ideally Claudio should confirm that PG_arch_1 is over-indicating.
Quoted 23 lines
--- arch/s390/mm/fault.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)arch/s390/mm/fault.c +1
-7@@ -440,13 +440,7 @@ void do_secure_storage_access(struct pt_regs *regs)if (is_kernel_fault (regs)) {if (is_vmalloc_addr ((void *)addr))return handle_fault_error_nolock (regs,0 ); - folio =virt_to_folio ((void *)addr); -if (unlikely (!folio_try_get (folio))) -return ; - rc =uv_convert_from_secure (folio_to_phys (folio)); -if (!rc) -clear_bit (PG_arch_1, &folio->flags.f); -folio_put (folio); + rc =uv_convert_from_secure (__pa (addr));/* * There are some valid fixup types for kernel * accesses to donated secure memory. zeropad is one
Re: [PATCH v4 5/8] s390/mm: Remove folio handling for kernel faults in do_secure_storage_access()
Quoted 11 lines · On Mon, 20 Jul 2026 12:53:29 +0200 Christian Borntraeger…
Am 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 10 lines
Remove folio handling for secure storage access exceptions that happen via the kernel mapping. Using
folio_try_get()for exceptions via the kernel mapping is racy: for example such an exception may happen on one CPU, while a different CPU frees the corresponding page. Subsequently the page can be part of a folio of different size, before the faulting CPU executesfolio_try_get(). This may lead to data corruption.As of now this scenario does not seem to be possible, however with the upcoming
load_unaligned_zeropad()this is possible.
Quoted 8 lines
Quoted 6 lines
As a side effect
PG_arch_1is not cleared anymore for such faults via the kernel mapping. However given that this bit is over-indicating it shouldn't matter.Signed-off-by Heiko Carstens <[email protected]>Ideally Claudio should confirm that
PG_arch_1is over-indicating.
PG_arch_1 is over-indicating.
Quoted 27 lines
Reviewed-by Christian Borntraeger <[email protected]>Quoted 23 lines
--- arch/s390/mm/fault.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)arch/s390/mm/fault.c +1
-7@@ -440,13 +440,7 @@ void do_secure_storage_access(struct pt_regs *regs)if (is_kernel_fault (regs)) {if (is_vmalloc_addr ((void *)addr))return handle_fault_error_nolock (regs,0 ); - folio =virt_to_folio ((void *)addr); -if (unlikely (!folio_try_get (folio))) -return ; - rc =uv_convert_from_secure (folio_to_phys (folio)); -if (!rc) -clear_bit (PG_arch_1, &folio->flags.f); -folio_put (folio); + rc =uv_convert_from_secure (__pa (addr));/* * There are some valid fixup types for kernel * accesses to donated secure memory. zeropad is one
Heiko Carstens
maintainerPATCH v4 6/8 s390/mm: Use handle_fault_error() in do_secure_storage_access()
In reply to Heiko CarstensSimilar to the normal exception handler use handle_fault_error() which handles accesses from user space and kernel space differently.
This also addresses failing kernel accesses via e.g. copy_from_user()to user space: instead of an endless loop, the fixup handling is performed. Even though this could only happen if arch_make_folio_accessible() would permanently fail, which is not realistic.
Therefore this is rather a cleanup patch.
--- arch/s390/mm/fault.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
arch/s390/mm/fault.c +3 -5
@@ -427,10 +427,8 @@ void do_secure_storage_access(struct pt_regs *regs)* was not supposed to do, e.g. branching into secure * memory. Trigger a segmentation fault. */ -if (user_mode (regs)) { -send_sig (SIGSEGV , current,0 ); -return ; - } +if (user_mode (regs)) +return handle_fault_error_nolock (regs,SEGV_ACCERR );/* * The kernel should never run into this case and * there is no way out of this situation.
@@ -465,7 +463,7 @@ void do_secure_storage_access(struct pt_regs *regs)folio_put (folio);folio_walk_end (&fw, vma);if (rc) -send_sig (SIGSEGV , current,0 ); +return handle_fault_error (regs,SEGV_ACCERR );mmap_read_unlock (mm); } }
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 6/8] s390/mm: Use handle_fault_error() in do_secure_storage_access()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 11 lines
Similar to the normal exception handler use
handle_fault_error()which handles accesses from user space and kernel space differently.This also addresses failing kernel accesses via e.g. copy_from_user()to user space: instead of an endless loop, the fixup handling is performed. Even though this could only happen if
arch_make_folio_accessible()would permanently fail, which is not realistic.Therefore this is rather a cleanup patch.
Signed-off-by Heiko Carstens <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Heiko Carstens
maintainerPATCH v4 7/8 s390/mm: Use goto statement in do_secure_storage_access()
In reply to Heiko CarstensMake do_secure_storage_access() look more like the do_exception() by using a goto statement to get rid of an extra open coded mmap_read_unlock() call.
Just cosmetic, no functional change.
--- arch/s390/mm/fault.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
arch/s390/mm/fault.c +3 -4
@@ -453,10 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)if (!vma)return handle_fault_error_nolock (regs,SEGV_MAPERR ); folio =folio_walk_start (&fw, vma, addr,0 ); -if (!folio) { -mmap_read_unlock (mm); -return ; - } +if (!folio) + goto out;/* arch_make_folio_accessible() needs a raised refcount. */ folio_get (folio); rc =arch_make_folio_accessible (folio);
@@ -464,6 +462,7 @@ void do_secure_storage_access(struct pt_regs *regs)folio_walk_end (&fw, vma);if (rc)return handle_fault_error (regs,SEGV_ACCERR ); +out:mmap_read_unlock (mm); } }
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 7/8] s390/mm: Use goto statement in do_secure_storage_access()
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Stale TLB / Premature Page Freeing in VM_SPARSE area teardown - [High] Invalid physical address…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stale TLB / Premature Page Freeing in VM_SPARSE area teardown
- [High] Invalid physical address translation for module addresses
--
--- Patch [7]: [PATCH v4 7/8] s390/mm: Use goto statement in do_secure_storage_access() --- commit e37b9a92584a0cc071d6b45d285a37b7eb5137cc
Author: Heiko Carstens <[email protected]>
s390/mm: Use goto statement in do_secure_storage_access()
This patch refactors do_secure_storage_access() to use a goto statement for unlocking mmap_read_unlock. It removes an open-coded unlock to make it look more like do_exception().
Quoted 5 lines
arch/s390/mm/fault.c +0
-0@@ -453,10 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)
[ ... ]
arch/s390/mm/fault.c void do_secure_storage_access(struct pt_regs *regs) · line 468
Quoted 7 lines
arch/s390/mm/fault.c +1
-0folio_walk_end (&fw, vma);if (rc)return handle_fault_error (regs,SEGV_ACCERR ); +out:mmap_read_unlock (mm); } }
While reviewing the changes in do_secure_storage_access(), I noticed a potential regression earlier in the same function:
arch/s390/mm/fault.c:do_secure_storage_access() {
...
if (is_vmalloc_addr((void *)addr))
return handle_fault_error_nolock(regs, 0);
rc = uv_convert_from_secure(__pa(addr));
...
}[Severity: High]
Does this incomplete vmalloc check allow module addresses to fall through to the invalid __pa() translation?
On s390, module memory resides outside the VMALLOC_START..VMALLOC_END range, so the is_vmalloc_addr() check returns false for module addresses. The code then falls through to uv_convert_from_secure(__pa(addr)).
Since module addresses are not direct-mapped, __pa(addr) generates a garbage physical address.
If this garbage address aligns with a valid physical page currently donated to another guest as secure memory, would the Ultravisor erroneously convert it, destroying the unrelated guest's secure state?
Should this check use is_vmalloc_or_module_addr() instead?
Also, looking at the VM_SPARSE area teardown sequence:
arch/s390/kernel/uv.c:uv_free_stor_var() {
...
apply_to_existing_page_range(&init_mm, addr, size, uv_free_range_cb, NULL);
free_vm_area(area);
...
}arch/s390/kernel/uv.c:uv_free_range_cb() {
...
__free_page(pte_page(*pte));
return 0;
...
}[Severity: High]
Can this sequence cause a stale TLB or premature page freeing during VM_SPARSE area teardown?
This sequence frees the physical pages to the buddy allocator while intentionally leaving the kernel PTEs valid. The PTEs are only cleared and the TLB flushed later by free_vm_area().
During this window, could any speculative or stray access to the vmalloc address access the physical page through the valid PTE/TLB, even though the page may have already been reallocated to another subsystem or guest, corrupting memory?
Should the teardown sequence clear the PTEs and flush the TLB before freeing the physical pages, as is normally done during standard vfree()?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
Re: [PATCH v4 7/8] s390/mm: Use goto statement in do_secure_storage_access()
In reply to Heiko CarstensAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 7 lines
Make
do_secure_storage_access()look more like thedo_exception()by using a goto statement to get rid of an extra open codedmmap_read_unlock()call.Just cosmetic, no functional change.
Signed-off-by Heiko Carstens <[email protected]>
Quoted 29 lines
--- arch/s390/mm/fault.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)arch/s390/mm/fault.c +3
-4@@ -453,10 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)if (!vma)return handle_fault_error_nolock (regs,SEGV_MAPERR ); folio =folio_walk_start (&fw, vma, addr,0 ); -if (!folio) { -mmap_read_unlock (mm); -return ; - } +if (!folio) + goto out;/* arch_make_folio_accessible() needs a raised refcount. */ folio_get (folio); rc =arch_make_folio_accessible (folio);@@ -464,6 +462,7 @@ void do_secure_storage_access(struct pt_regs *regs)folio_walk_end (&fw, vma);if (rc)return handle_fault_error (regs,SEGV_ACCERR ); +out:mmap_read_unlock (mm); } }
Heiko Carstens
maintainerPATCH v4 8/8 s390: Add support for DCACHE_WORD_ACCESS (again)
In reply to Heiko CarstensImplement load_unaligned_zeropad() and enable DCACHE_WORD_ACCESS to speed up string operations in fs/dcache.c and fs/namei.c.
With the secure storage access exception cases addressed, add support for DCACHE_WORD_ACCESS again.
--- arch/s390/Kconfig | 1 + arch/s390/include/asm/asm-extable.h | 4 ++++ arch/s390/include/asm/word-at-a-time.h | 22 ++++++++++++++++++++++ arch/s390/mm/extable.c | 18 ++++++++++++++++++ 4 files changed, 45 insertions(+)
arch/s390/Kconfig +1 -0
@@ -163,6 +163,7 @@ config S390 select ARCH_WANTS_THP_SWAP select BUILDTIME_TABLE_SORT select CLONE_BACKWARDS2 + select DCACHE_WORD_ACCESS if !KMSAN select DYNAMIC_FTRACE if FUNCTION_TRACER select FUNCTION_ALIGNMENT_8B if CC_IS_GCC select FUNCTION_ALIGNMENT_16B if !CC_IS_GCC
arch/s390/include/asm/asm-extable.h +4 -0
@@ -12,6 +12,7 @@#define EX_TYPE_UA_FAULT 3#define EX_TYPE_UA_LOAD_REG 5#define EX_TYPE_UA_LOAD_REGPAIR 6 +#define EX_TYPE_ZEROPAD 7#define EX_TYPE_FPC 8#define EX_TYPE_UA_MVCOS_TO 9#define EX_TYPE_UA_MVCOS_FROM 10
@@ -79,6 +80,9 @@#define EX_TABLE_UA_LOAD_REGPAIR (_fault, _target, _regerr, _regzero) \ __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_UA_LOAD_REGPAIR, _regerr, _regzero, 0) +#define EX_TABLE_ZEROPAD (_fault, _target, _regdata, _regaddr) \ + __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_ZEROPAD, _regdata, _regaddr, 0) +#define EX_TABLE_FPC (_fault, _target) \ __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_FPC, __stringify(%%r0), __stringify(%%r0), 0)
arch/s390/include/asm/word-at-a-time.h +22 -0
@@ -4,6 +4,7 @@#include <linux/bitops.h> #include <linux/wordpart.h> +#include <asm/asm-extable.h> #include <asm/bitsperlong.h> struct word_at_a_time {
@@ -40,4 +41,25 @@ static inline unsigned long zero_bytemask(unsigned long data) return ~1UL << data; } +/* +* Load an unaligned word from kernel space. +* +* In the (very unlikely) case of the word being a page-crosser +* and the next page not being mapped, take the exception and +* return zeroes in the non-existing part. +*/ +static inline unsigned long load_unaligned_zeropad (const void *addr) +{ +unsigned long data; + + asm_inline volatile( +"0: lg %[data],0(%[addr])\n" +"1: nopr %%r7\n" + EX_TABLE_ZEROPAD(0b, 1b, %[data], %[addr]) + EX_TABLE_ZEROPAD(1b, 1b, %[data], %[addr]) + : [data] "=d" (data) + : [addr] "a" (addr), "m" (*(unsigned long *)addr)); +return data; +} +#endif /* _ASM_WORD_AT_A_TIME_H */
arch/s390/mm/extable.c +18 -0
@@ -50,6 +50,22 @@ static bool ex_handler_ua_load_reg(const struct exception_table_entry *ex,return true; } +static bool ex_handler_zeropad (const struct exception_table_entry *ex,struct pt_regs *regs) +{ +unsigned int reg_addr =FIELD_GET (EX_DATA_REG_ADDR , ex->data); +unsigned int reg_data =FIELD_GET (EX_DATA_REG_ERR , ex->data); +unsigned long data, addr, offset; + + addr = regs->gprs[reg_addr]; + offset = addr & (sizeof (unsigned long ) -1 ); + addr &= ~(sizeof (unsigned long ) -1 ); + data = *(unsigned long *)addr; + data <<=BITS_PER_BYTE * offset; + regs->gprs[reg_data] = data; + regs->psw.addr =extable_fixup (ex); +return true; +} +static bool ex_handler_fpc (const struct exception_table_entry *ex,struct pt_regs *regs) {fpu_sfpc (0 );
@@ -118,6 +134,8 @@ bool fixup_exception(struct pt_regs *regs)return ex_handler_ua_load_reg (ex, false, regs);case EX_TYPE_UA_LOAD_REGPAIR :return ex_handler_ua_load_reg (ex, true, regs); +case EX_TYPE_ZEROPAD : +return ex_handler_zeropad (ex, regs);case EX_TYPE_FPC :return ex_handler_fpc (ex, regs);case EX_TYPE_UA_MVCOS_TO :
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v4 8/8] s390: Add support for DCACHE_WORD_ACCESS (again)
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines
Implement
load_unaligned_zeropad()and enableDCACHE_WORD_ACCESSto speed up string operations infs/dcache.candfs/namei.c.With the secure storage access exception cases addressed, add support for
DCACHE_WORD_ACCESSagain.Signed-off-by Heiko Carstens <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
Re: [PATCH v4 8/8] s390: Add support for DCACHE_WORD_ACCESS (again)
In reply to Heiko CarstensQuoted 115 lines · Heiko Carstens
Implement
load_unaligned_zeropad()and enableDCACHE_WORD_ACCESSto speed up string operations infs/dcache.candfs/namei.c.With the secure storage access exception cases addressed, add support for
DCACHE_WORD_ACCESSagain.Signed-off-by Heiko Carstens <[email protected]>--- arch/s390/Kconfig | 1 + arch/s390/include/asm/asm-extable.h | 4 ++++ arch/s390/include/asm/word-at-a-time.h | 22 ++++++++++++++++++++++ arch/s390/mm/extable.c | 18 ++++++++++++++++++ 4 files changed, 45 insertions(+)arch/s390/Kconfig +1
-0@@ -163,6 +163,7 @@ config S390 select ARCH_WANTS_THP_SWAP select BUILDTIME_TABLE_SORT select CLONE_BACKWARDS2 + select DCACHE_WORD_ACCESS if !KMSAN select DYNAMIC_FTRACE if FUNCTION_TRACER select FUNCTION_ALIGNMENT_8B if CC_IS_GCC select FUNCTION_ALIGNMENT_16B if !CC_IS_GCCarch/s390/include/asm/asm-extable.h +4
-0@@ -12,6 +12,7 @@#define EX_TYPE_UA_FAULT 3#define EX_TYPE_UA_LOAD_REG 5#define EX_TYPE_UA_LOAD_REGPAIR 6 +#define EX_TYPE_ZEROPAD 7#define EX_TYPE_FPC 8#define EX_TYPE_UA_MVCOS_TO 9#define EX_TYPE_UA_MVCOS_FROM 10@@ -79,6 +80,9 @@#define EX_TABLE_UA_LOAD_REGPAIR (_fault, _target, _regerr, _regzero) \ __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_UA_LOAD_REGPAIR, _regerr, _regzero, 0) +#define EX_TABLE_ZEROPAD (_fault, _target, _regdata, _regaddr) \ + __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_ZEROPAD, _regdata, _regaddr, 0) +#define EX_TABLE_FPC (_fault, _target) \ __EX_TABLE(__ex_table, _fault, _target, EX_TYPE_FPC, __stringify(%%r0), __stringify(%%r0), 0)arch/s390/include/asm/word-at-a-time.h +22
-0@@ -4,6 +4,7 @@#include <linux/bitops.h> #include <linux/wordpart.h> +#include <asm/asm-extable.h> #include <asm/bitsperlong.h> struct word_at_a_time {@@ -40,4 +41,25 @@ static inline unsigned long zero_bytemask(unsigned long data) return ~1UL << data; } +/* +* Load an unaligned word from kernel space. +* +* In the (very unlikely) case of the word being a page-crosser +* and the next page not being mapped, take the exception and +* return zeroes in the non-existing part. +*/ +static inline unsigned long load_unaligned_zeropad (const void *addr) +{ +unsigned long data; + + asm_inline volatile( +"0: lg %[data],0(%[addr])\n" +"1: nopr %%r7\n" + EX_TABLE_ZEROPAD(0b, 1b, %[data], %[addr]) + EX_TABLE_ZEROPAD(1b, 1b, %[data], %[addr]) + : [data] "=d" (data) + : [addr] "a" (addr), "m" (*(unsigned long *)addr)); +return data; +} +#endif /* _ASM_WORD_AT_A_TIME_H */ arch/s390/mm/extable.c +16
-0@@ -50,6 +50,22 @@ static bool ex_handler_ua_load_reg(const struct exception_table_entry *ex,return true; } +static bool ex_handler_zeropad (const struct exception_table_entry *ex,struct pt_regs *regs) +{ +unsigned int reg_addr =FIELD_GET (EX_DATA_REG_ADDR , ex->data); +unsigned int reg_data =FIELD_GET (EX_DATA_REG_ERR , ex->data); +unsigned long data, addr, offset; + + addr = regs->gprs[reg_addr]; + offset = addr & (sizeof (unsigned long ) -1 ); + addr &= ~(sizeof (unsigned long ) -1 ); + data = *(unsigned long *)addr; + data <<=BITS_PER_BYTE * offset; + regs->gprs[reg_data] = data; + regs->psw.addr =extable_fixup (ex); +return true; +} +static bool ex_handler_fpc (const struct exception_table_entry *ex,struct pt_regs *regs) {fpu_sfpc (0 );
arch/s390/mm/extable.c bool fixup_exception(struct pt_regs *regs) · line 141
Quoted 8 lines
arch/s390/mm/extable.c +2
-0@@ -118,6 +134,8 @@ bool fixup_exception(struct pt_regs *regs)return ex_handler_ua_load_reg (ex, false, regs);case EX_TYPE_UA_LOAD_REGPAIR :return ex_handler_ua_load_reg (ex, true, regs); +case EX_TYPE_ZEROPAD : +return ex_handler_zeropad (ex, regs);case EX_TYPE_FPC :return ex_handler_fpc (ex, regs);case EX_TYPE_UA_MVCOS_TO :
Christian Borntraeger
maintainerAm 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 5 lines
v4:
- Add two more cleanup patches based on Sashiko feedback, however I don't consider them as bug fixes. This is more cosmetics:
- Usehandle_fault_error()indo_secure_storage_access()
- Use goto statement indo_secure_storage_access()
Shall we run the kmemleak fix (for the initial memblock) and maybe additional kmemleak changes for the other secure memory arady on top of your patch set or shall I run them independently?
Heiko Carstens
maintainerQuoted 9 lines · Christian Borntraeger
Am 20.07.26 um 10:58 schrieb Heiko Carstens:
Quoted 5 lines
v4:
- Add two more cleanup patches based on Sashiko feedback, however I don't consider them as bug fixes. This is more cosmetics:
- Usehandle_fault_error()indo_secure_storage_access()
- Use goto statement indo_secure_storage_access()Shall we run the kmemleak fix (for the initial memblock) and maybe additional kmemleak changes for the other secure memory arady on top of your patch set or shall I run them independently?
Please handle that independently of this series.
After reading through all the Sashiko reports, at least (Sashiko) AI review is now complete. All current findings are either false positives, describe intended behavior, or are addressed with later patches in this series.
Now waiting for human review :)
Quoted 48 lines · Heiko Carstens
v4:
- Add two more cleanup patches based on Sashiko feedback, however I don't consider them as bug fixes. This is more cosmetics:
- Usehandle_fault_error()indo_secure_storage_access()
- Use goto statement indo_secure_storage_access()v3:
- Fix two more pre-existing bugs reported by Sashiko
- Add Christian's Tested-by tag to four of the six patchesv2:
- ExplicitelyPAGE_ALIGNsize inuv_alloc_stor_var()[Sashiko]
- Document that lazy PTE update / TLB flushing is intended [Sashiko]v1:
Support forDCACHE_WORD_ACCESSwas recently removed [1] since it caused problems with the incomplete handling of secure storage access exceptions. It looked like fixing the exception handler would be a larger effort; therefore support forDCACHE_WORD_ACCESSwas removed as a work around.Address the potential problems that exist with secure storage access exceptions and add support for
DCACHE_WORD_ACCESSagain.In particular address the following problems:
- Reading the guest variable storage area via the
/proc/kcoreinterface results in short reads. Address this by using aVM_SPARSEarea for the guest variable storage area.VM_SPARSEareas will be handled like memory holes (zeros will be read).- Fix handling of secure storage access exceptions in vmalloc area.
- Remove folio handling for secure storage access exceptions to avoid potential data corruption.
[1]
37540b8c287f("s390: Revert support forDCACHE_WORD_ACCESS")Heiko Carstens (8):
KVM: s390: pv: UseVM_SPARSEarea for guest variable storage area s390/mm: Add missing mm check todo_secure_storage_access()s390/mm: Uselock_mm_and_find_vma()indo_secure_storage_access()s390/mm: Fix handling of vmalloc area indo_secure_storage_access()s390/mm: Remove folio handling for kernel faults indo_secure_storage_access()s390/mm: Usehandle_fault_error()indo_secure_storage_access()s390/mm: Use goto statement indo_secure_storage_access()
s390: Add support forDCACHE_WORD_ACCESS(again)
Quoted 9 lines
arch/s390/Kconfig | 1 + arch/s390/include/asm/asm-extable.h | 4 ++ arch/s390/include/asm/uv.h | 2 + arch/s390/include/asm/word-at-a-time.h | 22 +++++++++ arch/s390/kernel/uv.c | 65 ++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 6 +-- arch/s390/mm/extable.c | 18 +++++++ arch/s390/mm/fault.c | 35 ++++++-------- 8 files changed, 128 insertions(+), 25 deletions(-)
Applied, thank you!