* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Darrick J. Wong @ 2025-04-21 4:00 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415121425.4146847-12-john.g.garry@oracle.com>
On Tue, Apr 15, 2025 at 12:14:22PM +0000, John Garry wrote:
> Add xfs_file_dio_write_atomic() for dedicated handling of atomic writes.
>
> The function works based on two operating modes:
> - HW offload, i.e. REQ_ATOMIC-based
> - CoW based with out-of-places write and atomic extent remapping
>
> The preferred method is HW offload as it will be faster. If HW offload is
> not possible, then we fallback to the CoW-based method.
>
> HW offload would not be possible for the write length exceeding the HW
> offload limit, the write spanning multiple extents, unaligned disk blocks,
> etc.
>
> Apart from the write exceeding the HW offload limit, other conditions for
> HW offload can only be detected in the iomap handling for the write. As
> such, we use a fallback method to issue the write if we detect in the
> ->iomap_begin() handler that HW offload is not possible. Special code
> -ENOPROTOOPT is returned from ->iomap_begin() to inform that HW offload
> not possible.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> fs/xfs/xfs_file.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ba4b02abc6e4..81a377f65aa3 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -728,6 +728,72 @@ xfs_file_dio_write_zoned(
> return ret;
> }
>
> +/*
> + * Handle block atomic writes
> + *
> + * Two methods of atomic writes are supported:
> + * - REQ_ATOMIC-based, which would typically use some form of HW offload in the
> + * disk
> + * - COW-based, which uses a COW fork as a staging extent for data updates
> + * before atomically updating extent mappings for the range being written
> + *
> + */
> +static noinline ssize_t
> +xfs_file_dio_write_atomic(
> + struct xfs_inode *ip,
> + struct kiocb *iocb,
> + struct iov_iter *from)
> +{
> + unsigned int iolock = XFS_IOLOCK_SHARED;
> + ssize_t ret, ocount = iov_iter_count(from);
> + const struct iomap_ops *dops;
> +
> + /*
> + * HW offload should be faster, so try that first if it is already
> + * known that the write length is not too large.
> + */
> + if (ocount > xfs_inode_buftarg(ip)->bt_bdev_awu_max)
> + dops = &xfs_atomic_write_cow_iomap_ops;
> + else
> + dops = &xfs_direct_write_iomap_ops;
> +
> +retry:
> + ret = xfs_ilock_iocb_for_write(iocb, &iolock);
> + if (ret)
> + return ret;
> +
> + ret = xfs_file_write_checks(iocb, from, &iolock, NULL);
> + if (ret)
> + goto out_unlock;
> +
> + /* Demote similar to xfs_file_dio_write_aligned() */
> + if (iolock == XFS_IOLOCK_EXCL) {
> + xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> + iolock = XFS_IOLOCK_SHARED;
> + }
> +
> + trace_xfs_file_direct_write(iocb, from);
> + ret = iomap_dio_rw(iocb, from, dops, &xfs_dio_write_ops,
> + 0, NULL, 0);
> +
> + /*
> + * The retry mechanism is based on the ->iomap_begin method returning
> + * -ENOPROTOOPT, which would be when the REQ_ATOMIC-based write is not
> + * possible. The REQ_ATOMIC-based method typically not be possible if
> + * the write spans multiple extents or the disk blocks are misaligned.
> + */
> + if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
> + xfs_iunlock(ip, iolock);
> + dops = &xfs_atomic_write_cow_iomap_ops;
> + goto retry;
> + }
> +
> +out_unlock:
> + if (iolock)
> + xfs_iunlock(ip, iolock);
> + return ret;
> +}
> +
> /*
> * Handle block unaligned direct I/O writes
> *
> @@ -843,6 +909,8 @@ xfs_file_dio_write(
> return xfs_file_dio_write_unaligned(ip, iocb, from);
> if (xfs_is_zoned_inode(ip))
> return xfs_file_dio_write_zoned(ip, iocb, from);
What happens to an IOCB_ATOMIC write to a zoned file? I think the
ioend for an atomic write to a zoned file involves a similar change as
an outofplace atomic write to a file (one big transaction to absorb
all the mapping changes) but I don't think the zoned code quite does
that...?
--D
> + if (iocb->ki_flags & IOCB_ATOMIC)
> + return xfs_file_dio_write_atomic(ip, iocb, from);
> return xfs_file_dio_write_aligned(ip, iocb, from,
> &xfs_direct_write_iomap_ops, &xfs_dio_write_ops, NULL);
> }
> --
> 2.31.1
>
>
^ permalink raw reply
* [PATCH] man/man2/clone.2: Document CLONE_NEWPID and CLONE_NEWUSER flag
From: devhoodit @ 2025-04-20 19:16 UTC (permalink / raw)
To: alx; +Cc: linux-man, linux-api, devhoodit, Carlos O'Donell,
Andrew Morton
CLONE_NEWPID and CLONE_PARENT can be used together, but not CLONE_THREAD. Similarly, CLONE_NEWUSER and CLONE_PARENT can be used together, but not CLONE_THREAD.
This was discussed here: <https://lore.kernel.org/linux-man/06febfb3-e2e2-4363-bc34-83a07692144f@redhat.com/T/>
Relevant code: <https://github.com/torvalds/linux/blob/219d54332a09e8d8741c1e1982f5eae56099de85/kernel/fork.c#L1815>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: devhoodit <devhoodit@gmail.com>
---
man/man2/clone.2 | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/man/man2/clone.2 b/man/man2/clone.2
index 1b74e4c92..b9561125a 100644
--- a/man/man2/clone.2
+++ b/man/man2/clone.2
@@ -776,9 +776,7 @@ .SS The flags mask
no privileges are needed to create a user namespace.
.IP
This flag can't be specified in conjunction with
-.B CLONE_THREAD
-or
-.BR CLONE_PARENT .
+.BR CLONE_THREAD .
For security reasons,
.\" commit e66eded8309ebf679d3d3c1f5820d1f2ca332c71
.\" https://lwn.net/Articles/543273/
@@ -1319,11 +1317,10 @@ .SH ERRORS
mask.
.TP
.B EINVAL
+Both
.B CLONE_NEWPID
-and one (or both) of
+and
.B CLONE_THREAD
-or
-.B CLONE_PARENT
were specified in the
.I flags
mask.
--
2.49.0
^ permalink raw reply related
* Re: [PATCH v3 13/20] mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
From: Kairui Song @ 2025-04-19 16:35 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <da399be3-4219-4ccf-a41d-9db7e1e45c14@redhat.com>
On Sun, Apr 20, 2025 at 12:32 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.04.25 18:25, David Hildenbrand wrote:
> > On 19.04.25 18:02, Kairui Song wrote:
> >> On Tue, Mar 4, 2025 at 12:46 AM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> Currently, we never end up reusing PTE-mapped THPs after fork. This
> >>> wasn't really a problem with PMD-sized THPs, because they would have to
> >>> be PTE-mapped first, but it's getting a problem with smaller THP
> >>> sizes that are effectively always PTE-mapped.
> >>>
> >>> With our new "mapped exclusively" vs "maybe mapped shared" logic for
> >>> large folios, implementing CoW reuse for PTE-mapped THPs is straight
> >>> forward: if exclusively mapped, make sure that all references are
> >>> from these (our) mappings. Add some helpful comments to explain the
> >>> details.
> >>>
> >>> CONFIG_TRANSPARENT_HUGEPAGE selects CONFIG_MM_ID. If we spot an anon
> >>> large folio without CONFIG_TRANSPARENT_HUGEPAGE in that code, something
> >>> is seriously messed up.
> >>>
> >>> There are plenty of things we can optimize in the future: For example, we
> >>> could remember that the folio is fully exclusive so we could speedup
> >>> the next fault further. Also, we could try "faulting around", turning
> >>> surrounding PTEs that map the same folio writable. But especially the
> >>> latter might increase COW latency, so it would need further
> >>> investigation.
> >>>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>> mm/memory.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++------
> >>> 1 file changed, 75 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/mm/memory.c b/mm/memory.c
> >>> index 73b783c7d7d51..bb245a8fe04bc 100644
> >>> --- a/mm/memory.c
> >>> +++ b/mm/memory.c
> >>> @@ -3729,19 +3729,86 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
> >>> return ret;
> >>> }
> >>>
> >>> -static bool wp_can_reuse_anon_folio(struct folio *folio,
> >>> - struct vm_area_struct *vma)
> >>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>> +static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
> >>> + struct vm_area_struct *vma)
> >>> {
> >>> + bool exclusive = false;
> >>> +
> >>> + /* Let's just free up a large folio if only a single page is mapped. */
> >>> + if (folio_large_mapcount(folio) <= 1)
> >>> + return false;
> >>> +
> >>> /*
> >>> - * We could currently only reuse a subpage of a large folio if no
> >>> - * other subpages of the large folios are still mapped. However,
> >>> - * let's just consistently not reuse subpages even if we could
> >>> - * reuse in that scenario, and give back a large folio a bit
> >>> - * sooner.
> >>> + * The assumption for anonymous folios is that each page can only get
> >>> + * mapped once into each MM. The only exception are KSM folios, which
> >>> + * are always small.
> >>> + *
> >>> + * Each taken mapcount must be paired with exactly one taken reference,
> >>> + * whereby the refcount must be incremented before the mapcount when
> >>> + * mapping a page, and the refcount must be decremented after the
> >>> + * mapcount when unmapping a page.
> >>> + *
> >>> + * If all folio references are from mappings, and all mappings are in
> >>> + * the page tables of this MM, then this folio is exclusive to this MM.
> >>> */
> >>> - if (folio_test_large(folio))
> >>> + if (folio_test_large_maybe_mapped_shared(folio))
> >>> + return false;
> >>> +
> >>> + VM_WARN_ON_ONCE(folio_test_ksm(folio));
> >>> + VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
> >>> + VM_WARN_ON_ONCE(folio_entire_mapcount(folio));
> >>> +
> >>> + if (unlikely(folio_test_swapcache(folio))) {
> >>> + /*
> >>> + * Note: freeing up the swapcache will fail if some PTEs are
> >>> + * still swap entries.
> >>> + */
> >>> + if (!folio_trylock(folio))
> >>> + return false;
> >>> + folio_free_swap(folio);
> >>> + folio_unlock(folio);
> >>> + }
> >>> +
> >>> + if (folio_large_mapcount(folio) != folio_ref_count(folio))
> >>> return false;
> >>>
> >>> + /* Stabilize the mapcount vs. refcount and recheck. */
> >>> + folio_lock_large_mapcount(folio);
> >>> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
> >>
> >> Hi David, I'm seeing this WARN_ON being triggered on my test machine:
> >
> > Hi!
> >
> > So I assume the following will not sort out the issue for you, correct?
> >
> > https://lore.kernel.org/all/20250415095007.569836-1-david@redhat.com/T/#u
> >
> >>
> >> I'm currently working on my swap table series and testing heavily with
> >> swap related workloads. I thought my patch may break the kernel, but
> >> after more investigation and reverting to current mm-unstable, it
> >> still occurs (with a much lower chance though, I think my series
> >> changed the timing so it's more frequent in my case).
> >>
> >> The test is simple, I just enable all mTHP sizes and repeatedly build
> >> linux kernel in a 1G memcg using tmpfs.
> >>
> >> The WARN is reproducible with current mm-unstable
> >> (dc683247117ee018e5da6b04f1c499acdc2a1418):
> >>
> >> [ 5268.100379] ------------[ cut here ]------------
> >> [ 5268.105925] WARNING: CPU: 2 PID: 700274 at mm/memory.c:3792
> >> do_wp_page+0xfc5/0x1080
> >> [ 5268.112437] Modules linked in: zram virtiofs
> >> [ 5268.115507] CPU: 2 UID: 0 PID: 700274 Comm: cc1 Kdump: loaded Not
> >> tainted 6.15.0-rc2.ptch-gdc683247117e #1434 PREEMPT(voluntary)
> >> [ 5268.120562] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> >> [ 5268.123025] RIP: 0010:do_wp_page+0xfc5/0x1080
> >> [ 5268.124807] Code: 0d 80 77 32 02 0f 85 3e f1 ff ff 0f 1f 44 00 00
> >> e9 34 f1 ff ff 48 0f ba 75 00 1f 65 ff 0d 63 77 32 02 0f 85 21 f1 ff
> >> ff eb e1 <0f> 0b e9 10 fd ff ff 65 ff 00 f0 48 0f b
> >> a 6d 00 1f 0f 83 ec fc ff
> >> [ 5268.132034] RSP: 0000:ffffc900234efd48 EFLAGS: 00010297
> >> [ 5268.134002] RAX: 0000000000000080 RBX: 0000000000000000 RCX: 000fffffffe00000
> >> [ 5268.136609] RDX: 0000000000000081 RSI: 00007f009cbad000 RDI: ffffea0012da0000
> >> [ 5268.139371] RBP: ffffea0012da0068 R08: 80000004b682d025 R09: 00007f009c7c0000
> >> [ 5268.142183] R10: ffff88839c48b8c0 R11: 0000000000000000 R12: ffff88839c48b8c0
> >> [ 5268.144738] R13: ffffea0012da0000 R14: 00007f009cbadf10 R15: ffffc900234efdd8
> >> [ 5268.147540] FS: 00007f009d1fdac0(0000) GS:ffff88a07ae14000(0000)
> >> knlGS:0000000000000000
> >> [ 5268.150715] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 5268.153270] CR2: 00007f009cbadf10 CR3: 000000016c7c0001 CR4: 0000000000770eb0
> >> [ 5268.155674] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> [ 5268.158100] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> [ 5268.160613] PKRU: 55555554
> >> [ 5268.161662] Call Trace:
> >> [ 5268.162609] <TASK>
> >> [ 5268.163438] ? ___pte_offset_map+0x1b/0x110
> >> [ 5268.165309] __handle_mm_fault+0xa51/0xf00
> >> [ 5268.166848] ? update_load_avg+0x80/0x760
> >> [ 5268.168376] handle_mm_fault+0x13d/0x360
> >> [ 5268.169930] do_user_addr_fault+0x2f2/0x7f0
> >> [ 5268.171630] exc_page_fault+0x6a/0x140
> >> [ 5268.173278] asm_exc_page_fault+0x26/0x30
> >> [ 5268.174866] RIP: 0033:0x120e8e4
> >> [ 5268.176272] Code: 84 a9 00 00 00 48 39 c3 0f 85 ae 00 00 00 48 8b
> >> 43 20 48 89 45 38 48 85 c0 0f 85 b7 00 00 00 48 8b 43 18 48 8b 15 6c
> >> 08 42 01 <0f> 11 43 10 48 89 1d 61 08 42 01 48 89 53 18 0f 11 03 0f 11
> >> 43 20
> >> [ 5268.184121] RSP: 002b:00007fff8a855160 EFLAGS: 00010246
> >> [ 5268.186343] RAX: 00007f009cbadbd0 RBX: 00007f009cbadf00 RCX: 0000000000000000
> >> [ 5268.189209] RDX: 00007f009cbba030 RSI: 00000000000006f4 RDI: 0000000000000000
> >> [ 5268.192145] RBP: 00007f009cbb6460 R08: 00007f009d10f000 R09: 000000000000016c
> >> [ 5268.194687] R10: 0000000000000000 R11: 0000000000000010 R12: 00007f009cf97660
> >> [ 5268.197172] R13: 00007f009756ede0 R14: 00007f0097582348 R15: 0000000000000002
> >> [ 5268.199419] </TASK>
> >> [ 5268.200227] ---[ end trace 0000000000000000 ]---
> >>
> >> I also once changed the WARN_ON to WARN_ON_FOLIO and I got more info here:
> >>
> >> [ 3994.907255] page: refcount:9 mapcount:1 mapping:0000000000000000
> >> index:0x7f90b3e98 pfn:0x615028
> >> [ 3994.914449] head: order:3 mapcount:8 entire_mapcount:0
> >> nr_pages_mapped:8 pincount:0
> >> [ 3994.924534] memcg:ffff888106746000
> >> [ 3994.927868] anon flags:
> >> 0x17ffffc002084c(referenced|uptodate|owner_2|head|swapbacked|node=0|zone=2|lastcpupid=0x1fffff)
> >> [ 3994.933479] raw: 0017ffffc002084c ffff88816edd9128 ffffea000beac108
> >> ffff8882e8ba6bc9
> >> [ 3994.936251] raw: 00000007f90b3e98 0000000000000000 0000000900000000
> >> ffff888106746000
> >> [ 3994.939466] head: 0017ffffc002084c ffff88816edd9128
> >> ffffea000beac108 ffff8882e8ba6bc9
> >> [ 3994.943355] head: 00000007f90b3e98 0000000000000000
> >> 0000000900000000 ffff888106746000
> >> [ 3994.946988] head: 0017ffffc0000203 ffffea0018540a01
> >> 0000000800000007 00000000ffffffff
> >> [ 3994.950328] head: ffffffff00000007 00000000800000a3
> >> 0000000000000000 0000000000000008
> >> [ 3994.953684] page dumped because:
> >> VM_WARN_ON_FOLIO(folio_large_mapcount(folio) < folio_ref_count(folio))
> >> [ 3994.957534] ------------[ cut here ]------------
> >> [ 3994.959917] WARNING: CPU: 16 PID: 555282 at mm/memory.c:3794
> >> do_wp_page+0x10c0/0x1110
> >> [ 3994.963069] Modules linked in: zram virtiofs
> >> [ 3994.964726] CPU: 16 UID: 0 PID: 555282 Comm: sh Kdump: loaded Not
> >> tainted 6.15.0-rc1.ptch-ge39aef85f4c0-dirty #1431 PREEMPT(voluntary)
> >> [ 3994.969985] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> >> [ 3994.972905] RIP: 0010:do_wp_page+0x10c0/0x1110
> >> [ 3994.974477] Code: fe ff 0f 0b bd f5 ff ff ff e9 16 fb ff ff 41 83
> >> a9 bc 12 00 00 01 e9 2f fb ff ff 48 c7 c6 90 c2 49 82 4c 89 ef e8 40
> >> fd fe ff <0f> 0b e9 6a fc ff ff 65 ff 00 f0 48 0f b
> >> a 6d 00 1f 0f 83 46 fc ff
> >> [ 3994.981033] RSP: 0000:ffffc9002b3c7d40 EFLAGS: 00010246
> >> [ 3994.982636] RAX: 000000000000005b RBX: 0000000000000000 RCX: 0000000000000000
> >> [ 3994.984778] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff889ffea16a80
> >> [ 3994.986865] RBP: ffffea0018540a68 R08: 0000000000000000 R09: c0000000ffff7fff
> >> [ 3994.989316] R10: 0000000000000001 R11: ffffc9002b3c7b80 R12: ffff88810cfd7d40
> >> [ 3994.991654] R13: ffffea0018540a00 R14: 00007f90b3e9d620 R15: ffffc9002b3c7dd8
> >> [ 3994.994076] FS: 00007f90b3caa740(0000) GS:ffff88a07b194000(0000)
> >> knlGS:0000000000000000
> >> [ 3994.996939] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 3994.998902] CR2: 00007f90b3e9d620 CR3: 0000000104088004 CR4: 0000000000770eb0
> >> [ 3995.001314] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> [ 3995.003746] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> [ 3995.006173] PKRU: 55555554
> >> [ 3995.007117] Call Trace:
> >> [ 3995.007988] <TASK>
> >> [ 3995.008755] ? __pfx_default_wake_function+0x10/0x10
> >> [ 3995.010490] ? ___pte_offset_map+0x1b/0x110
> >> [ 3995.011929] __handle_mm_fault+0xa51/0xf00
> >> [ 3995.013346] handle_mm_fault+0x13d/0x360
> >> [ 3995.014796] do_user_addr_fault+0x2f2/0x7f0
> >> [ 3995.016331] ? sigprocmask+0x77/0xa0
> >> [ 3995.017656] exc_page_fault+0x6a/0x140
> >> [ 3995.018978] asm_exc_page_fault+0x26/0x30
> >> [ 3995.020309] RIP: 0033:0x7f90b3d881a7
> >> [ 3995.021461] Code: e8 4e b1 f8 ff 66 66 2e 0f 1f 84 00 00 00 00 00
> >> 0f 1f 00 f3 0f 1e fa 55 31 c0 ba 01 00 00 00 48 89 e5 53 48 89 fb 48
> >> 83 ec 08 <f0> 0f b1 15 71 54 11 00 0f 85 3b 01 00 0
> >> 0 48 8b 35 84 54 11 00 48
> >> [ 3995.028091] RSP: 002b:00007ffc33632c90 EFLAGS: 00010206
> >> [ 3995.029992] RAX: 0000000000000000 RBX: 0000560cfbfc0a40 RCX: 0000000000000000
> >> [ 3995.032456] RDX: 0000000000000001 RSI: 0000000000000005 RDI: 0000560cfbfc0a40
> >> [ 3995.034794] RBP: 00007ffc33632ca0 R08: 00007ffc33632d50 R09: 00007ffc33632cff
> >> [ 3995.037534] R10: 00007ffc33632c70 R11: 00007ffc33632d00 R12: 0000560cfbfc0a40
> >> [ 3995.041063] R13: 00007f90b3e97fd0 R14: 00007f90b3e97fa8 R15: 0000000000000000
> >> [ 3995.044390] </TASK>
> >> [ 3995.045510] ---[ end trace 0000000000000000 ]---
> >>
> >> My guess is folio_ref_count is not a reliable thing to check here,
> >> anything can increase the folio's ref account even without locking it,
> >> for example, a swap cache lookup or maybe anything iterating the LRU.
> >
> > It is reliable, we are holding the mapcount lock, so for each mapcount
> > we must have a corresponding refcount. If that is not the case, we have
> > an issue elsewhere.
> >
> > Other reference may only increase the refcount, but not violate the
> > mapcount vs. refcount condition.
> >
> > Can you reproduce also with swap disabled?
>
> Oh, re-reading the condition 3 times, I realize that the sanity check is wrong ...
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 037b6ce211f1f..a17eeef3f1f89 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3789,7 +3789,7 @@ static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
>
> /* Stabilize the mapcount vs. refcount and recheck. */
> folio_lock_large_mapcount(folio);
> - VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) > folio_ref_count(folio));
Ah, now it makes sense to me now :)
Thanks for the quick response.
>
> if (folio_test_large_maybe_mapped_shared(folio))
> goto unlock;
>
> Our refcount must be at least the mapcount, that's what we want to assert.
>
> Can you test and send a fix patch if that makes it fly for you?
Sure I'll keep the testing, I think it will just fix it, I have a few
WARN_ON_FOLIO reports all reporting mapcount is smaller than refcount.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v3 13/20] mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
From: Kairui Song @ 2025-04-19 16:33 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <c7e85336-5e34-4dd9-950f-173f48ff0be1@redhat.com>
On Sun, Apr 20, 2025 at 12:26 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.04.25 18:02, Kairui Song wrote:
> > On Tue, Mar 4, 2025 at 12:46 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> Currently, we never end up reusing PTE-mapped THPs after fork. This
> >> wasn't really a problem with PMD-sized THPs, because they would have to
> >> be PTE-mapped first, but it's getting a problem with smaller THP
> >> sizes that are effectively always PTE-mapped.
> >>
> >> With our new "mapped exclusively" vs "maybe mapped shared" logic for
> >> large folios, implementing CoW reuse for PTE-mapped THPs is straight
> >> forward: if exclusively mapped, make sure that all references are
> >> from these (our) mappings. Add some helpful comments to explain the
> >> details.
> >>
> >> CONFIG_TRANSPARENT_HUGEPAGE selects CONFIG_MM_ID. If we spot an anon
> >> large folio without CONFIG_TRANSPARENT_HUGEPAGE in that code, something
> >> is seriously messed up.
> >>
> >> There are plenty of things we can optimize in the future: For example, we
> >> could remember that the folio is fully exclusive so we could speedup
> >> the next fault further. Also, we could try "faulting around", turning
> >> surrounding PTEs that map the same folio writable. But especially the
> >> latter might increase COW latency, so it would need further
> >> investigation.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> mm/memory.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 75 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 73b783c7d7d51..bb245a8fe04bc 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -3729,19 +3729,86 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
> >> return ret;
> >> }
> >>
> >> -static bool wp_can_reuse_anon_folio(struct folio *folio,
> >> - struct vm_area_struct *vma)
> >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >> +static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
> >> + struct vm_area_struct *vma)
> >> {
> >> + bool exclusive = false;
> >> +
> >> + /* Let's just free up a large folio if only a single page is mapped. */
> >> + if (folio_large_mapcount(folio) <= 1)
> >> + return false;
> >> +
> >> /*
> >> - * We could currently only reuse a subpage of a large folio if no
> >> - * other subpages of the large folios are still mapped. However,
> >> - * let's just consistently not reuse subpages even if we could
> >> - * reuse in that scenario, and give back a large folio a bit
> >> - * sooner.
> >> + * The assumption for anonymous folios is that each page can only get
> >> + * mapped once into each MM. The only exception are KSM folios, which
> >> + * are always small.
> >> + *
> >> + * Each taken mapcount must be paired with exactly one taken reference,
> >> + * whereby the refcount must be incremented before the mapcount when
> >> + * mapping a page, and the refcount must be decremented after the
> >> + * mapcount when unmapping a page.
> >> + *
> >> + * If all folio references are from mappings, and all mappings are in
> >> + * the page tables of this MM, then this folio is exclusive to this MM.
> >> */
> >> - if (folio_test_large(folio))
> >> + if (folio_test_large_maybe_mapped_shared(folio))
> >> + return false;
> >> +
> >> + VM_WARN_ON_ONCE(folio_test_ksm(folio));
> >> + VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
> >> + VM_WARN_ON_ONCE(folio_entire_mapcount(folio));
> >> +
> >> + if (unlikely(folio_test_swapcache(folio))) {
> >> + /*
> >> + * Note: freeing up the swapcache will fail if some PTEs are
> >> + * still swap entries.
> >> + */
> >> + if (!folio_trylock(folio))
> >> + return false;
> >> + folio_free_swap(folio);
> >> + folio_unlock(folio);
> >> + }
> >> +
> >> + if (folio_large_mapcount(folio) != folio_ref_count(folio))
> >> return false;
> >>
> >> + /* Stabilize the mapcount vs. refcount and recheck. */
> >> + folio_lock_large_mapcount(folio);
> >> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
> >
> > Hi David, I'm seeing this WARN_ON being triggered on my test machine:
>
> Hi!
>
> So I assume the following will not sort out the issue for you, correct?
>
> https://lore.kernel.org/all/20250415095007.569836-1-david@redhat.com/T/#u
Yes, double checked the commit I'm testing
dc683247117ee018e5da6b04f1c499acdc2a1418 (akpm/mm-unstable) includes
this fix.
>
> >
> > I'm currently working on my swap table series and testing heavily with
> > swap related workloads. I thought my patch may break the kernel, but
> > after more investigation and reverting to current mm-unstable, it
> > still occurs (with a much lower chance though, I think my series
> > changed the timing so it's more frequent in my case).
> >
> > The test is simple, I just enable all mTHP sizes and repeatedly build
> > linux kernel in a 1G memcg using tmpfs.
> >
> > The WARN is reproducible with current mm-unstable
> > (dc683247117ee018e5da6b04f1c499acdc2a1418):
> >
> > [ 5268.100379] ------------[ cut here ]------------
> > [ 5268.105925] WARNING: CPU: 2 PID: 700274 at mm/memory.c:3792
> > do_wp_page+0xfc5/0x1080
> > [ 5268.112437] Modules linked in: zram virtiofs
> > [ 5268.115507] CPU: 2 UID: 0 PID: 700274 Comm: cc1 Kdump: loaded Not
> > tainted 6.15.0-rc2.ptch-gdc683247117e #1434 PREEMPT(voluntary)
> > [ 5268.120562] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> > [ 5268.123025] RIP: 0010:do_wp_page+0xfc5/0x1080
> > [ 5268.124807] Code: 0d 80 77 32 02 0f 85 3e f1 ff ff 0f 1f 44 00 00
> > e9 34 f1 ff ff 48 0f ba 75 00 1f 65 ff 0d 63 77 32 02 0f 85 21 f1 ff
> > ff eb e1 <0f> 0b e9 10 fd ff ff 65 ff 00 f0 48 0f b
> > a 6d 00 1f 0f 83 ec fc ff
> > [ 5268.132034] RSP: 0000:ffffc900234efd48 EFLAGS: 00010297
> > [ 5268.134002] RAX: 0000000000000080 RBX: 0000000000000000 RCX: 000fffffffe00000
> > [ 5268.136609] RDX: 0000000000000081 RSI: 00007f009cbad000 RDI: ffffea0012da0000
> > [ 5268.139371] RBP: ffffea0012da0068 R08: 80000004b682d025 R09: 00007f009c7c0000
> > [ 5268.142183] R10: ffff88839c48b8c0 R11: 0000000000000000 R12: ffff88839c48b8c0
> > [ 5268.144738] R13: ffffea0012da0000 R14: 00007f009cbadf10 R15: ffffc900234efdd8
> > [ 5268.147540] FS: 00007f009d1fdac0(0000) GS:ffff88a07ae14000(0000)
> > knlGS:0000000000000000
> > [ 5268.150715] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 5268.153270] CR2: 00007f009cbadf10 CR3: 000000016c7c0001 CR4: 0000000000770eb0
> > [ 5268.155674] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 5268.158100] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 5268.160613] PKRU: 55555554
> > [ 5268.161662] Call Trace:
> > [ 5268.162609] <TASK>
> > [ 5268.163438] ? ___pte_offset_map+0x1b/0x110
> > [ 5268.165309] __handle_mm_fault+0xa51/0xf00
> > [ 5268.166848] ? update_load_avg+0x80/0x760
> > [ 5268.168376] handle_mm_fault+0x13d/0x360
> > [ 5268.169930] do_user_addr_fault+0x2f2/0x7f0
> > [ 5268.171630] exc_page_fault+0x6a/0x140
> > [ 5268.173278] asm_exc_page_fault+0x26/0x30
> > [ 5268.174866] RIP: 0033:0x120e8e4
> > [ 5268.176272] Code: 84 a9 00 00 00 48 39 c3 0f 85 ae 00 00 00 48 8b
> > 43 20 48 89 45 38 48 85 c0 0f 85 b7 00 00 00 48 8b 43 18 48 8b 15 6c
> > 08 42 01 <0f> 11 43 10 48 89 1d 61 08 42 01 48 89 53 18 0f 11 03 0f 11
> > 43 20
> > [ 5268.184121] RSP: 002b:00007fff8a855160 EFLAGS: 00010246
> > [ 5268.186343] RAX: 00007f009cbadbd0 RBX: 00007f009cbadf00 RCX: 0000000000000000
> > [ 5268.189209] RDX: 00007f009cbba030 RSI: 00000000000006f4 RDI: 0000000000000000
> > [ 5268.192145] RBP: 00007f009cbb6460 R08: 00007f009d10f000 R09: 000000000000016c
> > [ 5268.194687] R10: 0000000000000000 R11: 0000000000000010 R12: 00007f009cf97660
> > [ 5268.197172] R13: 00007f009756ede0 R14: 00007f0097582348 R15: 0000000000000002
> > [ 5268.199419] </TASK>
> > [ 5268.200227] ---[ end trace 0000000000000000 ]---
> >
> > I also once changed the WARN_ON to WARN_ON_FOLIO and I got more info here:
> >
> > [ 3994.907255] page: refcount:9 mapcount:1 mapping:0000000000000000
> > index:0x7f90b3e98 pfn:0x615028
> > [ 3994.914449] head: order:3 mapcount:8 entire_mapcount:0
> > nr_pages_mapped:8 pincount:0
> > [ 3994.924534] memcg:ffff888106746000
> > [ 3994.927868] anon flags:
> > 0x17ffffc002084c(referenced|uptodate|owner_2|head|swapbacked|node=0|zone=2|lastcpupid=0x1fffff)
> > [ 3994.933479] raw: 0017ffffc002084c ffff88816edd9128 ffffea000beac108
> > ffff8882e8ba6bc9
> > [ 3994.936251] raw: 00000007f90b3e98 0000000000000000 0000000900000000
> > ffff888106746000
> > [ 3994.939466] head: 0017ffffc002084c ffff88816edd9128
> > ffffea000beac108 ffff8882e8ba6bc9
> > [ 3994.943355] head: 00000007f90b3e98 0000000000000000
> > 0000000900000000 ffff888106746000
> > [ 3994.946988] head: 0017ffffc0000203 ffffea0018540a01
> > 0000000800000007 00000000ffffffff
> > [ 3994.950328] head: ffffffff00000007 00000000800000a3
> > 0000000000000000 0000000000000008
> > [ 3994.953684] page dumped because:
> > VM_WARN_ON_FOLIO(folio_large_mapcount(folio) < folio_ref_count(folio))
> > [ 3994.957534] ------------[ cut here ]------------
> > [ 3994.959917] WARNING: CPU: 16 PID: 555282 at mm/memory.c:3794
> > do_wp_page+0x10c0/0x1110
> > [ 3994.963069] Modules linked in: zram virtiofs
> > [ 3994.964726] CPU: 16 UID: 0 PID: 555282 Comm: sh Kdump: loaded Not
> > tainted 6.15.0-rc1.ptch-ge39aef85f4c0-dirty #1431 PREEMPT(voluntary)
> > [ 3994.969985] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> > [ 3994.972905] RIP: 0010:do_wp_page+0x10c0/0x1110
> > [ 3994.974477] Code: fe ff 0f 0b bd f5 ff ff ff e9 16 fb ff ff 41 83
> > a9 bc 12 00 00 01 e9 2f fb ff ff 48 c7 c6 90 c2 49 82 4c 89 ef e8 40
> > fd fe ff <0f> 0b e9 6a fc ff ff 65 ff 00 f0 48 0f b
> > a 6d 00 1f 0f 83 46 fc ff
> > [ 3994.981033] RSP: 0000:ffffc9002b3c7d40 EFLAGS: 00010246
> > [ 3994.982636] RAX: 000000000000005b RBX: 0000000000000000 RCX: 0000000000000000
> > [ 3994.984778] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff889ffea16a80
> > [ 3994.986865] RBP: ffffea0018540a68 R08: 0000000000000000 R09: c0000000ffff7fff
> > [ 3994.989316] R10: 0000000000000001 R11: ffffc9002b3c7b80 R12: ffff88810cfd7d40
> > [ 3994.991654] R13: ffffea0018540a00 R14: 00007f90b3e9d620 R15: ffffc9002b3c7dd8
> > [ 3994.994076] FS: 00007f90b3caa740(0000) GS:ffff88a07b194000(0000)
> > knlGS:0000000000000000
> > [ 3994.996939] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 3994.998902] CR2: 00007f90b3e9d620 CR3: 0000000104088004 CR4: 0000000000770eb0
> > [ 3995.001314] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 3995.003746] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 3995.006173] PKRU: 55555554
> > [ 3995.007117] Call Trace:
> > [ 3995.007988] <TASK>
> > [ 3995.008755] ? __pfx_default_wake_function+0x10/0x10
> > [ 3995.010490] ? ___pte_offset_map+0x1b/0x110
> > [ 3995.011929] __handle_mm_fault+0xa51/0xf00
> > [ 3995.013346] handle_mm_fault+0x13d/0x360
> > [ 3995.014796] do_user_addr_fault+0x2f2/0x7f0
> > [ 3995.016331] ? sigprocmask+0x77/0xa0
> > [ 3995.017656] exc_page_fault+0x6a/0x140
> > [ 3995.018978] asm_exc_page_fault+0x26/0x30
> > [ 3995.020309] RIP: 0033:0x7f90b3d881a7
> > [ 3995.021461] Code: e8 4e b1 f8 ff 66 66 2e 0f 1f 84 00 00 00 00 00
> > 0f 1f 00 f3 0f 1e fa 55 31 c0 ba 01 00 00 00 48 89 e5 53 48 89 fb 48
> > 83 ec 08 <f0> 0f b1 15 71 54 11 00 0f 85 3b 01 00 0
> > 0 48 8b 35 84 54 11 00 48
> > [ 3995.028091] RSP: 002b:00007ffc33632c90 EFLAGS: 00010206
> > [ 3995.029992] RAX: 0000000000000000 RBX: 0000560cfbfc0a40 RCX: 0000000000000000
> > [ 3995.032456] RDX: 0000000000000001 RSI: 0000000000000005 RDI: 0000560cfbfc0a40
> > [ 3995.034794] RBP: 00007ffc33632ca0 R08: 00007ffc33632d50 R09: 00007ffc33632cff
> > [ 3995.037534] R10: 00007ffc33632c70 R11: 00007ffc33632d00 R12: 0000560cfbfc0a40
> > [ 3995.041063] R13: 00007f90b3e97fd0 R14: 00007f90b3e97fa8 R15: 0000000000000000
> > [ 3995.044390] </TASK>
> > [ 3995.045510] ---[ end trace 0000000000000000 ]---
> >
> > My guess is folio_ref_count is not a reliable thing to check here,
> > anything can increase the folio's ref account even without locking it,
> > for example, a swap cache lookup or maybe anything iterating the LRU.
>
> It is reliable, we are holding the mapcount lock, so for each mapcount
> we must have a corresponding refcount. If that is not the case, we have
> an issue elsewhere.
Yes, each mapcount should have a refcount, but could there be any
other thing to increase the refcount?
The check is: folio_large_mapcount(folio) < folio_ref_count(folio), so
maybe something just called folio_try_get and then put the folio
quickly.
>
> Other reference may only increase the refcount, but not violate the
> mapcount vs. refcount condition.
>
> Can you reproduce also with swap disabled?
I'm not sure how to reproduce it else way, I only saw this while the
swap stress test.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH v3 13/20] mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
From: David Hildenbrand @ 2025-04-19 16:32 UTC (permalink / raw)
To: Kairui Song
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <c7e85336-5e34-4dd9-950f-173f48ff0be1@redhat.com>
On 19.04.25 18:25, David Hildenbrand wrote:
> On 19.04.25 18:02, Kairui Song wrote:
>> On Tue, Mar 4, 2025 at 12:46 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> Currently, we never end up reusing PTE-mapped THPs after fork. This
>>> wasn't really a problem with PMD-sized THPs, because they would have to
>>> be PTE-mapped first, but it's getting a problem with smaller THP
>>> sizes that are effectively always PTE-mapped.
>>>
>>> With our new "mapped exclusively" vs "maybe mapped shared" logic for
>>> large folios, implementing CoW reuse for PTE-mapped THPs is straight
>>> forward: if exclusively mapped, make sure that all references are
>>> from these (our) mappings. Add some helpful comments to explain the
>>> details.
>>>
>>> CONFIG_TRANSPARENT_HUGEPAGE selects CONFIG_MM_ID. If we spot an anon
>>> large folio without CONFIG_TRANSPARENT_HUGEPAGE in that code, something
>>> is seriously messed up.
>>>
>>> There are plenty of things we can optimize in the future: For example, we
>>> could remember that the folio is fully exclusive so we could speedup
>>> the next fault further. Also, we could try "faulting around", turning
>>> surrounding PTEs that map the same folio writable. But especially the
>>> latter might increase COW latency, so it would need further
>>> investigation.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> mm/memory.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 75 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 73b783c7d7d51..bb245a8fe04bc 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -3729,19 +3729,86 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>>> return ret;
>>> }
>>>
>>> -static bool wp_can_reuse_anon_folio(struct folio *folio,
>>> - struct vm_area_struct *vma)
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> +static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
>>> + struct vm_area_struct *vma)
>>> {
>>> + bool exclusive = false;
>>> +
>>> + /* Let's just free up a large folio if only a single page is mapped. */
>>> + if (folio_large_mapcount(folio) <= 1)
>>> + return false;
>>> +
>>> /*
>>> - * We could currently only reuse a subpage of a large folio if no
>>> - * other subpages of the large folios are still mapped. However,
>>> - * let's just consistently not reuse subpages even if we could
>>> - * reuse in that scenario, and give back a large folio a bit
>>> - * sooner.
>>> + * The assumption for anonymous folios is that each page can only get
>>> + * mapped once into each MM. The only exception are KSM folios, which
>>> + * are always small.
>>> + *
>>> + * Each taken mapcount must be paired with exactly one taken reference,
>>> + * whereby the refcount must be incremented before the mapcount when
>>> + * mapping a page, and the refcount must be decremented after the
>>> + * mapcount when unmapping a page.
>>> + *
>>> + * If all folio references are from mappings, and all mappings are in
>>> + * the page tables of this MM, then this folio is exclusive to this MM.
>>> */
>>> - if (folio_test_large(folio))
>>> + if (folio_test_large_maybe_mapped_shared(folio))
>>> + return false;
>>> +
>>> + VM_WARN_ON_ONCE(folio_test_ksm(folio));
>>> + VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
>>> + VM_WARN_ON_ONCE(folio_entire_mapcount(folio));
>>> +
>>> + if (unlikely(folio_test_swapcache(folio))) {
>>> + /*
>>> + * Note: freeing up the swapcache will fail if some PTEs are
>>> + * still swap entries.
>>> + */
>>> + if (!folio_trylock(folio))
>>> + return false;
>>> + folio_free_swap(folio);
>>> + folio_unlock(folio);
>>> + }
>>> +
>>> + if (folio_large_mapcount(folio) != folio_ref_count(folio))
>>> return false;
>>>
>>> + /* Stabilize the mapcount vs. refcount and recheck. */
>>> + folio_lock_large_mapcount(folio);
>>> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
>>
>> Hi David, I'm seeing this WARN_ON being triggered on my test machine:
>
> Hi!
>
> So I assume the following will not sort out the issue for you, correct?
>
> https://lore.kernel.org/all/20250415095007.569836-1-david@redhat.com/T/#u
>
>>
>> I'm currently working on my swap table series and testing heavily with
>> swap related workloads. I thought my patch may break the kernel, but
>> after more investigation and reverting to current mm-unstable, it
>> still occurs (with a much lower chance though, I think my series
>> changed the timing so it's more frequent in my case).
>>
>> The test is simple, I just enable all mTHP sizes and repeatedly build
>> linux kernel in a 1G memcg using tmpfs.
>>
>> The WARN is reproducible with current mm-unstable
>> (dc683247117ee018e5da6b04f1c499acdc2a1418):
>>
>> [ 5268.100379] ------------[ cut here ]------------
>> [ 5268.105925] WARNING: CPU: 2 PID: 700274 at mm/memory.c:3792
>> do_wp_page+0xfc5/0x1080
>> [ 5268.112437] Modules linked in: zram virtiofs
>> [ 5268.115507] CPU: 2 UID: 0 PID: 700274 Comm: cc1 Kdump: loaded Not
>> tainted 6.15.0-rc2.ptch-gdc683247117e #1434 PREEMPT(voluntary)
>> [ 5268.120562] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
>> [ 5268.123025] RIP: 0010:do_wp_page+0xfc5/0x1080
>> [ 5268.124807] Code: 0d 80 77 32 02 0f 85 3e f1 ff ff 0f 1f 44 00 00
>> e9 34 f1 ff ff 48 0f ba 75 00 1f 65 ff 0d 63 77 32 02 0f 85 21 f1 ff
>> ff eb e1 <0f> 0b e9 10 fd ff ff 65 ff 00 f0 48 0f b
>> a 6d 00 1f 0f 83 ec fc ff
>> [ 5268.132034] RSP: 0000:ffffc900234efd48 EFLAGS: 00010297
>> [ 5268.134002] RAX: 0000000000000080 RBX: 0000000000000000 RCX: 000fffffffe00000
>> [ 5268.136609] RDX: 0000000000000081 RSI: 00007f009cbad000 RDI: ffffea0012da0000
>> [ 5268.139371] RBP: ffffea0012da0068 R08: 80000004b682d025 R09: 00007f009c7c0000
>> [ 5268.142183] R10: ffff88839c48b8c0 R11: 0000000000000000 R12: ffff88839c48b8c0
>> [ 5268.144738] R13: ffffea0012da0000 R14: 00007f009cbadf10 R15: ffffc900234efdd8
>> [ 5268.147540] FS: 00007f009d1fdac0(0000) GS:ffff88a07ae14000(0000)
>> knlGS:0000000000000000
>> [ 5268.150715] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 5268.153270] CR2: 00007f009cbadf10 CR3: 000000016c7c0001 CR4: 0000000000770eb0
>> [ 5268.155674] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [ 5268.158100] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> [ 5268.160613] PKRU: 55555554
>> [ 5268.161662] Call Trace:
>> [ 5268.162609] <TASK>
>> [ 5268.163438] ? ___pte_offset_map+0x1b/0x110
>> [ 5268.165309] __handle_mm_fault+0xa51/0xf00
>> [ 5268.166848] ? update_load_avg+0x80/0x760
>> [ 5268.168376] handle_mm_fault+0x13d/0x360
>> [ 5268.169930] do_user_addr_fault+0x2f2/0x7f0
>> [ 5268.171630] exc_page_fault+0x6a/0x140
>> [ 5268.173278] asm_exc_page_fault+0x26/0x30
>> [ 5268.174866] RIP: 0033:0x120e8e4
>> [ 5268.176272] Code: 84 a9 00 00 00 48 39 c3 0f 85 ae 00 00 00 48 8b
>> 43 20 48 89 45 38 48 85 c0 0f 85 b7 00 00 00 48 8b 43 18 48 8b 15 6c
>> 08 42 01 <0f> 11 43 10 48 89 1d 61 08 42 01 48 89 53 18 0f 11 03 0f 11
>> 43 20
>> [ 5268.184121] RSP: 002b:00007fff8a855160 EFLAGS: 00010246
>> [ 5268.186343] RAX: 00007f009cbadbd0 RBX: 00007f009cbadf00 RCX: 0000000000000000
>> [ 5268.189209] RDX: 00007f009cbba030 RSI: 00000000000006f4 RDI: 0000000000000000
>> [ 5268.192145] RBP: 00007f009cbb6460 R08: 00007f009d10f000 R09: 000000000000016c
>> [ 5268.194687] R10: 0000000000000000 R11: 0000000000000010 R12: 00007f009cf97660
>> [ 5268.197172] R13: 00007f009756ede0 R14: 00007f0097582348 R15: 0000000000000002
>> [ 5268.199419] </TASK>
>> [ 5268.200227] ---[ end trace 0000000000000000 ]---
>>
>> I also once changed the WARN_ON to WARN_ON_FOLIO and I got more info here:
>>
>> [ 3994.907255] page: refcount:9 mapcount:1 mapping:0000000000000000
>> index:0x7f90b3e98 pfn:0x615028
>> [ 3994.914449] head: order:3 mapcount:8 entire_mapcount:0
>> nr_pages_mapped:8 pincount:0
>> [ 3994.924534] memcg:ffff888106746000
>> [ 3994.927868] anon flags:
>> 0x17ffffc002084c(referenced|uptodate|owner_2|head|swapbacked|node=0|zone=2|lastcpupid=0x1fffff)
>> [ 3994.933479] raw: 0017ffffc002084c ffff88816edd9128 ffffea000beac108
>> ffff8882e8ba6bc9
>> [ 3994.936251] raw: 00000007f90b3e98 0000000000000000 0000000900000000
>> ffff888106746000
>> [ 3994.939466] head: 0017ffffc002084c ffff88816edd9128
>> ffffea000beac108 ffff8882e8ba6bc9
>> [ 3994.943355] head: 00000007f90b3e98 0000000000000000
>> 0000000900000000 ffff888106746000
>> [ 3994.946988] head: 0017ffffc0000203 ffffea0018540a01
>> 0000000800000007 00000000ffffffff
>> [ 3994.950328] head: ffffffff00000007 00000000800000a3
>> 0000000000000000 0000000000000008
>> [ 3994.953684] page dumped because:
>> VM_WARN_ON_FOLIO(folio_large_mapcount(folio) < folio_ref_count(folio))
>> [ 3994.957534] ------------[ cut here ]------------
>> [ 3994.959917] WARNING: CPU: 16 PID: 555282 at mm/memory.c:3794
>> do_wp_page+0x10c0/0x1110
>> [ 3994.963069] Modules linked in: zram virtiofs
>> [ 3994.964726] CPU: 16 UID: 0 PID: 555282 Comm: sh Kdump: loaded Not
>> tainted 6.15.0-rc1.ptch-ge39aef85f4c0-dirty #1431 PREEMPT(voluntary)
>> [ 3994.969985] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
>> [ 3994.972905] RIP: 0010:do_wp_page+0x10c0/0x1110
>> [ 3994.974477] Code: fe ff 0f 0b bd f5 ff ff ff e9 16 fb ff ff 41 83
>> a9 bc 12 00 00 01 e9 2f fb ff ff 48 c7 c6 90 c2 49 82 4c 89 ef e8 40
>> fd fe ff <0f> 0b e9 6a fc ff ff 65 ff 00 f0 48 0f b
>> a 6d 00 1f 0f 83 46 fc ff
>> [ 3994.981033] RSP: 0000:ffffc9002b3c7d40 EFLAGS: 00010246
>> [ 3994.982636] RAX: 000000000000005b RBX: 0000000000000000 RCX: 0000000000000000
>> [ 3994.984778] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff889ffea16a80
>> [ 3994.986865] RBP: ffffea0018540a68 R08: 0000000000000000 R09: c0000000ffff7fff
>> [ 3994.989316] R10: 0000000000000001 R11: ffffc9002b3c7b80 R12: ffff88810cfd7d40
>> [ 3994.991654] R13: ffffea0018540a00 R14: 00007f90b3e9d620 R15: ffffc9002b3c7dd8
>> [ 3994.994076] FS: 00007f90b3caa740(0000) GS:ffff88a07b194000(0000)
>> knlGS:0000000000000000
>> [ 3994.996939] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 3994.998902] CR2: 00007f90b3e9d620 CR3: 0000000104088004 CR4: 0000000000770eb0
>> [ 3995.001314] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [ 3995.003746] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> [ 3995.006173] PKRU: 55555554
>> [ 3995.007117] Call Trace:
>> [ 3995.007988] <TASK>
>> [ 3995.008755] ? __pfx_default_wake_function+0x10/0x10
>> [ 3995.010490] ? ___pte_offset_map+0x1b/0x110
>> [ 3995.011929] __handle_mm_fault+0xa51/0xf00
>> [ 3995.013346] handle_mm_fault+0x13d/0x360
>> [ 3995.014796] do_user_addr_fault+0x2f2/0x7f0
>> [ 3995.016331] ? sigprocmask+0x77/0xa0
>> [ 3995.017656] exc_page_fault+0x6a/0x140
>> [ 3995.018978] asm_exc_page_fault+0x26/0x30
>> [ 3995.020309] RIP: 0033:0x7f90b3d881a7
>> [ 3995.021461] Code: e8 4e b1 f8 ff 66 66 2e 0f 1f 84 00 00 00 00 00
>> 0f 1f 00 f3 0f 1e fa 55 31 c0 ba 01 00 00 00 48 89 e5 53 48 89 fb 48
>> 83 ec 08 <f0> 0f b1 15 71 54 11 00 0f 85 3b 01 00 0
>> 0 48 8b 35 84 54 11 00 48
>> [ 3995.028091] RSP: 002b:00007ffc33632c90 EFLAGS: 00010206
>> [ 3995.029992] RAX: 0000000000000000 RBX: 0000560cfbfc0a40 RCX: 0000000000000000
>> [ 3995.032456] RDX: 0000000000000001 RSI: 0000000000000005 RDI: 0000560cfbfc0a40
>> [ 3995.034794] RBP: 00007ffc33632ca0 R08: 00007ffc33632d50 R09: 00007ffc33632cff
>> [ 3995.037534] R10: 00007ffc33632c70 R11: 00007ffc33632d00 R12: 0000560cfbfc0a40
>> [ 3995.041063] R13: 00007f90b3e97fd0 R14: 00007f90b3e97fa8 R15: 0000000000000000
>> [ 3995.044390] </TASK>
>> [ 3995.045510] ---[ end trace 0000000000000000 ]---
>>
>> My guess is folio_ref_count is not a reliable thing to check here,
>> anything can increase the folio's ref account even without locking it,
>> for example, a swap cache lookup or maybe anything iterating the LRU.
>
> It is reliable, we are holding the mapcount lock, so for each mapcount
> we must have a corresponding refcount. If that is not the case, we have
> an issue elsewhere.
>
> Other reference may only increase the refcount, but not violate the
> mapcount vs. refcount condition.
>
> Can you reproduce also with swap disabled?
Oh, re-reading the condition 3 times, I realize that the sanity check is wrong ...
diff --git a/mm/memory.c b/mm/memory.c
index 037b6ce211f1f..a17eeef3f1f89 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3789,7 +3789,7 @@ static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
/* Stabilize the mapcount vs. refcount and recheck. */
folio_lock_large_mapcount(folio);
- VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
+ VM_WARN_ON_ONCE(folio_large_mapcount(folio) > folio_ref_count(folio));
if (folio_test_large_maybe_mapped_shared(folio))
goto unlock;
Our refcount must be at least the mapcount, that's what we want to assert.
Can you test and send a fix patch if that makes it fly for you?
--
Cheers,
David / dhildenb
^ permalink raw reply related
* Re: [PATCH v3 13/20] mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
From: David Hildenbrand @ 2025-04-19 16:25 UTC (permalink / raw)
To: Kairui Song
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <CAMgjq7D+ea3eg9gRCVvRnto3Sv3_H3WVhupX4e=k8T5QAfBHbw@mail.gmail.com>
On 19.04.25 18:02, Kairui Song wrote:
> On Tue, Mar 4, 2025 at 12:46 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> Currently, we never end up reusing PTE-mapped THPs after fork. This
>> wasn't really a problem with PMD-sized THPs, because they would have to
>> be PTE-mapped first, but it's getting a problem with smaller THP
>> sizes that are effectively always PTE-mapped.
>>
>> With our new "mapped exclusively" vs "maybe mapped shared" logic for
>> large folios, implementing CoW reuse for PTE-mapped THPs is straight
>> forward: if exclusively mapped, make sure that all references are
>> from these (our) mappings. Add some helpful comments to explain the
>> details.
>>
>> CONFIG_TRANSPARENT_HUGEPAGE selects CONFIG_MM_ID. If we spot an anon
>> large folio without CONFIG_TRANSPARENT_HUGEPAGE in that code, something
>> is seriously messed up.
>>
>> There are plenty of things we can optimize in the future: For example, we
>> could remember that the folio is fully exclusive so we could speedup
>> the next fault further. Also, we could try "faulting around", turning
>> surrounding PTEs that map the same folio writable. But especially the
>> latter might increase COW latency, so it would need further
>> investigation.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> mm/memory.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 75 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 73b783c7d7d51..bb245a8fe04bc 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3729,19 +3729,86 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>> return ret;
>> }
>>
>> -static bool wp_can_reuse_anon_folio(struct folio *folio,
>> - struct vm_area_struct *vma)
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
>> + struct vm_area_struct *vma)
>> {
>> + bool exclusive = false;
>> +
>> + /* Let's just free up a large folio if only a single page is mapped. */
>> + if (folio_large_mapcount(folio) <= 1)
>> + return false;
>> +
>> /*
>> - * We could currently only reuse a subpage of a large folio if no
>> - * other subpages of the large folios are still mapped. However,
>> - * let's just consistently not reuse subpages even if we could
>> - * reuse in that scenario, and give back a large folio a bit
>> - * sooner.
>> + * The assumption for anonymous folios is that each page can only get
>> + * mapped once into each MM. The only exception are KSM folios, which
>> + * are always small.
>> + *
>> + * Each taken mapcount must be paired with exactly one taken reference,
>> + * whereby the refcount must be incremented before the mapcount when
>> + * mapping a page, and the refcount must be decremented after the
>> + * mapcount when unmapping a page.
>> + *
>> + * If all folio references are from mappings, and all mappings are in
>> + * the page tables of this MM, then this folio is exclusive to this MM.
>> */
>> - if (folio_test_large(folio))
>> + if (folio_test_large_maybe_mapped_shared(folio))
>> + return false;
>> +
>> + VM_WARN_ON_ONCE(folio_test_ksm(folio));
>> + VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
>> + VM_WARN_ON_ONCE(folio_entire_mapcount(folio));
>> +
>> + if (unlikely(folio_test_swapcache(folio))) {
>> + /*
>> + * Note: freeing up the swapcache will fail if some PTEs are
>> + * still swap entries.
>> + */
>> + if (!folio_trylock(folio))
>> + return false;
>> + folio_free_swap(folio);
>> + folio_unlock(folio);
>> + }
>> +
>> + if (folio_large_mapcount(folio) != folio_ref_count(folio))
>> return false;
>>
>> + /* Stabilize the mapcount vs. refcount and recheck. */
>> + folio_lock_large_mapcount(folio);
>> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
>
> Hi David, I'm seeing this WARN_ON being triggered on my test machine:
Hi!
So I assume the following will not sort out the issue for you, correct?
https://lore.kernel.org/all/20250415095007.569836-1-david@redhat.com/T/#u
>
> I'm currently working on my swap table series and testing heavily with
> swap related workloads. I thought my patch may break the kernel, but
> after more investigation and reverting to current mm-unstable, it
> still occurs (with a much lower chance though, I think my series
> changed the timing so it's more frequent in my case).
>
> The test is simple, I just enable all mTHP sizes and repeatedly build
> linux kernel in a 1G memcg using tmpfs.
>
> The WARN is reproducible with current mm-unstable
> (dc683247117ee018e5da6b04f1c499acdc2a1418):
>
> [ 5268.100379] ------------[ cut here ]------------
> [ 5268.105925] WARNING: CPU: 2 PID: 700274 at mm/memory.c:3792
> do_wp_page+0xfc5/0x1080
> [ 5268.112437] Modules linked in: zram virtiofs
> [ 5268.115507] CPU: 2 UID: 0 PID: 700274 Comm: cc1 Kdump: loaded Not
> tainted 6.15.0-rc2.ptch-gdc683247117e #1434 PREEMPT(voluntary)
> [ 5268.120562] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> [ 5268.123025] RIP: 0010:do_wp_page+0xfc5/0x1080
> [ 5268.124807] Code: 0d 80 77 32 02 0f 85 3e f1 ff ff 0f 1f 44 00 00
> e9 34 f1 ff ff 48 0f ba 75 00 1f 65 ff 0d 63 77 32 02 0f 85 21 f1 ff
> ff eb e1 <0f> 0b e9 10 fd ff ff 65 ff 00 f0 48 0f b
> a 6d 00 1f 0f 83 ec fc ff
> [ 5268.132034] RSP: 0000:ffffc900234efd48 EFLAGS: 00010297
> [ 5268.134002] RAX: 0000000000000080 RBX: 0000000000000000 RCX: 000fffffffe00000
> [ 5268.136609] RDX: 0000000000000081 RSI: 00007f009cbad000 RDI: ffffea0012da0000
> [ 5268.139371] RBP: ffffea0012da0068 R08: 80000004b682d025 R09: 00007f009c7c0000
> [ 5268.142183] R10: ffff88839c48b8c0 R11: 0000000000000000 R12: ffff88839c48b8c0
> [ 5268.144738] R13: ffffea0012da0000 R14: 00007f009cbadf10 R15: ffffc900234efdd8
> [ 5268.147540] FS: 00007f009d1fdac0(0000) GS:ffff88a07ae14000(0000)
> knlGS:0000000000000000
> [ 5268.150715] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 5268.153270] CR2: 00007f009cbadf10 CR3: 000000016c7c0001 CR4: 0000000000770eb0
> [ 5268.155674] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 5268.158100] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 5268.160613] PKRU: 55555554
> [ 5268.161662] Call Trace:
> [ 5268.162609] <TASK>
> [ 5268.163438] ? ___pte_offset_map+0x1b/0x110
> [ 5268.165309] __handle_mm_fault+0xa51/0xf00
> [ 5268.166848] ? update_load_avg+0x80/0x760
> [ 5268.168376] handle_mm_fault+0x13d/0x360
> [ 5268.169930] do_user_addr_fault+0x2f2/0x7f0
> [ 5268.171630] exc_page_fault+0x6a/0x140
> [ 5268.173278] asm_exc_page_fault+0x26/0x30
> [ 5268.174866] RIP: 0033:0x120e8e4
> [ 5268.176272] Code: 84 a9 00 00 00 48 39 c3 0f 85 ae 00 00 00 48 8b
> 43 20 48 89 45 38 48 85 c0 0f 85 b7 00 00 00 48 8b 43 18 48 8b 15 6c
> 08 42 01 <0f> 11 43 10 48 89 1d 61 08 42 01 48 89 53 18 0f 11 03 0f 11
> 43 20
> [ 5268.184121] RSP: 002b:00007fff8a855160 EFLAGS: 00010246
> [ 5268.186343] RAX: 00007f009cbadbd0 RBX: 00007f009cbadf00 RCX: 0000000000000000
> [ 5268.189209] RDX: 00007f009cbba030 RSI: 00000000000006f4 RDI: 0000000000000000
> [ 5268.192145] RBP: 00007f009cbb6460 R08: 00007f009d10f000 R09: 000000000000016c
> [ 5268.194687] R10: 0000000000000000 R11: 0000000000000010 R12: 00007f009cf97660
> [ 5268.197172] R13: 00007f009756ede0 R14: 00007f0097582348 R15: 0000000000000002
> [ 5268.199419] </TASK>
> [ 5268.200227] ---[ end trace 0000000000000000 ]---
>
> I also once changed the WARN_ON to WARN_ON_FOLIO and I got more info here:
>
> [ 3994.907255] page: refcount:9 mapcount:1 mapping:0000000000000000
> index:0x7f90b3e98 pfn:0x615028
> [ 3994.914449] head: order:3 mapcount:8 entire_mapcount:0
> nr_pages_mapped:8 pincount:0
> [ 3994.924534] memcg:ffff888106746000
> [ 3994.927868] anon flags:
> 0x17ffffc002084c(referenced|uptodate|owner_2|head|swapbacked|node=0|zone=2|lastcpupid=0x1fffff)
> [ 3994.933479] raw: 0017ffffc002084c ffff88816edd9128 ffffea000beac108
> ffff8882e8ba6bc9
> [ 3994.936251] raw: 00000007f90b3e98 0000000000000000 0000000900000000
> ffff888106746000
> [ 3994.939466] head: 0017ffffc002084c ffff88816edd9128
> ffffea000beac108 ffff8882e8ba6bc9
> [ 3994.943355] head: 00000007f90b3e98 0000000000000000
> 0000000900000000 ffff888106746000
> [ 3994.946988] head: 0017ffffc0000203 ffffea0018540a01
> 0000000800000007 00000000ffffffff
> [ 3994.950328] head: ffffffff00000007 00000000800000a3
> 0000000000000000 0000000000000008
> [ 3994.953684] page dumped because:
> VM_WARN_ON_FOLIO(folio_large_mapcount(folio) < folio_ref_count(folio))
> [ 3994.957534] ------------[ cut here ]------------
> [ 3994.959917] WARNING: CPU: 16 PID: 555282 at mm/memory.c:3794
> do_wp_page+0x10c0/0x1110
> [ 3994.963069] Modules linked in: zram virtiofs
> [ 3994.964726] CPU: 16 UID: 0 PID: 555282 Comm: sh Kdump: loaded Not
> tainted 6.15.0-rc1.ptch-ge39aef85f4c0-dirty #1431 PREEMPT(voluntary)
> [ 3994.969985] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
> [ 3994.972905] RIP: 0010:do_wp_page+0x10c0/0x1110
> [ 3994.974477] Code: fe ff 0f 0b bd f5 ff ff ff e9 16 fb ff ff 41 83
> a9 bc 12 00 00 01 e9 2f fb ff ff 48 c7 c6 90 c2 49 82 4c 89 ef e8 40
> fd fe ff <0f> 0b e9 6a fc ff ff 65 ff 00 f0 48 0f b
> a 6d 00 1f 0f 83 46 fc ff
> [ 3994.981033] RSP: 0000:ffffc9002b3c7d40 EFLAGS: 00010246
> [ 3994.982636] RAX: 000000000000005b RBX: 0000000000000000 RCX: 0000000000000000
> [ 3994.984778] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff889ffea16a80
> [ 3994.986865] RBP: ffffea0018540a68 R08: 0000000000000000 R09: c0000000ffff7fff
> [ 3994.989316] R10: 0000000000000001 R11: ffffc9002b3c7b80 R12: ffff88810cfd7d40
> [ 3994.991654] R13: ffffea0018540a00 R14: 00007f90b3e9d620 R15: ffffc9002b3c7dd8
> [ 3994.994076] FS: 00007f90b3caa740(0000) GS:ffff88a07b194000(0000)
> knlGS:0000000000000000
> [ 3994.996939] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3994.998902] CR2: 00007f90b3e9d620 CR3: 0000000104088004 CR4: 0000000000770eb0
> [ 3995.001314] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 3995.003746] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 3995.006173] PKRU: 55555554
> [ 3995.007117] Call Trace:
> [ 3995.007988] <TASK>
> [ 3995.008755] ? __pfx_default_wake_function+0x10/0x10
> [ 3995.010490] ? ___pte_offset_map+0x1b/0x110
> [ 3995.011929] __handle_mm_fault+0xa51/0xf00
> [ 3995.013346] handle_mm_fault+0x13d/0x360
> [ 3995.014796] do_user_addr_fault+0x2f2/0x7f0
> [ 3995.016331] ? sigprocmask+0x77/0xa0
> [ 3995.017656] exc_page_fault+0x6a/0x140
> [ 3995.018978] asm_exc_page_fault+0x26/0x30
> [ 3995.020309] RIP: 0033:0x7f90b3d881a7
> [ 3995.021461] Code: e8 4e b1 f8 ff 66 66 2e 0f 1f 84 00 00 00 00 00
> 0f 1f 00 f3 0f 1e fa 55 31 c0 ba 01 00 00 00 48 89 e5 53 48 89 fb 48
> 83 ec 08 <f0> 0f b1 15 71 54 11 00 0f 85 3b 01 00 0
> 0 48 8b 35 84 54 11 00 48
> [ 3995.028091] RSP: 002b:00007ffc33632c90 EFLAGS: 00010206
> [ 3995.029992] RAX: 0000000000000000 RBX: 0000560cfbfc0a40 RCX: 0000000000000000
> [ 3995.032456] RDX: 0000000000000001 RSI: 0000000000000005 RDI: 0000560cfbfc0a40
> [ 3995.034794] RBP: 00007ffc33632ca0 R08: 00007ffc33632d50 R09: 00007ffc33632cff
> [ 3995.037534] R10: 00007ffc33632c70 R11: 00007ffc33632d00 R12: 0000560cfbfc0a40
> [ 3995.041063] R13: 00007f90b3e97fd0 R14: 00007f90b3e97fa8 R15: 0000000000000000
> [ 3995.044390] </TASK>
> [ 3995.045510] ---[ end trace 0000000000000000 ]---
>
> My guess is folio_ref_count is not a reliable thing to check here,
> anything can increase the folio's ref account even without locking it,
> for example, a swap cache lookup or maybe anything iterating the LRU.
It is reliable, we are holding the mapcount lock, so for each mapcount
we must have a corresponding refcount. If that is not the case, we have
an issue elsewhere.
Other reference may only increase the refcount, but not violate the
mapcount vs. refcount condition.
Can you reproduce also with swap disabled?
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 13/20] mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
From: Kairui Song @ 2025-04-19 16:02 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <20250303163014.1128035-14-david@redhat.com>
On Tue, Mar 4, 2025 at 12:46 AM David Hildenbrand <david@redhat.com> wrote:
>
> Currently, we never end up reusing PTE-mapped THPs after fork. This
> wasn't really a problem with PMD-sized THPs, because they would have to
> be PTE-mapped first, but it's getting a problem with smaller THP
> sizes that are effectively always PTE-mapped.
>
> With our new "mapped exclusively" vs "maybe mapped shared" logic for
> large folios, implementing CoW reuse for PTE-mapped THPs is straight
> forward: if exclusively mapped, make sure that all references are
> from these (our) mappings. Add some helpful comments to explain the
> details.
>
> CONFIG_TRANSPARENT_HUGEPAGE selects CONFIG_MM_ID. If we spot an anon
> large folio without CONFIG_TRANSPARENT_HUGEPAGE in that code, something
> is seriously messed up.
>
> There are plenty of things we can optimize in the future: For example, we
> could remember that the folio is fully exclusive so we could speedup
> the next fault further. Also, we could try "faulting around", turning
> surrounding PTEs that map the same folio writable. But especially the
> latter might increase COW latency, so it would need further
> investigation.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> mm/memory.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 73b783c7d7d51..bb245a8fe04bc 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3729,19 +3729,86 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
> return ret;
> }
>
> -static bool wp_can_reuse_anon_folio(struct folio *folio,
> - struct vm_area_struct *vma)
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
> + struct vm_area_struct *vma)
> {
> + bool exclusive = false;
> +
> + /* Let's just free up a large folio if only a single page is mapped. */
> + if (folio_large_mapcount(folio) <= 1)
> + return false;
> +
> /*
> - * We could currently only reuse a subpage of a large folio if no
> - * other subpages of the large folios are still mapped. However,
> - * let's just consistently not reuse subpages even if we could
> - * reuse in that scenario, and give back a large folio a bit
> - * sooner.
> + * The assumption for anonymous folios is that each page can only get
> + * mapped once into each MM. The only exception are KSM folios, which
> + * are always small.
> + *
> + * Each taken mapcount must be paired with exactly one taken reference,
> + * whereby the refcount must be incremented before the mapcount when
> + * mapping a page, and the refcount must be decremented after the
> + * mapcount when unmapping a page.
> + *
> + * If all folio references are from mappings, and all mappings are in
> + * the page tables of this MM, then this folio is exclusive to this MM.
> */
> - if (folio_test_large(folio))
> + if (folio_test_large_maybe_mapped_shared(folio))
> + return false;
> +
> + VM_WARN_ON_ONCE(folio_test_ksm(folio));
> + VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
> + VM_WARN_ON_ONCE(folio_entire_mapcount(folio));
> +
> + if (unlikely(folio_test_swapcache(folio))) {
> + /*
> + * Note: freeing up the swapcache will fail if some PTEs are
> + * still swap entries.
> + */
> + if (!folio_trylock(folio))
> + return false;
> + folio_free_swap(folio);
> + folio_unlock(folio);
> + }
> +
> + if (folio_large_mapcount(folio) != folio_ref_count(folio))
> return false;
>
> + /* Stabilize the mapcount vs. refcount and recheck. */
> + folio_lock_large_mapcount(folio);
> + VM_WARN_ON_ONCE(folio_large_mapcount(folio) < folio_ref_count(folio));
Hi David, I'm seeing this WARN_ON being triggered on my test machine:
I'm currently working on my swap table series and testing heavily with
swap related workloads. I thought my patch may break the kernel, but
after more investigation and reverting to current mm-unstable, it
still occurs (with a much lower chance though, I think my series
changed the timing so it's more frequent in my case).
The test is simple, I just enable all mTHP sizes and repeatedly build
linux kernel in a 1G memcg using tmpfs.
The WARN is reproducible with current mm-unstable
(dc683247117ee018e5da6b04f1c499acdc2a1418):
[ 5268.100379] ------------[ cut here ]------------
[ 5268.105925] WARNING: CPU: 2 PID: 700274 at mm/memory.c:3792
do_wp_page+0xfc5/0x1080
[ 5268.112437] Modules linked in: zram virtiofs
[ 5268.115507] CPU: 2 UID: 0 PID: 700274 Comm: cc1 Kdump: loaded Not
tainted 6.15.0-rc2.ptch-gdc683247117e #1434 PREEMPT(voluntary)
[ 5268.120562] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
[ 5268.123025] RIP: 0010:do_wp_page+0xfc5/0x1080
[ 5268.124807] Code: 0d 80 77 32 02 0f 85 3e f1 ff ff 0f 1f 44 00 00
e9 34 f1 ff ff 48 0f ba 75 00 1f 65 ff 0d 63 77 32 02 0f 85 21 f1 ff
ff eb e1 <0f> 0b e9 10 fd ff ff 65 ff 00 f0 48 0f b
a 6d 00 1f 0f 83 ec fc ff
[ 5268.132034] RSP: 0000:ffffc900234efd48 EFLAGS: 00010297
[ 5268.134002] RAX: 0000000000000080 RBX: 0000000000000000 RCX: 000fffffffe00000
[ 5268.136609] RDX: 0000000000000081 RSI: 00007f009cbad000 RDI: ffffea0012da0000
[ 5268.139371] RBP: ffffea0012da0068 R08: 80000004b682d025 R09: 00007f009c7c0000
[ 5268.142183] R10: ffff88839c48b8c0 R11: 0000000000000000 R12: ffff88839c48b8c0
[ 5268.144738] R13: ffffea0012da0000 R14: 00007f009cbadf10 R15: ffffc900234efdd8
[ 5268.147540] FS: 00007f009d1fdac0(0000) GS:ffff88a07ae14000(0000)
knlGS:0000000000000000
[ 5268.150715] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5268.153270] CR2: 00007f009cbadf10 CR3: 000000016c7c0001 CR4: 0000000000770eb0
[ 5268.155674] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 5268.158100] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 5268.160613] PKRU: 55555554
[ 5268.161662] Call Trace:
[ 5268.162609] <TASK>
[ 5268.163438] ? ___pte_offset_map+0x1b/0x110
[ 5268.165309] __handle_mm_fault+0xa51/0xf00
[ 5268.166848] ? update_load_avg+0x80/0x760
[ 5268.168376] handle_mm_fault+0x13d/0x360
[ 5268.169930] do_user_addr_fault+0x2f2/0x7f0
[ 5268.171630] exc_page_fault+0x6a/0x140
[ 5268.173278] asm_exc_page_fault+0x26/0x30
[ 5268.174866] RIP: 0033:0x120e8e4
[ 5268.176272] Code: 84 a9 00 00 00 48 39 c3 0f 85 ae 00 00 00 48 8b
43 20 48 89 45 38 48 85 c0 0f 85 b7 00 00 00 48 8b 43 18 48 8b 15 6c
08 42 01 <0f> 11 43 10 48 89 1d 61 08 42 01 48 89 53 18 0f 11 03 0f 11
43 20
[ 5268.184121] RSP: 002b:00007fff8a855160 EFLAGS: 00010246
[ 5268.186343] RAX: 00007f009cbadbd0 RBX: 00007f009cbadf00 RCX: 0000000000000000
[ 5268.189209] RDX: 00007f009cbba030 RSI: 00000000000006f4 RDI: 0000000000000000
[ 5268.192145] RBP: 00007f009cbb6460 R08: 00007f009d10f000 R09: 000000000000016c
[ 5268.194687] R10: 0000000000000000 R11: 0000000000000010 R12: 00007f009cf97660
[ 5268.197172] R13: 00007f009756ede0 R14: 00007f0097582348 R15: 0000000000000002
[ 5268.199419] </TASK>
[ 5268.200227] ---[ end trace 0000000000000000 ]---
I also once changed the WARN_ON to WARN_ON_FOLIO and I got more info here:
[ 3994.907255] page: refcount:9 mapcount:1 mapping:0000000000000000
index:0x7f90b3e98 pfn:0x615028
[ 3994.914449] head: order:3 mapcount:8 entire_mapcount:0
nr_pages_mapped:8 pincount:0
[ 3994.924534] memcg:ffff888106746000
[ 3994.927868] anon flags:
0x17ffffc002084c(referenced|uptodate|owner_2|head|swapbacked|node=0|zone=2|lastcpupid=0x1fffff)
[ 3994.933479] raw: 0017ffffc002084c ffff88816edd9128 ffffea000beac108
ffff8882e8ba6bc9
[ 3994.936251] raw: 00000007f90b3e98 0000000000000000 0000000900000000
ffff888106746000
[ 3994.939466] head: 0017ffffc002084c ffff88816edd9128
ffffea000beac108 ffff8882e8ba6bc9
[ 3994.943355] head: 00000007f90b3e98 0000000000000000
0000000900000000 ffff888106746000
[ 3994.946988] head: 0017ffffc0000203 ffffea0018540a01
0000000800000007 00000000ffffffff
[ 3994.950328] head: ffffffff00000007 00000000800000a3
0000000000000000 0000000000000008
[ 3994.953684] page dumped because:
VM_WARN_ON_FOLIO(folio_large_mapcount(folio) < folio_ref_count(folio))
[ 3994.957534] ------------[ cut here ]------------
[ 3994.959917] WARNING: CPU: 16 PID: 555282 at mm/memory.c:3794
do_wp_page+0x10c0/0x1110
[ 3994.963069] Modules linked in: zram virtiofs
[ 3994.964726] CPU: 16 UID: 0 PID: 555282 Comm: sh Kdump: loaded Not
tainted 6.15.0-rc1.ptch-ge39aef85f4c0-dirty #1431 PREEMPT(voluntary)
[ 3994.969985] Hardware name: Red Hat KVM/RHEL-AV, BIOS 0.0.0 02/06/2015
[ 3994.972905] RIP: 0010:do_wp_page+0x10c0/0x1110
[ 3994.974477] Code: fe ff 0f 0b bd f5 ff ff ff e9 16 fb ff ff 41 83
a9 bc 12 00 00 01 e9 2f fb ff ff 48 c7 c6 90 c2 49 82 4c 89 ef e8 40
fd fe ff <0f> 0b e9 6a fc ff ff 65 ff 00 f0 48 0f b
a 6d 00 1f 0f 83 46 fc ff
[ 3994.981033] RSP: 0000:ffffc9002b3c7d40 EFLAGS: 00010246
[ 3994.982636] RAX: 000000000000005b RBX: 0000000000000000 RCX: 0000000000000000
[ 3994.984778] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff889ffea16a80
[ 3994.986865] RBP: ffffea0018540a68 R08: 0000000000000000 R09: c0000000ffff7fff
[ 3994.989316] R10: 0000000000000001 R11: ffffc9002b3c7b80 R12: ffff88810cfd7d40
[ 3994.991654] R13: ffffea0018540a00 R14: 00007f90b3e9d620 R15: ffffc9002b3c7dd8
[ 3994.994076] FS: 00007f90b3caa740(0000) GS:ffff88a07b194000(0000)
knlGS:0000000000000000
[ 3994.996939] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3994.998902] CR2: 00007f90b3e9d620 CR3: 0000000104088004 CR4: 0000000000770eb0
[ 3995.001314] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 3995.003746] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 3995.006173] PKRU: 55555554
[ 3995.007117] Call Trace:
[ 3995.007988] <TASK>
[ 3995.008755] ? __pfx_default_wake_function+0x10/0x10
[ 3995.010490] ? ___pte_offset_map+0x1b/0x110
[ 3995.011929] __handle_mm_fault+0xa51/0xf00
[ 3995.013346] handle_mm_fault+0x13d/0x360
[ 3995.014796] do_user_addr_fault+0x2f2/0x7f0
[ 3995.016331] ? sigprocmask+0x77/0xa0
[ 3995.017656] exc_page_fault+0x6a/0x140
[ 3995.018978] asm_exc_page_fault+0x26/0x30
[ 3995.020309] RIP: 0033:0x7f90b3d881a7
[ 3995.021461] Code: e8 4e b1 f8 ff 66 66 2e 0f 1f 84 00 00 00 00 00
0f 1f 00 f3 0f 1e fa 55 31 c0 ba 01 00 00 00 48 89 e5 53 48 89 fb 48
83 ec 08 <f0> 0f b1 15 71 54 11 00 0f 85 3b 01 00 0
0 48 8b 35 84 54 11 00 48
[ 3995.028091] RSP: 002b:00007ffc33632c90 EFLAGS: 00010206
[ 3995.029992] RAX: 0000000000000000 RBX: 0000560cfbfc0a40 RCX: 0000000000000000
[ 3995.032456] RDX: 0000000000000001 RSI: 0000000000000005 RDI: 0000560cfbfc0a40
[ 3995.034794] RBP: 00007ffc33632ca0 R08: 00007ffc33632d50 R09: 00007ffc33632cff
[ 3995.037534] R10: 00007ffc33632c70 R11: 00007ffc33632d00 R12: 0000560cfbfc0a40
[ 3995.041063] R13: 00007f90b3e97fd0 R14: 00007f90b3e97fa8 R15: 0000000000000000
[ 3995.044390] </TASK>
[ 3995.045510] ---[ end trace 0000000000000000 ]---
My guess is folio_ref_count is not a reliable thing to check here,
anything can increase the folio's ref account even without locking it,
for example, a swap cache lookup or maybe anything iterating the LRU.
^ permalink raw reply
* newlines in filenames; POSIX.1-2024
From: Alejandro Colomar @ 2025-04-16 16:50 UTC (permalink / raw)
To: linux-kernel, linux-api, linux-man
[-- Attachment #1: Type: text/plain, Size: 754 bytes --]
Hi,
I'm updating the manual pages for POSIX.1-2024. One of the changes in
this revision is that POSIX now encourages implementations to disallow
using new-line characters in file names.
Historically, Linux (and maybe all existing POSIX systems?) has allowed
new-line characters in file names.
I guess there's no intention to change that behavior. But I should ask.
I thought of adding this paragraph to all pages that create file names:
+.SH CAVEATS
+POSIX.1-2024 encourages implementations to
+disallow creation of filenames containing new-line characters.
+Linux doesn't follow this,
+and allows using new-line characters.
Are there any comments?
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time
From: Darrick J. Wong @ 2025-04-16 16:26 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <81f0fe3e-4c1a-497d-b20e-1f8d182ed208@oracle.com>
On Wed, Apr 16, 2025 at 11:08:25AM +0100, John Garry wrote:
> On 15/04/2025 23:36, Darrick J. Wong wrote:
>
> Thanks for this, but it still seems to be problematic for me.
>
> In my test, I have agsize=22400, and when I attempt to mount with
> atomic_write_max=8M, it passes when it shouldn't. It should not because
> max_pow_of_two_factor(22400) = 128, and 8MB > 128 FSB.
>
> How about these addition checks:
>
> > +
> > + if (new_max_bytes) {
> > + xfs_extlen_t max_write_fsbs =
> > + rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
> > + xfs_extlen_t max_group_fsbs =
> > + max(mp->m_groups[XG_TYPE_AG].blocks,
> > + mp->m_groups[XG_TYPE_RTG].blocks);
> > +
> > + ASSERT(max_write_fsbs <= U32_MAX);
>
> if (!is_power_of_2(new_max_bytes)) {
> xfs_warn(mp,
> "max atomic write size of %llu bytes is not a power-of-2",
> new_max_bytes);
> return -EINVAL;
> }
Long-term I'm not convinced that we really need to have all these power
of two checks because the software fallback can remap just about
anything, but for now I see no harm in doing this because
generic_atomic_write_valid enforces that property on the IO length.
> > +
> > + if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
> > + xfs_warn(mp,
> > + "max atomic write size of %llu bytes not aligned with fsblock",
> > + new_max_bytes);
> > + return -EINVAL;
> > + }
> > +
> > + if (new_max_fsbs > max_write_fsbs) {
> > + xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than max write size %lluk",
> > + new_max_bytes >> 10,
> > + XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
> > + return -EINVAL;
> > + }
> > +
> > + if (new_max_fsbs > max_group_fsbs) {
> > + xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than allocation group size %lluk",
> > + new_max_bytes >> 10,
> > + XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> > + return -EINVAL;
> > + }
> > + }
> > +
>
> if (new_max_fsbs > max_pow_of_two_factor(max_group_fsbs)) {
> xfs_warn(mp,
> "max atomic write size of %lluk not aligned with allocation group size
> %lluk",
> new_max_bytes >> 10,
> XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> return -EINVAL;
I think I'd rather clean up these bits:
if (mp->m_ddev_targp->bt_bdev_awu_min > 0)
max_agsize = max_pow_of_two_factor(mp->m_sb.sb_agblocks);
else
max_agsize = mp->m_ag_max_usable;
and
if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
max_rgsize = max_pow_of_two_factor(rgs->blocks);
else
max_rgsize = rgs->blocks;
into a shared helper for xfs_compute_atomic_write_unit_max so that we
use the exact same logic in both places. But I agree with the general
direction.
--D
> }
>
> thanks,
> John
>
^ permalink raw reply
* Re: [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time
From: John Garry @ 2025-04-16 10:08 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415223625.GV25675@frogsfrogsfrogs>
On 15/04/2025 23:36, Darrick J. Wong wrote:
Thanks for this, but it still seems to be problematic for me.
In my test, I have agsize=22400, and when I attempt to mount with
atomic_write_max=8M, it passes when it shouldn't. It should not because
max_pow_of_two_factor(22400) = 128, and 8MB > 128 FSB.
How about these addition checks:
> +
> + if (new_max_bytes) {
> + xfs_extlen_t max_write_fsbs =
> + rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
> + xfs_extlen_t max_group_fsbs =
> + max(mp->m_groups[XG_TYPE_AG].blocks,
> + mp->m_groups[XG_TYPE_RTG].blocks);
> +
> + ASSERT(max_write_fsbs <= U32_MAX);
if (!is_power_of_2(new_max_bytes)) {
xfs_warn(mp,
"max atomic write size of %llu bytes is not a power-of-2",
new_max_bytes);
return -EINVAL;
}
> +
> + if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
> + xfs_warn(mp,
> + "max atomic write size of %llu bytes not aligned with fsblock",
> + new_max_bytes);
> + return -EINVAL;
> + }
> +
> + if (new_max_fsbs > max_write_fsbs) {
> + xfs_warn(mp,
> + "max atomic write size of %lluk cannot be larger than max write size %lluk",
> + new_max_bytes >> 10,
> + XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
> + return -EINVAL;
> + }
> +
> + if (new_max_fsbs > max_group_fsbs) {
> + xfs_warn(mp,
> + "max atomic write size of %lluk cannot be larger than allocation group size %lluk",
> + new_max_bytes >> 10,
> + XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> + return -EINVAL;
> + }
> + }
> +
if (new_max_fsbs > max_pow_of_two_factor(max_group_fsbs)) {
xfs_warn(mp,
"max atomic write size of %lluk not aligned with allocation group size
%lluk",
new_max_bytes >> 10,
XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
return -EINVAL;
}
thanks,
John
^ permalink raw reply
* [PATCH RFT v16 8/8] selftests/clone3: Test shadow stack support
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
Add basic test coverage for specifying the shadow stack for a newly
created thread via clone3(), including coverage of the newly extended
argument structure. We check that a user specified shadow stack can be
provided, and that invalid combinations of parameters are rejected.
In order to facilitate testing on systems without userspace shadow stack
support we manually enable shadow stacks on startup, this is architecture
specific due to the use of an arch_prctl() on x86. Due to interactions with
potential userspace locking of features we actually detect support for
shadow stacks on the running system by attempting to allocate a shadow
stack page during initialisation using map_shadow_stack(), warning if this
succeeds when the enable failed.
In order to allow testing of user configured shadow stacks on
architectures with that feature we need to ensure that we do not return
from the function where the clone3() syscall is called in the child
process, doing so would trigger a shadow stack underflow. To do this we
use inline assembly rather than the standard syscall wrapper to call
clone3(). In order to avoid surprises we also use a syscall rather than
the libc exit() function., this should be overly cautious.
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 143 +++++++++++++++++++++-
tools/testing/selftests/clone3/clone3_selftests.h | 63 ++++++++++
2 files changed, 205 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index 5b8b7d640e70..6fd2b3238e2c 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -3,6 +3,7 @@
/* Based on Christian Brauner's clone3() example */
#define _GNU_SOURCE
+#include <asm/mman.h>
#include <errno.h>
#include <inttypes.h>
#include <linux/types.h>
@@ -11,6 +12,7 @@
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
+#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/un.h>
@@ -19,8 +21,12 @@
#include <sched.h>
#include "../kselftest.h"
+#include "../ksft_shstk.h"
#include "clone3_selftests.h"
+static bool shadow_stack_supported;
+static size_t max_supported_args_size;
+
enum test_mode {
CLONE3_ARGS_NO_TEST,
CLONE3_ARGS_ALL_0,
@@ -28,6 +34,10 @@ enum test_mode {
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG,
CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG,
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG,
+ CLONE3_ARGS_SHADOW_STACK,
+ CLONE3_ARGS_SHADOW_STACK_MISALIGNED,
+ CLONE3_ARGS_SHADOW_STACK_NO_TOKEN,
+ CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY,
};
typedef bool (*filter_function)(void);
@@ -44,6 +54,44 @@ struct test {
filter_function filter;
};
+
+/*
+ * We check for shadow stack support by attempting to use
+ * map_shadow_stack() since features may have been locked by the
+ * dynamic linker resulting in spurious errors when we attempt to
+ * enable on startup. We warn if the enable failed.
+ */
+static void test_shadow_stack_supported(void)
+{
+ long ret;
+
+ ret = syscall(__NR_map_shadow_stack, 0, getpagesize(), 0);
+ if (ret == -1) {
+ ksft_print_msg("map_shadow_stack() not supported\n");
+ } else if ((void *)ret == MAP_FAILED) {
+ ksft_print_msg("Failed to map shadow stack\n");
+ } else {
+ ksft_print_msg("Shadow stack supportd\n");
+ shadow_stack_supported = true;
+
+ if (!shadow_stack_enabled)
+ ksft_print_msg("Mapped but did not enable shadow stack\n");
+ }
+}
+
+static void *get_shadow_stack_page(unsigned long flags)
+{
+ unsigned long long page;
+
+ page = syscall(__NR_map_shadow_stack, 0, getpagesize(), flags);
+ if ((void *)page == MAP_FAILED) {
+ ksft_print_msg("map_shadow_stack() failed: %d\n", errno);
+ return 0;
+ }
+
+ return (void *)page;
+}
+
static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
{
struct __clone_args args = {
@@ -57,6 +105,7 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
} args_ext;
pid_t pid = -1;
+ void *p;
int status;
memset(&args_ext, 0, sizeof(args_ext));
@@ -89,6 +138,26 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
case CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG:
args.exit_signal = 0x00000000000000f0ULL;
break;
+ case CLONE3_ARGS_SHADOW_STACK:
+ p = get_shadow_stack_page(SHADOW_STACK_SET_TOKEN);
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_token = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_MISALIGNED:
+ p = get_shadow_stack_page(SHADOW_STACK_SET_TOKEN);
+ p += getpagesize() - sizeof(void *) - 1;
+ args.shadow_stack_token = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY:
+ p = malloc(getpagesize());
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_token = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_NO_TOKEN:
+ p = get_shadow_stack_page(0);
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_token = (unsigned long long)p;
+ break;
}
memcpy(&args_ext.args, &args, sizeof(struct __clone_args));
@@ -102,7 +171,12 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
if (pid == 0) {
ksft_print_msg("I am the child, my PID is %d\n", getpid());
- _exit(EXIT_SUCCESS);
+ /*
+ * Use a raw syscall to ensure we don't get issues
+ * with manually specified shadow stack and exit handlers.
+ */
+ syscall(__NR_exit, EXIT_SUCCESS);
+ ksft_print_msg("CHILD FAILED TO EXIT PID is %d\n", getpid());
}
ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
@@ -184,6 +258,26 @@ static bool no_timenamespace(void)
return true;
}
+static bool have_shadow_stack(void)
+{
+ if (shadow_stack_supported) {
+ ksft_print_msg("Shadow stack supported\n");
+ return true;
+ }
+
+ return false;
+}
+
+static bool no_shadow_stack(void)
+{
+ if (!shadow_stack_supported) {
+ ksft_print_msg("Shadow stack not supported\n");
+ return true;
+ }
+
+ return false;
+}
+
static size_t page_size_plus_8(void)
{
return getpagesize() + 8;
@@ -327,6 +421,50 @@ static const struct test tests[] = {
.expected = -EINVAL,
.test_mode = CLONE3_ARGS_NO_TEST,
},
+ {
+ .name = "Shadow stack on system with shadow stack",
+ .size = 0,
+ .expected = 0,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with misaligned address",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EINVAL,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_MISALIGNED,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with normal memory",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EFAULT,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with no token",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EINVAL,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_NO_TOKEN,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack on system without shadow stack",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EFAULT,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY,
+ .filter = have_shadow_stack,
+ },
};
int main(int argc, char *argv[])
@@ -334,9 +472,12 @@ int main(int argc, char *argv[])
size_t size;
int i;
+ enable_shadow_stack();
+
ksft_print_header();
ksft_set_plan(ARRAY_SIZE(tests));
test_clone3_supported();
+ test_shadow_stack_supported();
for (i = 0; i < ARRAY_SIZE(tests); i++)
test_clone3(&tests[i]);
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index 939b26c86d42..8151c4fc971a 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -31,12 +31,75 @@ struct __clone_args {
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
+#ifndef CLONE_ARGS_SIZE_VER2
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#endif
+ __aligned_u64 shadow_stack_token;
+#ifndef CLONE_ARGS_SIZE_VER3
+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
+#endif
};
+/*
+ * For architectures with shadow stack support we need to be
+ * absolutely sure that the clone3() syscall will be inline and not a
+ * function call so we open code.
+ */
+#ifdef __x86_64__
+static __always_inline pid_t sys_clone3(struct __clone_args *args, size_t size)
+{
+ register long _num __asm__ ("rax") = __NR_clone3;
+ register long _args __asm__ ("rdi") = (long)(args);
+ register long _size __asm__ ("rsi") = (long)(size);
+ long ret;
+
+ __asm__ volatile (
+ "syscall\n"
+ : "=a"(ret)
+ : "r"(_args), "r"(_size),
+ "0"(_num)
+ : "rcx", "r11", "memory", "cc"
+ );
+
+ if (ret < 0) {
+ errno = -ret;
+ return -1;
+ }
+
+ return ret;
+}
+#elif defined(__aarch64__)
+static __always_inline pid_t sys_clone3(struct __clone_args *args, size_t size)
+{
+ register long _num __asm__ ("x8") = __NR_clone3;
+ register long _args __asm__ ("x0") = (long)(args);
+ register long _size __asm__ ("x1") = (long)(size);
+ register long arg2 __asm__ ("x2") = 0;
+ register long arg3 __asm__ ("x3") = 0;
+ register long arg4 __asm__ ("x4") = 0;
+
+ __asm__ volatile (
+ "svc #0\n"
+ : "=r"(_args)
+ : "r"(_args), "r"(_size),
+ "r"(_num), "r"(arg2),
+ "r"(arg3), "r"(arg4)
+ : "memory", "cc"
+ );
+
+ if ((int)_args < 0) {
+ errno = -((int)_args);
+ return -1;
+ }
+
+ return _args;
+}
+#else
static pid_t sys_clone3(struct __clone_args *args, size_t size)
{
return syscall(__NR_clone3, args, size);
}
+#endif
static inline void test_clone3_supported(void)
{
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 7/8] selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
The clone_args structure is extensible, with the syscall passing in the
length of the structure. Inside the kernel we use copy_struct_from_user()
to read the struct but this has the unfortunate side effect of silently
accepting some overrun in the structure size providing the extra data is
all zeros. This means that we can't discover the clone3() features that
the running kernel supports by simply probing with various struct sizes.
We need to check this for the benefit of test systems which run newer
kselftests on old kernels.
Add a flag which can be set on a test to indicate that clone3() may return
-E2BIG due to the use of newer struct versions. Currently no tests need
this but it will become an issue for testing clone3() support for shadow
stacks, the support for shadow stacks is already present on x86.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index e066b201fa64..5b8b7d640e70 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -39,6 +39,7 @@ struct test {
size_t size;
size_function size_function;
int expected;
+ bool e2big_valid;
enum test_mode test_mode;
filter_function filter;
};
@@ -146,6 +147,11 @@ static void test_clone3(const struct test *test)
ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n",
getpid(), ret, test->expected);
if (ret != test->expected) {
+ if (test->e2big_valid && ret == -E2BIG) {
+ ksft_print_msg("Test reported -E2BIG\n");
+ ksft_test_result_skip("%s\n", test->name);
+ return;
+ }
ksft_print_msg(
"[%d] Result (%d) is different than expected (%d)\n",
getpid(), ret, test->expected);
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 6/8] selftests/clone3: Factor more of main loop into test_clone3()
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
In order to make it easier to add more configuration for the tests and
more support for runtime detection of when tests can be run pass the
structure describing the tests into test_clone3() rather than picking
the arguments out of it and have that function do all the per-test work.
No functional change.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 77 ++++++++++++++++-----------------
1 file changed, 37 insertions(+), 40 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index e61f07973ce5..e066b201fa64 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -30,6 +30,19 @@ enum test_mode {
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG,
};
+typedef bool (*filter_function)(void);
+typedef size_t (*size_function)(void);
+
+struct test {
+ const char *name;
+ uint64_t flags;
+ size_t size;
+ size_function size_function;
+ int expected;
+ enum test_mode test_mode;
+ filter_function filter;
+};
+
static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
{
struct __clone_args args = {
@@ -109,30 +122,40 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
return 0;
}
-static bool test_clone3(uint64_t flags, size_t size, int expected,
- enum test_mode test_mode)
+static void test_clone3(const struct test *test)
{
+ size_t size;
int ret;
+ if (test->filter && test->filter()) {
+ ksft_test_result_skip("%s\n", test->name);
+ return;
+ }
+
+ if (test->size_function)
+ size = test->size_function();
+ else
+ size = test->size;
+
+ ksft_print_msg("Running test '%s'\n", test->name);
+
ksft_print_msg(
"[%d] Trying clone3() with flags %#" PRIx64 " (size %zu)\n",
- getpid(), flags, size);
- ret = call_clone3(flags, size, test_mode);
+ getpid(), test->flags, size);
+ ret = call_clone3(test->flags, size, test->test_mode);
ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n",
- getpid(), ret, expected);
- if (ret != expected) {
+ getpid(), ret, test->expected);
+ if (ret != test->expected) {
ksft_print_msg(
"[%d] Result (%d) is different than expected (%d)\n",
- getpid(), ret, expected);
- return false;
+ getpid(), ret, test->expected);
+ ksft_test_result_fail("%s\n", test->name);
+ return;
}
- return true;
+ ksft_test_result_pass("%s\n", test->name);
}
-typedef bool (*filter_function)(void);
-typedef size_t (*size_function)(void);
-
static bool not_root(void)
{
if (getuid() != 0) {
@@ -160,16 +183,6 @@ static size_t page_size_plus_8(void)
return getpagesize() + 8;
}
-struct test {
- const char *name;
- uint64_t flags;
- size_t size;
- size_function size_function;
- int expected;
- enum test_mode test_mode;
- filter_function filter;
-};
-
static const struct test tests[] = {
{
.name = "simple clone3()",
@@ -319,24 +332,8 @@ int main(int argc, char *argv[])
ksft_set_plan(ARRAY_SIZE(tests));
test_clone3_supported();
- for (i = 0; i < ARRAY_SIZE(tests); i++) {
- if (tests[i].filter && tests[i].filter()) {
- ksft_test_result_skip("%s\n", tests[i].name);
- continue;
- }
-
- if (tests[i].size_function)
- size = tests[i].size_function();
- else
- size = tests[i].size;
-
- ksft_print_msg("Running test '%s'\n", tests[i].name);
-
- ksft_test_result(test_clone3(tests[i].flags, size,
- tests[i].expected,
- tests[i].test_mode),
- "%s\n", tests[i].name);
- }
+ for (i = 0; i < ARRAY_SIZE(tests); i++)
+ test_clone3(&tests[i]);
ksft_finished();
}
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 5/8] selftests/clone3: Remove redundant flushes of output streams
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
Since there were widespread issues with output not being flushed the
kselftest framework was modified to explicitly set the output streams
unbuffered in commit 58e2847ad2e6 ("selftests: line buffer test
program's stdout") so there is no need to explicitly flush in the clone3
tests.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3_selftests.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index eeca8005723f..939b26c86d42 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -35,8 +35,6 @@ struct __clone_args {
static pid_t sys_clone3(struct __clone_args *args, size_t size)
{
- fflush(stdout);
- fflush(stderr);
return syscall(__NR_clone3, args, size);
}
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
Unlike with the normal stack there is no API for configuring the shadow
stack for a new thread, instead the kernel will dynamically allocate a
new shadow stack with the same size as the normal stack. This appears to
be due to the shadow stack series having been in development since
before the more extensible clone3() was added rather than anything more
deliberate.
Add a parameter to clone3() specifying the shadow stack pointer to use
for the new thread, this is inconsistent with the way we specify the
normal stack but during review concerns were expressed about having to
identify where the shadow stack pointer should be placed especially in
cases where the shadow stack has been previously active. If no shadow
stack is specified then the existing implicit allocation behaviour is
maintained.
If a shadow stack pointer is specified then it is required to have an
architecture defined token placed on the stack, this will be consumed by
the new task. If no valid token is present then this will be reported
with -EINVAL. This token prevents new threads being created pointing at
the shadow stack of an existing running thread.
If the architecture does not support shadow stacks the shadow stack
pointer must be not be specified, architectures that do support the
feature are expected to enforce the same requirement on individual
systems that lack shadow stack support.
Update the existing arm64 and x86 implementations to pay attention to
the newly added arguments, in order to maintain compatibility we use the
existing behaviour if no shadow stack is specified. Since we are now
using more fields from the kernel_clone_args we pass that into the
shadow stack code rather than individual fields.
Portions of the x86 architecture code were written by Rick Edgecombe.
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/mm/gcs.c | 53 +++++++++++++++++++++-
arch/x86/include/asm/shstk.h | 11 +++--
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 57 +++++++++++++++++++++---
include/asm-generic/cacheflush.h | 11 +++++
include/linux/sched/task.h | 17 +++++++
include/uapi/linux/sched.h | 9 ++--
kernel/fork.c | 96 +++++++++++++++++++++++++++++++++++-----
8 files changed, 230 insertions(+), 26 deletions(-)
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 1f633a482558..586494f7669b 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -43,8 +43,23 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
{
unsigned long addr, size;
- if (!system_supports_gcs())
+ if (!system_supports_gcs()) {
+ if (args->shadow_stack_token)
+ return -EINVAL;
+
+ return 0;
+ }
+
+ /*
+ * If the user specified a GCS then use it, otherwise fall
+ * back to a default allocation strategy. Validation is done
+ * in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_token) {
+ tsk->thread.gcs_base = 0;
+ tsk->thread.gcs_size = 0;
return 0;
+ }
if (!task_gcs_el0_enabled(tsk))
return 0;
@@ -68,6 +83,42 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
return 0;
}
+static bool gcs_consume_token(struct vm_area_struct *vma, struct page *page,
+ unsigned long user_addr)
+{
+ u64 expected = GCS_CAP(user_addr);
+ u64 *token = page_address(page) + offset_in_page(user_addr);
+
+ if (!cmpxchg_to_user_page(vma, page, user_addr, token, expected, 0))
+ return false;
+ set_page_dirty_lock(page);
+
+ return true;
+}
+
+int arch_shstk_validate_clone(struct task_struct *tsk,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ unsigned long gcspr_el0;
+ int ret = 0;
+
+ /* Ensure that a token written as a result of a pivot is visible */
+ gcsb_dsync();
+
+ gcspr_el0 = args->shadow_stack_token;
+ if (!gcs_consume_token(vma, page, gcspr_el0))
+ return -EINVAL;
+
+ tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
+
+ /* Ensure that our token consumption visible */
+ gcsb_dsync();
+
+ return ret;
+}
+
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
{
unsigned long alloc_size;
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index ba6f2fe43848..827e983430aa 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -6,6 +6,7 @@
#include <linux/types.h>
struct task_struct;
+struct kernel_clone_args;
struct ksignal;
#ifdef CONFIG_X86_USER_SHADOW_STACK
@@ -16,8 +17,8 @@ struct thread_shstk {
long shstk_prctl(struct task_struct *task, int option, unsigned long arg2);
void reset_thread_features(void);
-unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
- unsigned long stack_size);
+unsigned long shstk_alloc_thread_stack(struct task_struct *p,
+ const struct kernel_clone_args *args);
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
@@ -28,8 +29,10 @@ static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
static inline void reset_thread_features(void) {}
static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
- unsigned long clone_flags,
- unsigned long stack_size) { return 0; }
+ const struct kernel_clone_args *args)
+{
+ return 0;
+}
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 962c3ce39323..002b05483c62 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -213,7 +213,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
* is disabled, new_ssp will remain 0, and fpu_clone() will know not to
* update it.
*/
- new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
+ new_ssp = shstk_alloc_thread_stack(p, args);
if (IS_ERR_VALUE(new_ssp))
return PTR_ERR((void *)new_ssp);
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 059685612362..8cce74ee0d96 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -191,18 +191,65 @@ void reset_thread_features(void)
current->thread.features_locked = 0;
}
-unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags,
- unsigned long stack_size)
+int arch_shstk_validate_clone(struct task_struct *t,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ /*
+ * SSP is aligned, so reserved bits and mode bit are a zero, just mark
+ * the token 64-bit.
+ */
+ void *maddr = page_address(page);
+ int offset, token;
+ unsigned long ssp;
+ u64 expected;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ token = args->shadow_stack_token;
+ ssp = token + 8;
+ expected = (ssp + FRAME_SIZE) | BIT(0);
+ offset = offset_in_page(token);
+
+ if (!cmpxchg_to_user_page(vma, page, token, (unsigned long *)(maddr + offset),
+ expected, 0))
+ return -EINVAL;
+ set_page_dirty_lock(page);
+
+ return 0;
+}
+
+unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
struct thread_shstk *shstk = &tsk->thread.shstk;
+ unsigned long clone_flags = args->flags;
unsigned long addr, size;
/*
* If shadow stack is not enabled on the new thread, skip any
- * switch to a new shadow stack.
+ * implicit switch to a new shadow stack and reject attempts to
+ * explicitly specify one.
*/
- if (!features_enabled(ARCH_SHSTK_SHSTK))
+ if (!features_enabled(ARCH_SHSTK_SHSTK)) {
+ if (args->shadow_stack_token)
+ return (unsigned long)ERR_PTR(-EINVAL);
+
return 0;
+ }
+
+ /*
+ * If the user specified a shadow stack then use it, otherwise
+ * fall back to a default allocation strategy. Validation is
+ * done in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_token) {
+ shstk->base = 0;
+ shstk->size = 0;
+ return args->shadow_stack_token + 8;
+ }
/*
* For CLONE_VFORK the child will share the parents shadow stack.
@@ -222,7 +269,7 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long cl
if (!(clone_flags & CLONE_VM))
return 0;
- size = adjust_shstk_size(stack_size);
+ size = adjust_shstk_size(args->stack_size);
addr = alloc_shstk(0, size, 0, false);
if (IS_ERR_VALUE(addr))
return addr;
diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h
index 7ee8a179d103..96cc0c7a5c90 100644
--- a/include/asm-generic/cacheflush.h
+++ b/include/asm-generic/cacheflush.h
@@ -124,4 +124,15 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
} while (0)
#endif
+#ifndef cmpxchg_to_user_page
+#define cmpxchg_to_user_page(vma, page, vaddr, ptr, old, new) \
+({ \
+ bool ret; \
+ \
+ ret = try_cmpxchg(ptr, &old, new); \
+ flush_icache_user_page(vma, page, vaddr, sizeof(*ptr)); \
+ ret; \
+})
+#endif
+
#endif /* _ASM_GENERIC_CACHEFLUSH_H */
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index ca1db4b92c32..c34f3cb68822 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -16,6 +16,7 @@ struct task_struct;
struct rusage;
union thread_union;
struct css_set;
+struct vm_area_struct;
/* All the bits taken by the old clone syscall. */
#define CLONE_LEGACY_FLAGS 0xffffffffULL
@@ -44,6 +45,7 @@ struct kernel_clone_args {
struct cgroup *cgrp;
struct css_set *cset;
unsigned int kill_seq;
+ unsigned long shadow_stack_token;
};
/*
@@ -237,4 +239,19 @@ static inline void task_unlock(struct task_struct *p)
DEFINE_GUARD(task_lock, struct task_struct *, task_lock(_T), task_unlock(_T))
+#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
+int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args);
+#else
+static inline int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_SCHED_TASK_H */
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 359a14cc76a4..9cf5c419e109 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -84,6 +84,7 @@
* kernel's limit of nested PID namespaces.
* @cgroup: If CLONE_INTO_CGROUP is specified set this to
* a file descriptor for the cgroup.
+ * @shadow_stack_token: Pointer to shadow stack token at top of stack.
*
* The structure is versioned by size and thus extensible.
* New struct members must go at the end of the struct and
@@ -101,12 +102,14 @@ struct clone_args {
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
+ __aligned_u64 shadow_stack_token;
};
#endif
-#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
-#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
-#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
/*
* Scheduling policies
diff --git a/kernel/fork.c b/kernel/fork.c
index c4b26cd8998b..98f19bd1f35d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2162,6 +2162,51 @@ static void rv_task_fork(struct task_struct *p)
#define rv_task_fork(p) do {} while (0)
#endif
+static int shstk_validate_clone(struct task_struct *p,
+ struct kernel_clone_args *args)
+{
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ struct page *page;
+ unsigned long addr;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
+ return 0;
+
+ if (!args->shadow_stack_token)
+ return 0;
+
+ mm = get_task_mm(p);
+ if (!mm)
+ return -EFAULT;
+
+ mmap_read_lock(mm);
+
+ addr = untagged_addr_remote(mm, args->shadow_stack_token);
+ page = get_user_page_vma_remote(mm, addr, FOLL_FORCE | FOLL_WRITE,
+ &vma);
+ if (IS_ERR(page)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (!(vma->vm_flags & VM_SHADOW_STACK) ||
+ !(vma->vm_flags & VM_WRITE)) {
+ ret = -EFAULT;
+ goto out_page;
+ }
+
+ ret = arch_shstk_validate_clone(p, vma, page, args);
+
+out_page:
+ put_page(page);
+out:
+ mmap_read_unlock(mm);
+ mmput(mm);
+ return ret;
+}
+
/*
* This creates a new process as a copy of the old one,
* but does not actually start it yet.
@@ -2436,6 +2481,9 @@ __latent_entropy struct task_struct *copy_process(
if (retval)
goto bad_fork_cleanup_namespaces;
retval = copy_thread(p, args);
+ if (retval)
+ goto bad_fork_cleanup_io;
+ retval = shstk_validate_clone(p, args);
if (retval)
goto bad_fork_cleanup_io;
@@ -3002,7 +3050,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER1);
BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=
CLONE_ARGS_SIZE_VER2);
- BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
+ BUILD_BUG_ON(offsetofend(struct clone_args, shadow_stack_token) !=
+ CLONE_ARGS_SIZE_VER3);
+ BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER3);
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
@@ -3035,16 +3085,17 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
return -EINVAL;
*kargs = (struct kernel_clone_args){
- .flags = args.flags,
- .pidfd = u64_to_user_ptr(args.pidfd),
- .child_tid = u64_to_user_ptr(args.child_tid),
- .parent_tid = u64_to_user_ptr(args.parent_tid),
- .exit_signal = args.exit_signal,
- .stack = args.stack,
- .stack_size = args.stack_size,
- .tls = args.tls,
- .set_tid_size = args.set_tid_size,
- .cgroup = args.cgroup,
+ .flags = args.flags,
+ .pidfd = u64_to_user_ptr(args.pidfd),
+ .child_tid = u64_to_user_ptr(args.child_tid),
+ .parent_tid = u64_to_user_ptr(args.parent_tid),
+ .exit_signal = args.exit_signal,
+ .stack = args.stack,
+ .stack_size = args.stack_size,
+ .tls = args.tls,
+ .set_tid_size = args.set_tid_size,
+ .cgroup = args.cgroup,
+ .shadow_stack_token = args.shadow_stack_token,
};
if (args.set_tid &&
@@ -3085,6 +3136,27 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
return true;
}
+/**
+ * clone3_shadow_stack_valid - check and prepare shadow stack
+ * @kargs: kernel clone args
+ *
+ * Verify that shadow stacks are only enabled if supported.
+ */
+static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs)
+{
+ if (!kargs->shadow_stack_token)
+ return true;
+
+ if (!IS_ALIGNED(kargs->shadow_stack_token, sizeof(void *)))
+ return false;
+
+ /*
+ * The architecture must check support on the specific
+ * machine.
+ */
+ return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
+}
+
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
@@ -3107,7 +3179,7 @@ static bool clone3_args_valid(struct kernel_clone_args *kargs)
kargs->exit_signal)
return false;
- if (!clone3_stack_valid(kargs))
+ if (!clone3_stack_valid(kargs) || !clone3_shadow_stack_valid(kargs))
return false;
return true;
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 3/8] selftests: Provide helper header for shadow stack testing
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
While almost all users of shadow stacks should be relying on the dynamic
linker and libc to enable the feature there are several low level test
programs where it is useful to enable without any libc support, allowing
testing without full system enablement. This low level testing is helpful
during bringup of the support itself, and also in enabling coverage by
automated testing without needing all system components in the target root
filesystems to have enablement.
Provide a header with helpers for this purpose, intended for use only by
test programs directly exercising shadow stack interfaces.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/tools/testing/selftests/ksft_shstk.h b/tools/testing/selftests/ksft_shstk.h
new file mode 100644
index 000000000000..fecf91218ea5
--- /dev/null
+++ b/tools/testing/selftests/ksft_shstk.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Helpers for shadow stack enablement, this is intended to only be
+ * used by low level test programs directly exercising interfaces for
+ * working with shadow stacks.
+ *
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#ifndef __KSFT_SHSTK_H
+#define __KSFT_SHSTK_H
+
+#include <asm/mman.h>
+
+/* This is currently only defined for x86 */
+#ifndef SHADOW_STACK_SET_TOKEN
+#define SHADOW_STACK_SET_TOKEN (1ULL << 0)
+#endif
+
+static bool shadow_stack_enabled;
+
+#ifdef __x86_64__
+#define ARCH_SHSTK_ENABLE 0x5001
+#define ARCH_SHSTK_SHSTK (1ULL << 0)
+
+#define ARCH_PRCTL(arg1, arg2) \
+({ \
+ long _ret; \
+ register long _num asm("eax") = __NR_arch_prctl; \
+ register long _arg1 asm("rdi") = (long)(arg1); \
+ register long _arg2 asm("rsi") = (long)(arg2); \
+ \
+ asm volatile ( \
+ "syscall\n" \
+ : "=a"(_ret) \
+ : "r"(_arg1), "r"(_arg2), \
+ "0"(_num) \
+ : "rcx", "r11", "memory", "cc" \
+ ); \
+ _ret; \
+})
+
+#define ENABLE_SHADOW_STACK
+static __always_inline void enable_shadow_stack(void)
+{
+ int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifdef __aarch64__
+#define PR_SET_SHADOW_STACK_STATUS 75
+# define PR_SHADOW_STACK_ENABLE (1UL << 0)
+
+#define my_syscall2(num, arg1, arg2) \
+({ \
+ register long _num __asm__ ("x8") = (num); \
+ register long _arg1 __asm__ ("x0") = (long)(arg1); \
+ register long _arg2 __asm__ ("x1") = (long)(arg2); \
+ register long _arg3 __asm__ ("x2") = 0; \
+ register long _arg4 __asm__ ("x3") = 0; \
+ register long _arg5 __asm__ ("x4") = 0; \
+ \
+ __asm__ volatile ( \
+ "svc #0\n" \
+ : "=r"(_arg1) \
+ : "r"(_arg1), "r"(_arg2), \
+ "r"(_arg3), "r"(_arg4), \
+ "r"(_arg5), "r"(_num) \
+ : "memory", "cc" \
+ ); \
+ _arg1; \
+})
+
+#define ENABLE_SHADOW_STACK
+static __always_inline void enable_shadow_stack(void)
+{
+ int ret;
+
+ ret = my_syscall2(__NR_prctl, PR_SET_SHADOW_STACK_STATUS,
+ PR_SHADOW_STACK_ENABLE);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifndef __NR_map_shadow_stack
+#define __NR_map_shadow_stack 453
+#endif
+
+#ifndef ENABLE_SHADOW_STACK
+static inline void enable_shadow_stack(void) { }
+#endif
+
+#endif
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 2/8] Documentation: userspace-api: Add shadow stack API documentation
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
There are a number of architectures with shadow stack features which we are
presenting to userspace with as consistent an API as we can (though there
are some architecture specifics). Especially given that there are some
important considerations for userspace code interacting directly with the
feature let's provide some documentation covering the common aspects.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
Reviewed-by: Deepak Gupta <debug@rivosinc.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 44 ++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index b8c73be4fb11..0167e59b541e 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -62,6 +62,7 @@ Everything else
ELF
netlink/index
+ shadow_stack
sysfs-platform_profile
vduse
futex2
diff --git a/Documentation/userspace-api/shadow_stack.rst b/Documentation/userspace-api/shadow_stack.rst
new file mode 100644
index 000000000000..65c665496624
--- /dev/null
+++ b/Documentation/userspace-api/shadow_stack.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+Shadow Stacks
+=============
+
+Introduction
+============
+
+Several architectures have features which provide backward edge
+control flow protection through a hardware maintained stack, only
+writeable by userspace through very limited operations. This feature
+is referred to as shadow stacks on Linux, on x86 it is part of Intel
+Control Enforcement Technology (CET), on arm64 it is Guarded Control
+Stacks feature (FEAT_GCS) and for RISC-V it is the Zicfiss extension.
+It is expected that this feature will normally be managed by the
+system dynamic linker and libc in ways broadly transparent to
+application code, this document covers interfaces and considerations.
+
+
+Enabling
+========
+
+Shadow stacks default to disabled when a userspace process is
+executed, they can be enabled for the current thread with a syscall:
+
+ - For x86 the ARCH_SHSTK_ENABLE arch_prctl()
+ - For other architectures the PR_SET_SHADOW_STACK_ENABLE prctl()
+
+It is expected that this will normally be done by the dynamic linker.
+Any new threads created by a thread with shadow stacks enabled will
+themselves have shadow stacks enabled.
+
+
+Enablement considerations
+=========================
+
+- Returning from the function that enables shadow stacks without first
+ disabling them will cause a shadow stack exception. This includes
+ any syscall wrapper or other library functions, the syscall will need
+ to be inlined.
+- A lock feature allows userspace to prevent disabling of shadow stacks.
+- Those that change the stack context like longjmp() or use of ucontext
+ changes on signal return will need support from libc.
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 1/8] arm64/gcs: Return a success value from gcs_alloc_thread_stack()
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook
In-Reply-To: <20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org>
Currently as a result of templating from x86 code gcs_alloc_thread_stack()
returns a pointer as an unsigned int however on arm64 we don't actually use
this pointer value as anything other than a pass/fail flag. Simplify the
interface to just return an int with 0 on success and a negative error code
on failure.
Acked-by: Deepak Gupta <debug@rivosinc.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/include/asm/gcs.h | 8 ++++----
arch/arm64/kernel/process.c | 8 ++++----
arch/arm64/mm/gcs.c | 8 ++++----
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index f50660603ecf..d8923b5f03b7 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -64,8 +64,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
void gcs_set_el0_mode(struct task_struct *task);
void gcs_free(struct task_struct *task);
void gcs_preserve_current_state(void);
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args);
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args);
static inline int gcs_check_locked(struct task_struct *task,
unsigned long new_val)
@@ -91,8 +91,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
static inline void gcs_set_el0_mode(struct task_struct *task) { }
static inline void gcs_free(struct task_struct *task) { }
static inline void gcs_preserve_current_state(void) { }
-static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+static inline int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
return -ENOTSUPP;
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 42faebb7b712..45130ea7ea6e 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -297,7 +297,7 @@ static void flush_gcs(void)
static int copy_thread_gcs(struct task_struct *p,
const struct kernel_clone_args *args)
{
- unsigned long gcs;
+ int ret;
if (!system_supports_gcs())
return 0;
@@ -305,9 +305,9 @@ static int copy_thread_gcs(struct task_struct *p,
p->thread.gcs_base = 0;
p->thread.gcs_size = 0;
- gcs = gcs_alloc_thread_stack(p, args);
- if (IS_ERR_VALUE(gcs))
- return PTR_ERR((void *)gcs);
+ ret = gcs_alloc_thread_stack(p, args);
+ if (ret != 0)
+ return ret;
p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 5c46ec527b1c..1f633a482558 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -38,8 +38,8 @@ static unsigned long gcs_size(unsigned long size)
return max(PAGE_SIZE, size);
}
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
unsigned long addr, size;
@@ -59,13 +59,13 @@ unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
size = gcs_size(size);
addr = alloc_gcs(0, size);
if (IS_ERR_VALUE(addr))
- return addr;
+ return PTR_ERR((void *)addr);
tsk->thread.gcs_base = addr;
tsk->thread.gcs_size = size;
tsk->thread.gcspr_el0 = addr + size - sizeof(u64);
- return addr;
+ return 0;
}
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v16 0/8] fork: Support shadow stacks in clone3()
From: Mark Brown @ 2025-04-15 23:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
The kernel has recently added support for shadow stacks, currently
x86 only using their CET feature but both arm64 and RISC-V have
equivalent features (GCS and Zicfiss respectively), I am actively
working on GCS[1]. With shadow stacks the hardware maintains an
additional stack containing only the return addresses for branch
instructions which is not generally writeable by userspace and ensures
that any returns are to the recorded addresses. This provides some
protection against ROP attacks and making it easier to collect call
stacks. These shadow stacks are allocated in the address space of the
userspace process.
Our API for shadow stacks does not currently offer userspace any
flexiblity for managing the allocation of shadow stacks for newly
created threads, instead the kernel allocates a new shadow stack with
the same size as the normal stack whenever a thread is created with the
feature enabled. The stacks allocated in this way are freed by the
kernel when the thread exits or shadow stacks are disabled for the
thread. This lack of flexibility and control isn't ideal, in the vast
majority of cases the shadow stack will be over allocated and the
implicit allocation and deallocation is not consistent with other
interfaces. As far as I can tell the interface is done in this manner
mainly because the shadow stack patches were in development since before
clone3() was implemented.
Since clone3() is readily extensible let's add support for specifying a
shadow stack when creating a new thread or process, keeping the current
implicit allocation behaviour if one is not specified either with
clone3() or through the use of clone(). The user must provide a shadow
stack pointer, this must point to memory mapped for use as a shadow
stackby map_shadow_stack() with an architecture specified shadow stack
token at the top of the stack.
Please note that the x86 portions of this code are build tested only, I
don't appear to have a system that can run CET available to me.
[1] https://lore.kernel.org/linux-arm-kernel/20241001-arm64-gcs-v13-0-222b78d87eee@kernel.org/T/#mc58f97f27461749ccf400ebabf6f9f937116a86b
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v16:
- Rebase onto v6.15-rc2.
- Roll in fixes from x86 testing from Rick Edgecombe.
- Rework so that the argument is shadow_stack_token.
- Link to v15: https://lore.kernel.org/r/20250408-clone3-shadow-stack-v15-0-3fa245c6e3be@kernel.org
Changes in v15:
- Rebase onto v6.15-rc1.
- Link to v14: https://lore.kernel.org/r/20250206-clone3-shadow-stack-v14-0-805b53af73b9@kernel.org
Changes in v14:
- Rebase onto v6.14-rc1.
- Link to v13: https://lore.kernel.org/r/20241203-clone3-shadow-stack-v13-0-93b89a81a5ed@kernel.org
Changes in v13:
- Rebase onto v6.13-rc1.
- Link to v12: https://lore.kernel.org/r/20241031-clone3-shadow-stack-v12-0-7183eb8bee17@kernel.org
Changes in v12:
- Add the regular prctl() to the userspace API document since arm64
support is queued in -next.
- Link to v11: https://lore.kernel.org/r/20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org
Changes in v11:
- Rebase onto arm64 for-next/gcs, which is based on v6.12-rc1, and
integrate arm64 support.
- Rework the interface to specify a shadow stack pointer rather than a
base and size like we do for the regular stack.
- Link to v10: https://lore.kernel.org/r/20240821-clone3-shadow-stack-v10-0-06e8797b9445@kernel.org
Changes in v10:
- Integrate fixes & improvements for the x86 implementation from Rick
Edgecombe.
- Require that the shadow stack be VM_WRITE.
- Require that the shadow stack base and size be sizeof(void *) aligned.
- Clean up trailing newline.
- Link to v9: https://lore.kernel.org/r/20240819-clone3-shadow-stack-v9-0-962d74f99464@kernel.org
Changes in v9:
- Pull token validation earlier and report problems with an error return
to parent rather than signal delivery to the child.
- Verify that the top of the supplied shadow stack is VM_SHADOW_STACK.
- Rework token validation to only do the page mapping once.
- Drop no longer needed support for testing for signals in selftest.
- Fix typo in comments.
- Link to v8: https://lore.kernel.org/r/20240808-clone3-shadow-stack-v8-0-0acf37caf14c@kernel.org
Changes in v8:
- Fix token verification with user specified shadow stack.
- Don't track user managed shadow stacks for child processes.
- Link to v7: https://lore.kernel.org/r/20240731-clone3-shadow-stack-v7-0-a9532eebfb1d@kernel.org
Changes in v7:
- Rebase onto v6.11-rc1.
- Typo fixes.
- Link to v6: https://lore.kernel.org/r/20240623-clone3-shadow-stack-v6-0-9ee7783b1fb9@kernel.org
Changes in v6:
- Rebase onto v6.10-rc3.
- Ensure we don't try to free the parent shadow stack in error paths of
x86 arch code.
- Spelling fixes in userspace API document.
- Additional cleanups and improvements to the clone3() tests to support
the shadow stack tests.
- Link to v5: https://lore.kernel.org/r/20240203-clone3-shadow-stack-v5-0-322c69598e4b@kernel.org
Changes in v5:
- Rebase onto v6.8-rc2.
- Rework ABI to have the user allocate the shadow stack memory with
map_shadow_stack() and a token.
- Force inlining of the x86 shadow stack enablement.
- Move shadow stack enablement out into a shared header for reuse by
other tests.
- Link to v4: https://lore.kernel.org/r/20231128-clone3-shadow-stack-v4-0-8b28ffe4f676@kernel.org
Changes in v4:
- Formatting changes.
- Use a define for minimum shadow stack size and move some basic
validation to fork.c.
- Link to v3: https://lore.kernel.org/r/20231120-clone3-shadow-stack-v3-0-a7b8ed3e2acc@kernel.org
Changes in v3:
- Rebase onto v6.7-rc2.
- Remove stale shadow_stack in internal kargs.
- If a shadow stack is specified unconditionally use it regardless of
CLONE_ parameters.
- Force enable shadow stacks in the selftest.
- Update changelogs for RISC-V feature rename.
- Link to v2: https://lore.kernel.org/r/20231114-clone3-shadow-stack-v2-0-b613f8681155@kernel.org
Changes in v2:
- Rebase onto v6.7-rc1.
- Remove ability to provide preallocated shadow stack, just specify the
desired size.
- Link to v1: https://lore.kernel.org/r/20231023-clone3-shadow-stack-v1-0-d867d0b5d4d0@kernel.org
---
Mark Brown (8):
arm64/gcs: Return a success value from gcs_alloc_thread_stack()
Documentation: userspace-api: Add shadow stack API documentation
selftests: Provide helper header for shadow stack testing
fork: Add shadow stack support to clone3()
selftests/clone3: Remove redundant flushes of output streams
selftests/clone3: Factor more of main loop into test_clone3()
selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
selftests/clone3: Test shadow stack support
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 44 +++++
arch/arm64/include/asm/gcs.h | 8 +-
arch/arm64/kernel/process.c | 8 +-
arch/arm64/mm/gcs.c | 61 +++++-
arch/x86/include/asm/shstk.h | 11 +-
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 57 +++++-
include/asm-generic/cacheflush.h | 11 ++
include/linux/sched/task.h | 17 ++
include/uapi/linux/sched.h | 9 +-
kernel/fork.c | 96 +++++++--
tools/testing/selftests/clone3/clone3.c | 226 ++++++++++++++++++----
tools/testing/selftests/clone3/clone3_selftests.h | 65 ++++++-
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++
15 files changed, 633 insertions(+), 81 deletions(-)
---
base-commit: 8ffd015db85fea3e15a77027fda6c02ced4d2444
change-id: 20231019-clone3-shadow-stack-15d40d2bf536
Best regards,
--
Mark Brown <broonie@kernel.org>
^ permalink raw reply
* [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time
From: Darrick J. Wong @ 2025-04-15 22:36 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415121425.4146847-15-john.g.garry@oracle.com>
From: Darrick J. Wong <djwong@kernel.org>
Introduce a mount option to allow sysadmins to specify the maximum size
of an atomic write. If the filesystem can work with the supplied value,
that becomes the new guaranteed maximum.
The value mustn't be too big for the existing filesystem geometry (max
write size, max AG/rtgroup size). We dynamically recompute the
tr_atomic_write transaction reservation based on the given block size,
check that the current log size isn't less than the new minimum log size
constraints, and set a new maximum.
The actual software atomic write max is still computed based off of
tr_atomic_ioend the same way it has for the past few commits.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
v7.1: make all the tweaks I already complained about here
---
fs/xfs/libxfs/xfs_trans_resv.h | 2 +
fs/xfs/xfs_mount.h | 6 ++++
fs/xfs/xfs_trace.h | 33 ++++++++++++++++++++
Documentation/admin-guide/xfs.rst | 11 +++++++
fs/xfs/libxfs/xfs_trans_resv.c | 53 +++++++++++++++++++++++++++++++++
fs/xfs/xfs_mount.c | 59 ++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_super.c | 60 ++++++++++++++++++++++++++++++++++++-
7 files changed, 222 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index a6d303b836883f..ea50a239c31107 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -122,5 +122,7 @@ unsigned int xfs_calc_write_reservation_minlogsize(struct xfs_mount *mp);
unsigned int xfs_calc_qm_dqalloc_reservation_minlogsize(struct xfs_mount *mp);
xfs_extlen_t xfs_calc_max_atomic_write_fsblocks(struct xfs_mount *mp);
+bool xfs_calc_atomic_write_reservation(struct xfs_mount *mp,
+ xfs_extlen_t blockcount);
#endif /* __XFS_TRANS_RESV_H__ */
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index c0eff3adfa31f6..68e2acc00b5321 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -236,6 +236,9 @@ typedef struct xfs_mount {
bool m_update_sb; /* sb needs update in mount */
unsigned int m_max_open_zones;
+ /* max_atomic_write mount option value */
+ unsigned long long m_awu_max_bytes;
+
/*
* Bitsets of per-fs metadata that have been checked and/or are sick.
* Callers must hold m_sb_lock to access these two fields.
@@ -798,4 +801,7 @@ static inline void xfs_mod_sb_delalloc(struct xfs_mount *mp, int64_t delta)
percpu_counter_add(&mp->m_delalloc_blks, delta);
}
+int xfs_set_max_atomic_write_opt(struct xfs_mount *mp,
+ unsigned long long new_max_bytes);
+
#endif /* __XFS_MOUNT_H__ */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 24d73e9bbe83f4..d41885f1efe25b 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -230,6 +230,39 @@ TRACE_EVENT(xfs_calc_max_atomic_write_fsblocks,
__entry->blockcount)
);
+TRACE_EVENT(xfs_calc_max_atomic_write_reservation,
+ TP_PROTO(struct xfs_mount *mp, unsigned int per_intent,
+ unsigned int step_size, unsigned int blockcount,
+ unsigned int min_logblocks, unsigned int logres),
+ TP_ARGS(mp, per_intent, step_size, blockcount, min_logblocks, logres),
+ TP_STRUCT__entry(
+ __field(dev_t, dev)
+ __field(unsigned int, per_intent)
+ __field(unsigned int, step_size)
+ __field(unsigned int, blockcount)
+ __field(unsigned int, min_logblocks)
+ __field(unsigned int, cur_logblocks)
+ __field(unsigned int, logres)
+ ),
+ TP_fast_assign(
+ __entry->dev = mp->m_super->s_dev;
+ __entry->per_intent = per_intent;
+ __entry->step_size = step_size;
+ __entry->blockcount = blockcount;
+ __entry->min_logblocks = min_logblocks;
+ __entry->cur_logblocks = mp->m_sb.sb_logblocks;
+ __entry->logres = logres;
+ ),
+ TP_printk("dev %d:%d per_intent %u step_size %u blockcount %u min_logblocks %u logblocks %u logres %u",
+ MAJOR(__entry->dev), MINOR(__entry->dev),
+ __entry->per_intent,
+ __entry->step_size,
+ __entry->blockcount,
+ __entry->min_logblocks,
+ __entry->cur_logblocks,
+ __entry->logres)
+);
+
TRACE_EVENT(xlog_intent_recovery_failed,
TP_PROTO(struct xfs_mount *mp, const struct xfs_defer_op_type *ops,
int error),
diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index b67772cf36d6dc..0f6e5d4784f9c3 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -143,6 +143,17 @@ When mounting an XFS filesystem, the following options are accepted.
optional, and the log section can be separate from the data
section or contained within it.
+ max_atomic_write=value
+ Set the maximum size of an atomic write. The size may be
+ specified in bytes, in kilobytes with a "k" suffix, in megabytes
+ with a "m" suffix, or in gigabytes with a "g" suffix. The size
+ cannot be larger than the maximum write size, larger than the
+ size of any allocation group, or larger than the size of a
+ remapping operation that the log can complete atomically.
+
+ The default value is to set the maximum I/O completion size
+ to allow each CPU to handle one at a time.
+
noalign
Data allocations will not be aligned at stripe unit
boundaries. This is only relevant to filesystems created
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index f530aa5d72f552..48e75c7ba2bb69 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -1475,3 +1475,56 @@ xfs_calc_max_atomic_write_fsblocks(
return ret;
}
+
+/*
+ * Compute the log reservation needed to complete an atomic write of a given
+ * number of blocks. Worst case, each block requires separate handling.
+ * Returns true if the blockcount is supported, false otherwise.
+ */
+bool
+xfs_calc_atomic_write_reservation(
+ struct xfs_mount *mp,
+ xfs_extlen_t blockcount)
+{
+ struct xfs_trans_res *curr_res = &M_RES(mp)->tr_atomic_ioend;
+ unsigned int per_intent, step_size;
+ unsigned int logres;
+ uint old_logres =
+ M_RES(mp)->tr_atomic_ioend.tr_logres;
+ int min_logblocks;
+
+ /*
+ * If the caller doesn't ask for a specific atomic write size, then
+ * we'll use conservatively use tr_itruncate as the basis for computing
+ * a reasonable maximum.
+ */
+ if (blockcount == 0) {
+ curr_res->tr_logres = M_RES(mp)->tr_itruncate.tr_logres;
+ return true;
+ }
+
+ /* Untorn write completions require out of place write remapping */
+ if (!xfs_has_reflink(mp))
+ return false;
+
+ per_intent = xfs_calc_atomic_write_ioend_geometry(mp, &step_size);
+
+ if (check_mul_overflow(blockcount, per_intent, &logres))
+ return false;
+ if (check_add_overflow(logres, step_size, &logres))
+ return false;
+
+ curr_res->tr_logres = logres;
+ min_logblocks = xfs_log_calc_minimum_size(mp);
+
+ trace_xfs_calc_max_atomic_write_reservation(mp, per_intent, step_size,
+ blockcount, min_logblocks, curr_res->tr_logres);
+
+ if (min_logblocks > mp->m_sb.sb_logblocks) {
+ /* Log too small, revert changes. */
+ curr_res->tr_logres = old_logres;
+ return false;
+ }
+
+ return true;
+}
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index cec202cf7803d8..1eda18dfb1f667 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -737,6 +737,61 @@ xfs_compute_atomic_write_unit_max(
max_agsize, max_rgsize);
}
+/*
+ * Try to set the atomic write maximum to a new value that we got from
+ * userspace via mount option.
+ */
+int
+xfs_set_max_atomic_write_opt(
+ struct xfs_mount *mp,
+ unsigned long long new_max_bytes)
+{
+ xfs_filblks_t new_max_fsbs = XFS_B_TO_FSBT(mp, new_max_bytes);
+
+ if (new_max_bytes) {
+ xfs_extlen_t max_write_fsbs =
+ rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
+ xfs_extlen_t max_group_fsbs =
+ max(mp->m_groups[XG_TYPE_AG].blocks,
+ mp->m_groups[XG_TYPE_RTG].blocks);
+
+ ASSERT(max_write_fsbs <= U32_MAX);
+
+ if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
+ xfs_warn(mp,
+ "max atomic write size of %llu bytes not aligned with fsblock",
+ new_max_bytes);
+ return -EINVAL;
+ }
+
+ if (new_max_fsbs > max_write_fsbs) {
+ xfs_warn(mp,
+ "max atomic write size of %lluk cannot be larger than max write size %lluk",
+ new_max_bytes >> 10,
+ XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
+ return -EINVAL;
+ }
+
+ if (new_max_fsbs > max_group_fsbs) {
+ xfs_warn(mp,
+ "max atomic write size of %lluk cannot be larger than allocation group size %lluk",
+ new_max_bytes >> 10,
+ XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
+ return -EINVAL;
+ }
+ }
+
+ if (!xfs_calc_atomic_write_reservation(mp, new_max_fsbs)) {
+ xfs_warn(mp,
+ "cannot support completing atomic writes of %lluk",
+ new_max_bytes >> 10);
+ return -EINVAL;
+ }
+
+ xfs_compute_atomic_write_unit_max(mp);
+ return 0;
+}
+
/* Compute maximum possible height for realtime btree types for this fs. */
static inline void
xfs_rtbtree_compute_maxlevels(
@@ -1158,7 +1213,9 @@ xfs_mountfs(
* derived from transaction reservations, so we must do this after the
* log is fully initialized.
*/
- xfs_compute_atomic_write_unit_max(mp);
+ error = xfs_set_max_atomic_write_opt(mp, mp->m_awu_max_bytes);
+ if (error)
+ goto out_agresv;
return 0;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b2dd0c0bf50979..9f422bcf651801 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -111,7 +111,7 @@ enum {
Opt_prjquota, Opt_uquota, Opt_gquota, Opt_pquota,
Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum, Opt_max_open_zones,
- Opt_lifetime, Opt_nolifetime,
+ Opt_lifetime, Opt_nolifetime, Opt_max_atomic_write,
};
static const struct fs_parameter_spec xfs_fs_parameters[] = {
@@ -159,6 +159,7 @@ static const struct fs_parameter_spec xfs_fs_parameters[] = {
fsparam_u32("max_open_zones", Opt_max_open_zones),
fsparam_flag("lifetime", Opt_lifetime),
fsparam_flag("nolifetime", Opt_nolifetime),
+ fsparam_string("max_atomic_write", Opt_max_atomic_write),
{}
};
@@ -241,6 +242,9 @@ xfs_fs_show_options(
if (mp->m_max_open_zones)
seq_printf(m, ",max_open_zones=%u", mp->m_max_open_zones);
+ if (mp->m_awu_max_bytes)
+ seq_printf(m, ",max_atomic_write=%lluk",
+ mp->m_awu_max_bytes >> 10);
return 0;
}
@@ -1334,6 +1338,42 @@ suffix_kstrtoint(
return ret;
}
+static int
+suffix_kstrtoull(
+ const char *s,
+ unsigned int base,
+ unsigned long long *res)
+{
+ int last, shift_left_factor = 0;
+ unsigned long long _res;
+ char *value;
+ int ret = 0;
+
+ value = kstrdup(s, GFP_KERNEL);
+ if (!value)
+ return -ENOMEM;
+
+ last = strlen(value) - 1;
+ if (value[last] == 'K' || value[last] == 'k') {
+ shift_left_factor = 10;
+ value[last] = '\0';
+ }
+ if (value[last] == 'M' || value[last] == 'm') {
+ shift_left_factor = 20;
+ value[last] = '\0';
+ }
+ if (value[last] == 'G' || value[last] == 'g') {
+ shift_left_factor = 30;
+ value[last] = '\0';
+ }
+
+ if (kstrtoull(value, base, &_res))
+ ret = -EINVAL;
+ kfree(value);
+ *res = _res << shift_left_factor;
+ return ret;
+}
+
static inline void
xfs_fs_warn_deprecated(
struct fs_context *fc,
@@ -1518,6 +1558,14 @@ xfs_fs_parse_param(
case Opt_nolifetime:
parsing_mp->m_features |= XFS_FEAT_NOLIFETIME;
return 0;
+ case Opt_max_atomic_write:
+ if (suffix_kstrtoull(param->string, 10,
+ &parsing_mp->m_awu_max_bytes)) {
+ xfs_warn(parsing_mp,
+ "max atomic write size must be positive integer");
+ return -EINVAL;
+ }
+ return 0;
default:
xfs_warn(parsing_mp, "unknown mount option [%s].", param->key);
return -EINVAL;
@@ -2114,6 +2162,16 @@ xfs_fs_reconfigure(
if (error)
return error;
+ /* Validate new max_atomic_write option before making other changes */
+ if (mp->m_awu_max_bytes != new_mp->m_awu_max_bytes) {
+ error = xfs_set_max_atomic_write_opt(mp,
+ new_mp->m_awu_max_bytes);
+ if (error)
+ return error;
+
+ mp->m_awu_max_bytes = new_mp->m_awu_max_bytes;
+ }
+
/* inode32 -> inode64 */
if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
mp->m_features &= ~XFS_FEAT_SMALL_INUMS;
^ permalink raw reply related
* Re: [PATCH v7 09/14] xfs: add large atomic writes checks in xfs_direct_write_iomap_begin()
From: John Garry @ 2025-04-15 17:46 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415173439.GU25675@frogsfrogsfrogs>
On 15/04/2025 18:34, Darrick J. Wong wrote:
>> + /*
>> + * Spanning multiple extents would mean that multiple BIOs would be
>> + * issued, and so would lose atomicity required for REQ_ATOMIC-based
>> + * atomics.
>> + */
>> + if (!imap_spans_range(imap, offset_fsb, end_fsb))
>> + return false;
>> +
>> + /*
>> + * The ->iomap_begin caller should ensure this, but check anyway.
>> + */
>> + if (len > xfs_inode_buftarg(ip)->bt_bdev_awu_max)
>> + return false;
> This needs to check len against bt_bdev_awu_min so that we don't submit
> too-short atomic writes to the hardware.
Right, let me check this.
I think that we should only support sane HW which can write 1x FS block
or more.
> Let's say that the hardware
> minimum is 32k and the fsblock size is 4k. XFS can perform an out of
> place write for 4k-16k writes, but right now we'll just throw invalid
> commands at the bdev, and it'll return EINVAL.
>
> /me wonders if statx should grow a atomic_write_unit_min_opt field
> too, unless everyone in block layer land is convinced that awu_min will
> always match lbasize? (I probably missed that conversation)
Nothing states that it should (match lbasize), but again HW which can
only write >1 FS block is something which I don't want to support (yet).
Thanks,
John
^ permalink raw reply
* Re: [PATCH v7 09/14] xfs: add large atomic writes checks in xfs_direct_write_iomap_begin()
From: Darrick J. Wong @ 2025-04-15 17:34 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415121425.4146847-10-john.g.garry@oracle.com>
On Tue, Apr 15, 2025 at 12:14:20PM +0000, John Garry wrote:
> For when large atomic writes (> 1x FS block) are supported, there will be
> various occasions when HW offload may not be possible.
>
> Such instances include:
> - unaligned extent mapping wrt write length
> - extent mappings which do not cover the full write, e.g. the write spans
> sparse or mixed-mapping extents
> - the write length is greater than HW offload can support
>
> In those cases, we need to fallback to the CoW-based atomic write mode. For
> this, report special code -ENOPROTOOPT to inform the caller that HW
> offload-based method is not possible.
>
> In addition to the occasions mentioned, if the write covers an unallocated
> range, we again judge that we need to rely on the CoW-based method when we
> would need to allocate anything more than 1x block. This is because if we
> allocate less blocks that is required for the write, then again HW
> offload-based method would not be possible. So we are taking a pessimistic
> approach to writes covering unallocated space.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> fs/xfs/xfs_iomap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 049655ebc3f7..02bb8257ea24 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -798,6 +798,41 @@ imap_spans_range(
> return true;
> }
>
> +static bool
> +xfs_bmap_hw_atomic_write_possible(
> + struct xfs_inode *ip,
> + struct xfs_bmbt_irec *imap,
> + xfs_fileoff_t offset_fsb,
> + xfs_fileoff_t end_fsb)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + xfs_fsize_t len = XFS_FSB_TO_B(mp, end_fsb - offset_fsb);
> +
> + /*
> + * atomic writes are required to be naturally aligned for disk blocks,
> + * which ensures that we adhere to block layer rules that we won't
> + * straddle any boundary or violate write alignment requirement.
> + */
> + if (!IS_ALIGNED(imap->br_startblock, imap->br_blockcount))
> + return false;
> +
> + /*
> + * Spanning multiple extents would mean that multiple BIOs would be
> + * issued, and so would lose atomicity required for REQ_ATOMIC-based
> + * atomics.
> + */
> + if (!imap_spans_range(imap, offset_fsb, end_fsb))
> + return false;
> +
> + /*
> + * The ->iomap_begin caller should ensure this, but check anyway.
> + */
> + if (len > xfs_inode_buftarg(ip)->bt_bdev_awu_max)
> + return false;
This needs to check len against bt_bdev_awu_min so that we don't submit
too-short atomic writes to the hardware. Let's say that the hardware
minimum is 32k and the fsblock size is 4k. XFS can perform an out of
place write for 4k-16k writes, but right now we'll just throw invalid
commands at the bdev, and it'll return EINVAL.
/me wonders if statx should grow a atomic_write_unit_min_opt field
too, unless everyone in block layer land is convinced that awu_min will
always match lbasize? (I probably missed that conversation)
--D
> +
> + return true;
> +}
> +
> static int
> xfs_direct_write_iomap_begin(
> struct inode *inode,
> @@ -812,9 +847,11 @@ xfs_direct_write_iomap_begin(
> struct xfs_bmbt_irec imap, cmap;
> xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset);
> xfs_fileoff_t end_fsb = xfs_iomap_end_fsb(mp, offset, length);
> + xfs_fileoff_t orig_end_fsb = end_fsb;
> int nimaps = 1, error = 0;
> bool shared = false;
> u16 iomap_flags = 0;
> + bool needs_alloc;
> unsigned int lockmode;
> u64 seq;
>
> @@ -875,13 +912,37 @@ xfs_direct_write_iomap_begin(
> (flags & IOMAP_DIRECT) || IS_DAX(inode));
> if (error)
> goto out_unlock;
> - if (shared)
> + if (shared) {
> + if ((flags & IOMAP_ATOMIC) &&
> + !xfs_bmap_hw_atomic_write_possible(ip, &cmap,
> + offset_fsb, end_fsb)) {
> + error = -ENOPROTOOPT;
> + goto out_unlock;
> + }
> goto out_found_cow;
> + }
> end_fsb = imap.br_startoff + imap.br_blockcount;
> length = XFS_FSB_TO_B(mp, end_fsb) - offset;
> }
>
> - if (imap_needs_alloc(inode, flags, &imap, nimaps))
> + needs_alloc = imap_needs_alloc(inode, flags, &imap, nimaps);
> +
> + if (flags & IOMAP_ATOMIC) {
> + error = -ENOPROTOOPT;
> + /*
> + * If we allocate less than what is required for the write
> + * then we may end up with multiple extents, which means that
> + * REQ_ATOMIC-based cannot be used, so avoid this possibility.
> + */
> + if (needs_alloc && orig_end_fsb - offset_fsb > 1)
> + goto out_unlock;
> +
> + if (!xfs_bmap_hw_atomic_write_possible(ip, &imap, offset_fsb,
> + orig_end_fsb))
> + goto out_unlock;
> + }
> +
> + if (needs_alloc)
> goto allocate_blocks;
>
> /*
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [PATCH v7 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time
From: Darrick J. Wong @ 2025-04-15 16:55 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415121425.4146847-15-john.g.garry@oracle.com>
On Tue, Apr 15, 2025 at 12:14:25PM +0000, John Garry wrote:
> From: "Darrick J. Wong" <djwong@kernel.org>
>
> Introduce a mount option to allow sysadmins to specify the maximum size
> of an atomic write. When this happens, we dynamically recompute the
> tr_atomic_write transaction reservation based on the given block size,
> and then check that we don't violate any of the minimum log size
> constraints.
>
> The actual software atomic write max is still computed based off of
> tr_atomic the same way it has for the past few commits.
>
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> Documentation/admin-guide/xfs.rst | 8 +++++
> fs/xfs/libxfs/xfs_trans_resv.c | 54 +++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_trans_resv.h | 1 +
> fs/xfs/xfs_mount.c | 8 ++++-
> fs/xfs/xfs_mount.h | 5 +++
> fs/xfs/xfs_super.c | 28 +++++++++++++++-
> fs/xfs/xfs_trace.h | 33 +++++++++++++++++++
> 7 files changed, 135 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index b67772cf36d6..715019ec4f24 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -143,6 +143,14 @@ When mounting an XFS filesystem, the following options are accepted.
> optional, and the log section can be separate from the data
> section or contained within it.
>
> + max_atomic_write=value
> + Set the maximum size of an atomic write. The size may be
> + specified in bytes, in kilobytes with a "k" suffix, in megabytes
> + with a "m" suffix, or in gigabytes with a "g" suffix.
> +
> + The default value is to set the maximum io completion size
> + to allow each CPU to handle one at a time.
> +
> noalign
> Data allocations will not be aligned at stripe unit
> boundaries. This is only relevant to filesystems created
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index f530aa5d72f5..36e47ec3c3c2 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -1475,3 +1475,57 @@ xfs_calc_max_atomic_write_fsblocks(
>
> return ret;
> }
> +
> +/*
> + * Compute the log reservation needed to complete an atomic write of a given
> + * number of blocks. Worst case, each block requires separate handling.
> + * Returns true if the blockcount is supported, false otherwise.
> + */
> +bool
> +xfs_calc_atomic_write_reservation(
> + struct xfs_mount *mp,
> + int bytes)
Hmm, the comment says this should be a block count, not a byte count.
xfs_extlen_t blockcount)
> +{
> + struct xfs_trans_res *curr_res = &M_RES(mp)->tr_atomic_ioend;
> + unsigned int per_intent, step_size;
> + unsigned int logres;
> + xfs_extlen_t blockcount = XFS_B_TO_FSBT(mp, bytes);
> + uint old_logres =
> + M_RES(mp)->tr_atomic_ioend.tr_logres;
> + int min_logblocks;
> +
> + /*
> + * If the caller doesn't ask for a specific atomic write size, then
> + * we'll use conservatively use tr_itruncate as the basis for computing
> + * a reasonable maximum.
> + */
> + if (blockcount == 0) {
> + curr_res->tr_logres = M_RES(mp)->tr_itruncate.tr_logres;
> + return true;
> + }
> +
> + /* Untorn write completions require out of place write remapping */
> + if (!xfs_has_reflink(mp))
> + return false;
> +
> + per_intent = xfs_calc_atomic_write_ioend_geometry(mp, &step_size);
> +
> + if (check_mul_overflow(blockcount, per_intent, &logres))
> + return false;
> + if (check_add_overflow(logres, step_size, &logres))
> + return false;
> +
> + curr_res->tr_logres = logres;
> + min_logblocks = xfs_log_calc_minimum_size(mp);
> +
> + trace_xfs_calc_max_atomic_write_reservation(mp, per_intent, step_size,
> + blockcount, min_logblocks, curr_res->tr_logres);
> +
> + if (min_logblocks > mp->m_sb.sb_logblocks) {
> + /* Log too small, revert changes. */
> + curr_res->tr_logres = old_logres;
> + return false;
> + }
> +
> + return true;
> +}
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index a6d303b83688..af974f920556 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -122,5 +122,6 @@ unsigned int xfs_calc_write_reservation_minlogsize(struct xfs_mount *mp);
> unsigned int xfs_calc_qm_dqalloc_reservation_minlogsize(struct xfs_mount *mp);
>
> xfs_extlen_t xfs_calc_max_atomic_write_fsblocks(struct xfs_mount *mp);
> +bool xfs_calc_atomic_write_reservation(struct xfs_mount *mp, int bytes);
>
> #endif /* __XFS_TRANS_RESV_H__ */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 860fc3c91fd5..b8dd9e956c2a 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -671,7 +671,7 @@ static inline unsigned int max_pow_of_two_factor(const unsigned int nr)
> return 1 << (ffs(nr) - 1);
> }
>
> -static inline void
> +void
> xfs_compute_atomic_write_unit_max(
> struct xfs_mount *mp)
> {
> @@ -1160,6 +1160,12 @@ xfs_mountfs(
> * derived from transaction reservations, so we must do this after the
> * log is fully initialized.
> */
> + if (!xfs_calc_atomic_write_reservation(mp, mp->m_awu_max_bytes)) {
> + xfs_warn(mp, "cannot support atomic writes of %u bytes",
> + mp->m_awu_max_bytes);
> + error = -EINVAL;
> + goto out_agresv;
> + }
Hmm. I don't think this is sufficient validation of m_awu_max_bytes.
xfs_compute_atomic_write_unit_max never allows an atomic write that is
larger than MAX_RW_COUNT or larger than the allocation group, because
it's not possible to land a single write larger than either of those
sizes. The parsing code ignores values that aren't congruent with
an fsblock size, and suffix_kstrtoint gets confused if you feed it
values that are too large (like "2g"). I propose something like this:
/*
* Try to set the atomic write maximum to a new value that we got from
* userspace via mount option.
*/
int
xfs_set_max_atomic_write_opt(
struct xfs_mount *mp,
unsigned long long new_max_bytes)
{
xfs_filblks_t new_max_fsbs = XFS_B_TO_FSBT(mp, new_max_bytes);
if (new_max_bytes) {
xfs_extlen_t max_write_fsbs =
rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
xfs_extlen_t max_group_fsbs =
max(mp->m_groups[XG_TYPE_AG].blocks,
mp->m_groups[XG_TYPE_RTG].blocks);
ASSERT(max_write_fsbs <= U32_MAX);
if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
xfs_warn(mp,
"max atomic write size of %llu bytes not aligned with fsblock",
new_max_bytes);
return -EINVAL;
}
if (new_max_fsbs > max_write_fsbs) {
xfs_warn(mp,
"max atomic write size of %lluk cannot be larger than max write size %lluk",
new_max_bytes >> 10,
XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
return -EINVAL;
}
if (new_max_fsbs > max_group_fsbs) {
xfs_warn(mp,
"max atomic write size of %lluk cannot be larger than allocation group size %lluk",
new_max_bytes >> 10,
XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
return -EINVAL;
}
}
if (!xfs_calc_atomic_write_reservation(mp, new_max_fsbs)) {
xfs_warn(mp,
"cannot support completing atomic writes of %lluk",
new_max_bytes >> 10);
return -EINVAL;
}
xfs_compute_atomic_write_unit_max(mp);
return 0;
}
and then we get some nicer error messages about what exactly failed
validation.
> xfs_compute_atomic_write_unit_max(mp);
>
> return 0;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index c0eff3adfa31..a5037db4ecff 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -236,6 +236,9 @@ typedef struct xfs_mount {
> bool m_update_sb; /* sb needs update in mount */
> unsigned int m_max_open_zones;
>
> + /* max_atomic_write mount option value */
> + unsigned int m_awu_max_bytes;
> +
> /*
> * Bitsets of per-fs metadata that have been checked and/or are sick.
> * Callers must hold m_sb_lock to access these two fields.
> @@ -798,4 +801,6 @@ static inline void xfs_mod_sb_delalloc(struct xfs_mount *mp, int64_t delta)
> percpu_counter_add(&mp->m_delalloc_blks, delta);
> }
>
> +void xfs_compute_atomic_write_unit_max(struct xfs_mount *mp);
> +
> #endif /* __XFS_MOUNT_H__ */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index b2dd0c0bf509..f7849052e5ff 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -111,7 +111,7 @@ enum {
> Opt_prjquota, Opt_uquota, Opt_gquota, Opt_pquota,
> Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
> Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum, Opt_max_open_zones,
> - Opt_lifetime, Opt_nolifetime,
> + Opt_lifetime, Opt_nolifetime, Opt_max_atomic_write,
> };
>
> static const struct fs_parameter_spec xfs_fs_parameters[] = {
> @@ -159,6 +159,7 @@ static const struct fs_parameter_spec xfs_fs_parameters[] = {
> fsparam_u32("max_open_zones", Opt_max_open_zones),
> fsparam_flag("lifetime", Opt_lifetime),
> fsparam_flag("nolifetime", Opt_nolifetime),
> + fsparam_string("max_atomic_write", Opt_max_atomic_write),
> {}
> };
>
> @@ -241,6 +242,9 @@ xfs_fs_show_options(
>
> if (mp->m_max_open_zones)
> seq_printf(m, ",max_open_zones=%u", mp->m_max_open_zones);
> + if (mp->m_awu_max_bytes)
> + seq_printf(m, ",max_atomic_write=%uk",
> + mp->m_awu_max_bytes >> 10);
>
> return 0;
> }
> @@ -1518,6 +1522,13 @@ xfs_fs_parse_param(
> case Opt_nolifetime:
> parsing_mp->m_features |= XFS_FEAT_NOLIFETIME;
> return 0;
> + case Opt_max_atomic_write:
> + if (suffix_kstrtoint(param->string, 10,
> + &parsing_mp->m_awu_max_bytes))
Let's replace this with a new suffix_kstrtoull helper that returns an
unsigned long long quantity that won't get confused. This has the
unfortunate consequence that we have to burn a u64 in xfs_mount instead
of a u32.
--D
> + return -EINVAL;
> + if (parsing_mp->m_awu_max_bytes < 0)
> + return -EINVAL;
> + return 0;
> default:
> xfs_warn(parsing_mp, "unknown mount option [%s].", param->key);
> return -EINVAL;
> @@ -2114,6 +2125,16 @@ xfs_fs_reconfigure(
> if (error)
> return error;
>
> + /* validate new max_atomic_write option before making other changes */
> + if (mp->m_awu_max_bytes != new_mp->m_awu_max_bytes) {
> + if (!xfs_calc_atomic_write_reservation(mp,
> + new_mp->m_awu_max_bytes)) {
> + xfs_warn(mp, "cannot support atomic writes of %u bytes",
> + new_mp->m_awu_max_bytes);
> + return -EINVAL;
> + }
> + }
> +
> /* inode32 -> inode64 */
> if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
> mp->m_features &= ~XFS_FEAT_SMALL_INUMS;
> @@ -2140,6 +2161,11 @@ xfs_fs_reconfigure(
> return error;
> }
>
> + /* set new atomic write max here */
> + if (mp->m_awu_max_bytes != new_mp->m_awu_max_bytes) {
> + xfs_compute_atomic_write_unit_max(mp);
> + mp->m_awu_max_bytes = new_mp->m_awu_max_bytes;
> + }
> return 0;
> }
>
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 24d73e9bbe83..d41885f1efe2 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -230,6 +230,39 @@ TRACE_EVENT(xfs_calc_max_atomic_write_fsblocks,
> __entry->blockcount)
> );
>
> +TRACE_EVENT(xfs_calc_max_atomic_write_reservation,
> + TP_PROTO(struct xfs_mount *mp, unsigned int per_intent,
> + unsigned int step_size, unsigned int blockcount,
> + unsigned int min_logblocks, unsigned int logres),
> + TP_ARGS(mp, per_intent, step_size, blockcount, min_logblocks, logres),
> + TP_STRUCT__entry(
> + __field(dev_t, dev)
> + __field(unsigned int, per_intent)
> + __field(unsigned int, step_size)
> + __field(unsigned int, blockcount)
> + __field(unsigned int, min_logblocks)
> + __field(unsigned int, cur_logblocks)
> + __field(unsigned int, logres)
> + ),
> + TP_fast_assign(
> + __entry->dev = mp->m_super->s_dev;
> + __entry->per_intent = per_intent;
> + __entry->step_size = step_size;
> + __entry->blockcount = blockcount;
> + __entry->min_logblocks = min_logblocks;
> + __entry->cur_logblocks = mp->m_sb.sb_logblocks;
> + __entry->logres = logres;
> + ),
> + TP_printk("dev %d:%d per_intent %u step_size %u blockcount %u min_logblocks %u logblocks %u logres %u",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __entry->per_intent,
> + __entry->step_size,
> + __entry->blockcount,
> + __entry->min_logblocks,
> + __entry->cur_logblocks,
> + __entry->logres)
> +);
> +
> TRACE_EVENT(xlog_intent_recovery_failed,
> TP_PROTO(struct xfs_mount *mp, const struct xfs_defer_op_type *ops,
> int error),
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [PATCH v7 12/14] xfs: add xfs_compute_atomic_write_unit_max()
From: Darrick J. Wong @ 2025-04-15 16:39 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <2a34fd18-7975-4c6c-a220-9a5279f8d58a@oracle.com>
On Tue, Apr 15, 2025 at 05:35:33PM +0100, John Garry wrote:
> On 15/04/2025 17:25, Darrick J. Wong wrote:
> > > Signed-off-by: John Garry<john.g.garry@oracle.com>
> > > [djwong: use a new reservation type for atomic write ioends]
> > There should be a
> > Signed-off-by: "Darrick J. Wong"<djwong@kernel.org>
> > underneath this line.
>
> Fine, but then I think that I need to add my SoB tag again after that, since
> we have this history: I sent, you sent, I sent.
There's no need to duplicate trailers. This is perfectly fine:
Signed-off-by: John Garry<john.g.garry@oracle.com>
[djwong: use a new reservation type for atomic write ioends]
Signed-off-by: "Darrick J. Wong"<djwong@kernel.org>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Remember, these tags only exist so that bughunters and lawyers can
figure out who to point their fingers at.
--D
> Maybe Co-developed may be better, but some don't like that tag...
>
> thanks,
> John
>
^ permalink raw reply
* Re: [PATCH v7 12/14] xfs: add xfs_compute_atomic_write_unit_max()
From: John Garry @ 2025-04-15 16:35 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250415162501.GP25675@frogsfrogsfrogs>
On 15/04/2025 17:25, Darrick J. Wong wrote:
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
>> [djwong: use a new reservation type for atomic write ioends]
> There should be a
> Signed-off-by: "Darrick J. Wong"<djwong@kernel.org>
> underneath this line.
Fine, but then I think that I need to add my SoB tag again after that,
since we have this history: I sent, you sent, I sent.
Maybe Co-developed may be better, but some don't like that tag...
thanks,
John
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox