* [PATCH v3 11/20] mm/rmap: use folio_large_nr_pages() in add/remove functions
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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, Kirill A. Shutemov
In-Reply-To: <20250303163014.1128035-1-david@redhat.com>
Let's just use the "large" variant in code where we are sure that we
have a large folio in our hands: this way we are sure that we don't
perform any unnecessary "large" checks.
While at it, convert the VM_BUG_ON_VMA to a VM_WARN_ON_ONCE.
Maybe in the future there will not be a difference in that regard
between large and small folios; in that case, unifying the handling again
will be easy. E.g., folio_large_nr_pages() will simply translate to
folio_nr_pages() until we replace all instances.
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
mm/rmap.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/mm/rmap.c b/mm/rmap.c
index 08846b7eced60..c9922928616ee 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1274,7 +1274,7 @@ static __always_inline unsigned int __folio_add_rmap(struct folio *folio,
if (first) {
nr = atomic_add_return_relaxed(ENTIRELY_MAPPED, mapped);
if (likely(nr < ENTIRELY_MAPPED + ENTIRELY_MAPPED)) {
- nr_pages = folio_nr_pages(folio);
+ nr_pages = folio_large_nr_pages(folio);
/*
* We only track PMD mappings of PMD-sized
* folios separately.
@@ -1522,14 +1522,11 @@ void folio_add_anon_rmap_pmd(struct folio *folio, struct page *page,
void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
unsigned long address, rmap_t flags)
{
- const int nr = folio_nr_pages(folio);
const bool exclusive = flags & RMAP_EXCLUSIVE;
- int nr_pmdmapped = 0;
+ int nr = 1, nr_pmdmapped = 0;
VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
VM_WARN_ON_FOLIO(!exclusive && !folio_test_locked(folio), folio);
- VM_BUG_ON_VMA(address < vma->vm_start ||
- address + (nr << PAGE_SHIFT) > vma->vm_end, vma);
/*
* VM_DROPPABLE mappings don't swap; instead they're just dropped when
@@ -1547,6 +1544,7 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
} else if (!folio_test_pmd_mappable(folio)) {
int i;
+ nr = folio_large_nr_pages(folio);
for (i = 0; i < nr; i++) {
struct page *page = folio_page(folio, i);
@@ -1559,6 +1557,7 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
folio_set_large_mapcount(folio, nr, vma);
atomic_set(&folio->_nr_pages_mapped, nr);
} else {
+ nr = folio_large_nr_pages(folio);
/* increment count (starts at -1) */
atomic_set(&folio->_entire_mapcount, 0);
folio_set_large_mapcount(folio, 1, vma);
@@ -1568,6 +1567,9 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
nr_pmdmapped = nr;
}
+ VM_WARN_ON_ONCE(address < vma->vm_start ||
+ address + (nr << PAGE_SHIFT) > vma->vm_end);
+
__folio_mod_stat(folio, nr, nr_pmdmapped);
mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_ANON, 1);
}
@@ -1681,7 +1683,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
if (last) {
nr = atomic_sub_return_relaxed(ENTIRELY_MAPPED, mapped);
if (likely(nr < ENTIRELY_MAPPED)) {
- nr_pages = folio_nr_pages(folio);
+ nr_pages = folio_large_nr_pages(folio);
if (level == RMAP_LEVEL_PMD)
nr_pmdmapped = nr_pages;
nr = nr_pages - (nr & FOLIO_PAGES_MAPPED);
--
2.48.1
^ permalink raw reply related
* [PATCH v3 15/20] mm: CONFIG_NO_PAGE_MAPCOUNT to prepare for not maintain per-page mapcounts in large folios
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
We're close to the finishing line: let's introduce a new
CONFIG_NO_PAGE_MAPCOUNT config option where we will incrementally remove
any dependencies on per-page mapcounts in large folios. Once that's
done, we'll stop maintaining the per-page mapcounts with this
config option enabled.
CONFIG_NO_PAGE_MAPCOUNT will be EXPERIMENTAL for now, as we'll have to
learn about some of the real world impact of some of the implications.
As writing "!CONFIG_NO_PAGE_MAPCOUNT" is really nasty, let's introduce
a helper config option "CONFIG_PAGE_MAPCOUNT" that expresses the
negation.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
mm/Kconfig | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index 4034a0441f650..e4bdcf11d1b86 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -881,8 +881,25 @@ config READ_ONLY_THP_FOR_FS
support of file THPs will be developed in the next few release
cycles.
+config NO_PAGE_MAPCOUNT
+ bool "No per-page mapcount (EXPERIMENTAL)"
+ help
+ Do not maintain per-page mapcounts for pages part of larger
+ allocations, such as transparent huge pages.
+
+ When this config option is enabled, some interfaces that relied on
+ this information will rely on less-precise per-allocation information
+ instead: for example, using the average per-page mapcount in such
+ a large allocation instead of the per-page mapcount.
+
+ EXPERIMENTAL because the impact of some changes is still unclear.
+
endif # TRANSPARENT_HUGEPAGE
+# simple helper to make the code a bit easier to read
+config PAGE_MAPCOUNT
+ def_bool !NO_PAGE_MAPCOUNT
+
#
# The architecture supports pgtable leaves that is larger than PAGE_SIZE
#
--
2.48.1
^ permalink raw reply related
* [PATCH v3 14/20] mm: convert folio_likely_mapped_shared() to folio_maybe_mapped_shared()
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Let's reuse our new MM ownership tracking infrastructure for large folios
to make folio_likely_mapped_shared() never return false negatives --
never indicating "not mapped shared" although the folio *is* mapped shared.
With that, we can rename it to folio_maybe_mapped_shared() and get rid of
the dependency on the mapcount of the first folio page.
The semantics are now arguably clearer: no mixture of "false negatives" and
"false positives", only the remaining possibility for "false positives".
Thoroughly document the new semantics. We might now detect that a large
folio is "maybe mapped shared" although it *no longer* is -- but once was.
Now, if more than two MMs mapped a folio at the same time, and the MM
mapping the folio exclusively at the end is not one tracked in the two
folio MM slots, we will detect the folio as "maybe mapped shared".
For anonymous folios, usually (except weird corner cases) all PTEs
that target a "maybe mapped shared" folio are R/O. As soon as a child
process would write to them (iow, actively use them), we would CoW and
effectively replace these PTEs. Most cases (below) are not expected to
really matter with large anonymous folios for this reason.
Most importantly, there will be no change at all for:
* small folios
* hugetlb folios
* PMD-mapped PMD-sized THPs (single mapping)
This change has the potential to affect existing callers of
folio_likely_mapped_shared() -> folio_maybe_mapped_shared():
(1) fs/proc/task_mmu.c: no change (hugetlb)
(2) khugepaged counts PTEs that target shared folios towards
max_ptes_shared (default: HPAGE_PMD_NR / 2), meaning we could skip a
collapse where we would have previously collapsed. This only applies to
anonymous folios and is not expected to matter in practice.
Worth noting that this change sorts out case (A) documented in
commit 1bafe96e89f0 ("mm/khugepaged: replace page_mapcount() check by
folio_likely_mapped_shared()") by removing the possibility
for "false negatives".
(3) MADV_COLD / MADV_PAGEOUT / MADV_FREE will not try splitting PTE-mapped
THPs that are considered shared but not fully covered by the
requested range, consequently not processing them.
PMD-mapped PMD-sized THP are not affected, or when all PTEs are
covered. These functions are usually only called on anon/file folios
that are exclusively mapped most of the time (no other file mappings
or no fork()), so the "false negatives" are not expected to matter in
practice.
(4) mbind() / migrate_pages() / move_pages() will refuse to migrate shared
folios unless MPOL_MF_MOVE_ALL is effective (requires CAP_SYS_NICE).
We will now reject some folios that could be migrated.
Similar to (3), especially with MPOL_MF_MOVE_ALL, so this is not
expected to matter in practice.
Note that cpuset_migrate_mm_workfn() calls do_migrate_pages() with
MPOL_MF_MOVE_ALL.
(5) NUMA hinting
mm/migrate.c:migrate_misplaced_folio_prepare() will skip file folios
that are probably shared libraries (-> "mapped shared" and
executable). This check would have detected it as a shared library
at some point (at least 3 MMs mapping it), so detecting it
afterwards does not sound wrong (still a shared library). Not
expected to matter.
mm/memory.c:numa_migrate_check() will indicate TNF_SHARED in
MAP_SHARED file mappings when encountering a shared folio. Similar
reasoning, not expected to matter.
mm/mprotect.c:change_pte_range() will skip folios detected as shared
in CoW mappings. Similarly, this is not expected to matter in
practice, but if it would ever be a problem we could relax that
check a bit (e.g., basing it on the average page-mapcount in a folio),
because it was only an optimization when many (e.g., 288) processes
were mapping the same folios -- see commit 859d4adc3415 ("mm: numa: do
not trap faults on shared data section pages.")
(6) mm/rmap.c:folio_referenced_one() will skip exclusive swapbacked folios
in dying processes. Applies to anonymous folios only. Without "false
negatives", we'll now skip all actually shared ones. Skipping ones
that are actually exclusive won't really matter, it's a pure
optimization, and is not expected to matter in practice.
In theory, one can detect the problematic scenario: folio_mapcount() > 0
and no folio MM slot is occupied ("state unknown"). One could reset the MM
slots while doing an rmap walk, which migration / folio split already do
when setting everything up. Further, when batching PTEs we might naturally
learn about a owner (e.g., folio_mapcount() == nr_ptes) and could update
the owner. However, we'll defer that until the scenarios where it would
really matter are clear.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
fs/proc/task_mmu.c | 4 ++--
include/linux/mm.h | 43 ++++++++++++++++++++++---------------------
mm/huge_memory.c | 2 +-
mm/khugepaged.c | 8 +++-----
mm/madvise.c | 6 +++---
mm/memory.c | 2 +-
mm/mempolicy.c | 8 ++++----
mm/migrate.c | 7 +++----
mm/mprotect.c | 2 +-
mm/rmap.c | 2 +-
10 files changed, 41 insertions(+), 43 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index c17615e21a5d6..1162f0e72df2e 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1023,7 +1023,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
if (folio) {
/* We treat non-present entries as "maybe shared". */
- if (!present || folio_likely_mapped_shared(folio) ||
+ if (!present || folio_maybe_mapped_shared(folio) ||
hugetlb_pmd_shared(pte))
mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
else
@@ -1882,7 +1882,7 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
if (!folio_test_anon(folio))
flags |= PM_FILE;
- if (!folio_likely_mapped_shared(folio) &&
+ if (!folio_maybe_mapped_shared(folio) &&
!hugetlb_pmd_shared(ptep))
flags |= PM_MMAP_EXCLUSIVE;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 53dd4f99fdabc..a4f2c56fcf524 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2245,23 +2245,18 @@ static inline size_t folio_size(const struct folio *folio)
}
/**
- * folio_likely_mapped_shared - Estimate if the folio is mapped into the page
- * tables of more than one MM
+ * folio_maybe_mapped_shared - Whether the folio is mapped into the page
+ * tables of more than one MM
* @folio: The folio.
*
- * This function checks if the folio is currently mapped into more than one
- * MM ("mapped shared"), or if the folio is only mapped into a single MM
- * ("mapped exclusively").
+ * This function checks if the folio maybe currently mapped into more than one
+ * MM ("maybe mapped shared"), or if the folio is certainly mapped into a single
+ * MM ("mapped exclusively").
*
* For KSM folios, this function also returns "mapped shared" when a folio is
* mapped multiple times into the same MM, because the individual page mappings
* are independent.
*
- * As precise information is not easily available for all folios, this function
- * estimates the number of MMs ("sharers") that are currently mapping a folio
- * using the number of times the first page of the folio is currently mapped
- * into page tables.
- *
* For small anonymous folios and anonymous hugetlb folios, the return
* value will be exactly correct: non-KSM folios can only be mapped at most once
* into an MM, and they cannot be partially mapped. KSM folios are
@@ -2269,8 +2264,8 @@ static inline size_t folio_size(const struct folio *folio)
*
* For other folios, the result can be fuzzy:
* #. For partially-mappable large folios (THP), the return value can wrongly
- * indicate "mapped exclusively" (false negative) when the folio is
- * only partially mapped into at least one MM.
+ * indicate "mapped shared" (false positive) if a folio was mapped by
+ * more than two MMs at one point in time.
* #. For pagecache folios (including hugetlb), the return value can wrongly
* indicate "mapped shared" (false positive) when two VMAs in the same MM
* cover the same file range.
@@ -2287,7 +2282,7 @@ static inline size_t folio_size(const struct folio *folio)
*
* Return: Whether the folio is estimated to be mapped into more than one MM.
*/
-static inline bool folio_likely_mapped_shared(struct folio *folio)
+static inline bool folio_maybe_mapped_shared(struct folio *folio)
{
int mapcount = folio_mapcount(folio);
@@ -2295,16 +2290,22 @@ static inline bool folio_likely_mapped_shared(struct folio *folio)
if (!folio_test_large(folio) || unlikely(folio_test_hugetlb(folio)))
return mapcount > 1;
- /* A single mapping implies "mapped exclusively". */
- if (mapcount <= 1)
- return false;
-
- /* If any page is mapped more than once we treat it "mapped shared". */
- if (folio_entire_mapcount(folio) || mapcount > folio_nr_pages(folio))
+ /*
+ * vm_insert_page() without CONFIG_TRANSPARENT_HUGEPAGE ...
+ * simply assume "mapped shared", nobody should really care
+ * about this for arbitrary kernel allocations.
+ */
+ if (!IS_ENABLED(CONFIG_MM_ID))
return true;
- /* Let's guess based on the first subpage. */
- return atomic_read(&folio->_mapcount) > 0;
+ /*
+ * A single mapping implies "mapped exclusively", even if the
+ * folio flag says something different: it's easier to handle this
+ * case here instead of on the RMAP hot path.
+ */
+ if (mapcount <= 1)
+ return false;
+ return folio_test_large_maybe_mapped_shared(folio);
}
#ifndef HAVE_ARCH_MAKE_FOLIO_ACCESSIBLE
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 8e8b07e8b12fe..826bfe907017f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2155,7 +2155,7 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
* If other processes are mapping this folio, we couldn't discard
* the folio unless they all do MADV_FREE so let's skip the folio.
*/
- if (folio_likely_mapped_shared(folio))
+ if (folio_maybe_mapped_shared(folio))
goto out;
if (!folio_trylock(folio))
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 5f0be134141e8..cc945c6ab3bdb 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -607,7 +607,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
VM_BUG_ON_FOLIO(!folio_test_anon(folio), folio);
/* See hpage_collapse_scan_pmd(). */
- if (folio_likely_mapped_shared(folio)) {
+ if (folio_maybe_mapped_shared(folio)) {
++shared;
if (cc->is_khugepaged &&
shared > khugepaged_max_ptes_shared) {
@@ -1359,11 +1359,9 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
/*
* We treat a single page as shared if any part of the THP
- * is shared. "False negatives" from
- * folio_likely_mapped_shared() are not expected to matter
- * much in practice.
+ * is shared.
*/
- if (folio_likely_mapped_shared(folio)) {
+ if (folio_maybe_mapped_shared(folio)) {
++shared;
if (cc->is_khugepaged &&
shared > khugepaged_max_ptes_shared) {
diff --git a/mm/madvise.c b/mm/madvise.c
index e01e93e179a8a..388dc289b5d12 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -387,7 +387,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
folio = pmd_folio(orig_pmd);
/* Do not interfere with other mappings of this folio */
- if (folio_likely_mapped_shared(folio))
+ if (folio_maybe_mapped_shared(folio))
goto huge_unlock;
if (pageout_anon_only_filter && !folio_test_anon(folio))
@@ -486,7 +486,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
if (nr < folio_nr_pages(folio)) {
int err;
- if (folio_likely_mapped_shared(folio))
+ if (folio_maybe_mapped_shared(folio))
continue;
if (pageout_anon_only_filter && !folio_test_anon(folio))
continue;
@@ -721,7 +721,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
if (nr < folio_nr_pages(folio)) {
int err;
- if (folio_likely_mapped_shared(folio))
+ if (folio_maybe_mapped_shared(folio))
continue;
if (!folio_trylock(folio))
continue;
diff --git a/mm/memory.c b/mm/memory.c
index bb245a8fe04bc..a838c8c44bfdc 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5700,7 +5700,7 @@ int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
* Flag if the folio is shared between multiple address spaces. This
* is later used when determining whether to group tasks together
*/
- if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
+ if (folio_maybe_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
*flags |= TNF_SHARED;
/*
* For memory tiering mode, cpupid of slow memory page is used
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index bbaadbeeb2919..530e71fe91476 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -642,11 +642,11 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
* Unless MPOL_MF_MOVE_ALL, we try to avoid migrating a shared folio.
* Choosing not to migrate a shared folio is not counted as a failure.
*
- * See folio_likely_mapped_shared() on possible imprecision when we
+ * See folio_maybe_mapped_shared() on possible imprecision when we
* cannot easily detect if a folio is shared.
*/
if ((flags & MPOL_MF_MOVE_ALL) ||
- (!folio_likely_mapped_shared(folio) && !hugetlb_pmd_shared(pte)))
+ (!folio_maybe_mapped_shared(folio) && !hugetlb_pmd_shared(pte)))
if (!folio_isolate_hugetlb(folio, qp->pagelist))
qp->nr_failed++;
unlock:
@@ -1033,10 +1033,10 @@ static bool migrate_folio_add(struct folio *folio, struct list_head *foliolist,
* Unless MPOL_MF_MOVE_ALL, we try to avoid migrating a shared folio.
* Choosing not to migrate a shared folio is not counted as a failure.
*
- * See folio_likely_mapped_shared() on possible imprecision when we
+ * See folio_maybe_mapped_shared() on possible imprecision when we
* cannot easily detect if a folio is shared.
*/
- if ((flags & MPOL_MF_MOVE_ALL) || !folio_likely_mapped_shared(folio)) {
+ if ((flags & MPOL_MF_MOVE_ALL) || !folio_maybe_mapped_shared(folio)) {
if (folio_isolate_lru(folio)) {
list_add_tail(&folio->lru, foliolist);
node_stat_mod_folio(folio,
diff --git a/mm/migrate.c b/mm/migrate.c
index 365c6daa8d1b1..fb4afd31baf0c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2228,7 +2228,7 @@ static int __add_folio_for_migration(struct folio *folio, int node,
if (folio_nid(folio) == node)
return 0;
- if (folio_likely_mapped_shared(folio) && !migrate_all)
+ if (folio_maybe_mapped_shared(folio) && !migrate_all)
return -EACCES;
if (folio_test_hugetlb(folio)) {
@@ -2653,11 +2653,10 @@ int migrate_misplaced_folio_prepare(struct folio *folio,
* processes with execute permissions as they are probably
* shared libraries.
*
- * See folio_likely_mapped_shared() on possible imprecision
+ * See folio_maybe_mapped_shared() on possible imprecision
* when we cannot easily detect if a folio is shared.
*/
- if ((vma->vm_flags & VM_EXEC) &&
- folio_likely_mapped_shared(folio))
+ if ((vma->vm_flags & VM_EXEC) && folio_maybe_mapped_shared(folio))
return -EACCES;
/*
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 1444878f7aeb2..62c1f79457412 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -133,7 +133,7 @@ static long change_pte_range(struct mmu_gather *tlb,
/* Also skip shared copy-on-write pages */
if (is_cow_mapping(vma->vm_flags) &&
(folio_maybe_dma_pinned(folio) ||
- folio_likely_mapped_shared(folio)))
+ folio_maybe_mapped_shared(folio)))
continue;
/*
diff --git a/mm/rmap.c b/mm/rmap.c
index c9922928616ee..8de415157bc8d 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -889,7 +889,7 @@ static bool folio_referenced_one(struct folio *folio,
if ((!atomic_read(&vma->vm_mm->mm_users) ||
check_stable_address_space(vma->vm_mm)) &&
folio_test_anon(folio) && folio_test_swapbacked(folio) &&
- !folio_likely_mapped_shared(folio)) {
+ !folio_maybe_mapped_shared(folio)) {
pra->referenced = -1;
page_vma_mapped_walk_done(&pvmw);
return false;
--
2.48.1
^ permalink raw reply related
* [PATCH v3 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Let's implement an alternative when per-page mapcounts in large folios
are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
For large folios, we'll return the per-page average mapcount within the
folio, whereby we round to the closest integer when calculating the
average: however, we'll always return at least 1 if the folio is
mapped.
So assuming a folio with 512 pages, the average would be:
* 0 if not pages are mapped
* 1 if there are 1 .. 767 per-page mappings
* 2 if there are 767 .. 1279 per-page mappings
...
For hugetlb folios and for large folios that are fully mapped
into all address spaces, there is no change.
We'll make use of this helper in other context next.
As an alternative, we could simply return 0 for non-hugetlb large folios,
or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
But the information exposed by this interface can still be valuable, and
frequently we deal with fully-mapped large folios where the average
corresponds to the actual page mapcount. So we'll leave it like this for
now and document the new behavior.
Note: this interface is likely not very relevant for performance. If
ever required, we could try doing a rather expensive rmap walk to collect
precisely how often this folio page is mapped.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/admin-guide/mm/pagemap.rst | 7 ++++-
fs/proc/internal.h | 35 ++++++++++++++++++++++++
fs/proc/page.c | 11 ++++++--
3 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
index a297e824f9900..d6647daca9122 100644
--- a/Documentation/admin-guide/mm/pagemap.rst
+++ b/Documentation/admin-guide/mm/pagemap.rst
@@ -43,7 +43,12 @@ There are four components to pagemap:
skip over unmapped regions.
* ``/proc/kpagecount``. This file contains a 64-bit count of the number of
- times each page is mapped, indexed by PFN.
+ times each page is mapped, indexed by PFN. Some kernel configurations do
+ not track the precise number of times a page part of a larger allocation
+ (e.g., THP) is mapped. In these configurations, the average number of
+ mappings per page in this larger allocation is returned instead. However,
+ if any page of the large allocation is mapped, the returned value will
+ be at least 1.
The page-types tool in the tools/mm directory can be used to query the
number of times a page is mapped.
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 1695509370b88..96ea58e843114 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -174,6 +174,41 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
return mapcount;
}
+/**
+ * folio_average_page_mapcount() - Average number of mappings per page in this
+ * folio
+ * @folio: The folio.
+ *
+ * The average number of user page table entries that reference each page in
+ * this folio as tracked via the RMAP: either referenced directly (PTE) or
+ * as part of a larger area that covers this page (e.g., PMD).
+ *
+ * The average is calculated by rounding to the nearest integer; however,
+ * to avoid duplicated code in current callers, the average is at least
+ * 1 if any page of the folio is mapped.
+ *
+ * Returns: The average number of mappings per page in this folio.
+ */
+static inline int folio_average_page_mapcount(struct folio *folio)
+{
+ int mapcount, entire_mapcount, avg;
+
+ if (!folio_test_large(folio))
+ return atomic_read(&folio->_mapcount) + 1;
+
+ mapcount = folio_large_mapcount(folio);
+ if (unlikely(mapcount <= 0))
+ return 0;
+ entire_mapcount = folio_entire_mapcount(folio);
+ if (mapcount <= entire_mapcount)
+ return entire_mapcount;
+ mapcount -= entire_mapcount;
+
+ /* Round to closest integer ... */
+ avg = ((unsigned int)mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
+ /* ... but return at least 1. */
+ return max_t(int, avg + entire_mapcount, 1);
+}
/*
* array.c
*/
diff --git a/fs/proc/page.c b/fs/proc/page.c
index a55f5acefa974..23fc771100ae5 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -67,9 +67,14 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf,
* memmaps that were actually initialized.
*/
page = pfn_to_online_page(pfn);
- if (page)
- mapcount = folio_precise_page_mapcount(page_folio(page),
- page);
+ if (page) {
+ struct folio *folio = page_folio(page);
+
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ mapcount = folio_precise_page_mapcount(folio, page);
+ else
+ mapcount = folio_average_page_mapcount(folio);
+ }
if (put_user(mapcount, out)) {
ret = -EFAULT;
--
2.48.1
^ permalink raw reply related
* [PATCH v3 10/20] bit_spinlock: __always_inline (un)lock functions
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
The compiler might decide that it is a smart idea to not inline
bit_spin_lock(), primarily when a couple of functions in the same file end
up calling it. Especially when used in RMAP map/unmap code next, the
compiler sometimes decides to not inline, which is then observable in
some micro-benchmarks.
Let's simply flag all lock/unlock functions as __always_inline;
arch_test_and_set_bit_lock() and friends are already tagged like that
(but not test_and_set_bit_lock() for some reason).
If ever a problem, we could split it into a fast and a slow path, and
only force the fast path to be inlined. But there is nothing
particularly "big" here.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/bit_spinlock.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/bit_spinlock.h b/include/linux/bit_spinlock.h
index bbc4730a6505c..c0989b5b0407f 100644
--- a/include/linux/bit_spinlock.h
+++ b/include/linux/bit_spinlock.h
@@ -13,7 +13,7 @@
* Don't use this unless you really need to: spin_lock() and spin_unlock()
* are significantly faster.
*/
-static inline void bit_spin_lock(int bitnum, unsigned long *addr)
+static __always_inline void bit_spin_lock(int bitnum, unsigned long *addr)
{
/*
* Assuming the lock is uncontended, this never enters
@@ -38,7 +38,7 @@ static inline void bit_spin_lock(int bitnum, unsigned long *addr)
/*
* Return true if it was acquired
*/
-static inline int bit_spin_trylock(int bitnum, unsigned long *addr)
+static __always_inline int bit_spin_trylock(int bitnum, unsigned long *addr)
{
preempt_disable();
#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
@@ -54,7 +54,7 @@ static inline int bit_spin_trylock(int bitnum, unsigned long *addr)
/*
* bit-based spin_unlock()
*/
-static inline void bit_spin_unlock(int bitnum, unsigned long *addr)
+static __always_inline void bit_spin_unlock(int bitnum, unsigned long *addr)
{
#ifdef CONFIG_DEBUG_SPINLOCK
BUG_ON(!test_bit(bitnum, addr));
@@ -71,7 +71,7 @@ static inline void bit_spin_unlock(int bitnum, unsigned long *addr)
* non-atomic version, which can be used eg. if the bit lock itself is
* protecting the rest of the flags in the word.
*/
-static inline void __bit_spin_unlock(int bitnum, unsigned long *addr)
+static __always_inline void __bit_spin_unlock(int bitnum, unsigned long *addr)
{
#ifdef CONFIG_DEBUG_SPINLOCK
BUG_ON(!test_bit(bitnum, addr));
--
2.48.1
^ permalink raw reply related
* [PATCH v3 18/20] fs/proc/task_mmu: remove per-page mapcount dependency for "mapmax" (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Let's implement an alternative when per-page mapcounts in large folios are
no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
For calculating "mapmax", we now use the average per-page mapcount in
a large folio instead of the per-page mapcount.
For hugetlb folios and folios that are not partially mapped into MMs,
there is no change.
Likely, this change will not matter much in practice, and an alternative
might be to simple remove this stat with CONFIG_NO_PAGE_MAPCOUNT.
However, there might be value to it, so let's keep it like that and
document the behavior.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/filesystems/proc.rst | 5 +++++
fs/proc/task_mmu.c | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 09f0aed5a08ba..1aa190017f796 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -686,6 +686,11 @@ Where:
node locality page counters (N0 == node0, N1 == node1, ...) and the kernel page
size, in KB, that is backing the mapping up.
+Note that some kernel configurations do not track the precise number of times
+a page part of a larger allocation (e.g., THP) is mapped. In these
+configurations, "mapmax" might corresponds to the average number of mappings
+per page in such a larger allocation instead.
+
1.2 Kernel data
---------------
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f937c2df7b3f4..5043376ebd476 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -2866,7 +2866,12 @@ static void gather_stats(struct page *page, struct numa_maps *md, int pte_dirty,
unsigned long nr_pages)
{
struct folio *folio = page_folio(page);
- int count = folio_precise_page_mapcount(folio, page);
+ int count;
+
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ count = folio_precise_page_mapcount(folio, page);
+ else
+ count = folio_average_page_mapcount(folio);
md->pages += nr_pages;
if (pte_dirty || folio_test_dirty(folio))
--
2.48.1
^ permalink raw reply related
* [PATCH v3 20/20] mm: stop maintaining the per-page mapcount of large folios (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Everything is in place to stop using the per-page mapcounts in large
folios: the mapcount of tail pages will always be logically 0 (-1 value),
just like it currently is for hugetlb folios already, and the page
mapcount of the head page is either 0 (-1 value) or contains a page type
(e.g., hugetlb).
Maintaining _nr_pages_mapped without per-page mapcounts is impossible,
so that one also has to go with CONFIG_NO_PAGE_MAPCOUNT.
There are two remaining implications:
(1) Per-node, per-cgroup and per-lruvec stats of "NR_ANON_MAPPED"
("mapped anonymous memory") and "NR_FILE_MAPPED"
("mapped file memory"):
As soon as any page of the folio is mapped -- folio_mapped() -- we
now account the complete folio as mapped. Once the last page is
unmapped -- !folio_mapped() -- we account the complete folio as
unmapped.
This implies that ...
* "AnonPages" and "Mapped" in /proc/meminfo and
/sys/devices/system/node/*/meminfo
* cgroup v2: "anon" and "file_mapped" in "memory.stat" and
"memory.numa_stat"
* cgroup v1: "rss" and "mapped_file" in "memory.stat" and
"memory.numa_stat
... can now appear higher than before. But note that these folios do
consume that memory, simply not all pages are actually currently
mapped.
It's worth nothing that other accounting in the kernel (esp. cgroup
charging on allocation) is not affected by this change.
[why oh why is "anon" called "rss" in cgroup v1]
(2) Detecting partial mappings
Detecting whether anon THPs are partially mapped gets a bit more
unreliable. As long as a single MM maps such a large folio
("exclusively mapped"), we can reliably detect it. Especially before
fork() / after a short-lived child process quit, we will detect
partial mappings reliably, which is the common case.
In essence, if the average per-page mapcount in an anon THP is < 1,
we know for sure that we have a partial mapping.
However, as soon as multiple MMs are involved, we might miss detecting
partial mappings: this might be relevant with long-lived child
processes. If we have a fully-mapped anon folio before fork(), once
our child processes and our parent all unmap (zap/COW) the same pages
(but not the complete folio), we might not detect the partial mapping.
However, once the child processes quit we would detect the partial
mapping.
How relevant this case is in practice remains to be seen.
Swapout/migration will likely mitigate this.
In the future, RMAP walkers could check for that for that case
(e.g., when collecting access bits during reclaim) and simply flag
them for deferred-splitting.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
.../admin-guide/cgroup-v1/memory.rst | 4 +
Documentation/admin-guide/cgroup-v2.rst | 10 ++-
Documentation/filesystems/proc.rst | 10 ++-
Documentation/mm/transhuge.rst | 31 +++++--
include/linux/rmap.h | 35 ++++++--
mm/internal.h | 5 +-
mm/page_alloc.c | 3 +-
mm/rmap.c | 80 +++++++++++++++++--
8 files changed, 150 insertions(+), 28 deletions(-)
diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
index 286d16fc22ebb..53cf081b22e81 100644
--- a/Documentation/admin-guide/cgroup-v1/memory.rst
+++ b/Documentation/admin-guide/cgroup-v1/memory.rst
@@ -609,6 +609,10 @@ memory.stat file includes following statistics:
'rss + mapped_file" will give you resident set size of cgroup.
+ Note that some kernel configurations might account complete larger
+ allocations (e.g., THP) towards 'rss' and 'mapped_file', even if
+ only some, but not all that memory is mapped.
+
(Note: file and shmem may be shared among other cgroups. In that case,
mapped_file is accounted only when the memory cgroup is owner of page
cache.)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 175e9435ad5c1..53ada5c2620a7 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1448,7 +1448,10 @@ The following nested keys are defined.
anon
Amount of memory used in anonymous mappings such as
- brk(), sbrk(), and mmap(MAP_ANONYMOUS)
+ brk(), sbrk(), and mmap(MAP_ANONYMOUS). Note that
+ some kernel configurations might account complete larger
+ allocations (e.g., THP) if only some, but not all the
+ memory of such an allocation is mapped anymore.
file
Amount of memory used to cache filesystem data,
@@ -1491,7 +1494,10 @@ The following nested keys are defined.
Amount of application memory swapped out to zswap.
file_mapped
- Amount of cached filesystem data mapped with mmap()
+ Amount of cached filesystem data mapped with mmap(). Note
+ that some kernel configurations might account complete
+ larger allocations (e.g., THP) if only some, but not
+ not all the memory of such an allocation is mapped.
file_dirty
Amount of cached filesystem data that was modified but
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index c9e62e8e0685e..3c37b248fc4f1 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1153,9 +1153,15 @@ Dirty
Writeback
Memory which is actively being written back to the disk
AnonPages
- Non-file backed pages mapped into userspace page tables
+ Non-file backed pages mapped into userspace page tables. Note that
+ some kernel configurations might consider all pages part of a
+ larger allocation (e.g., THP) as "mapped", as soon as a single
+ page is mapped.
Mapped
- files which have been mmapped, such as libraries
+ files which have been mmapped, such as libraries. Note that some
+ kernel configurations might consider all pages part of a larger
+ allocation (e.g., THP) as "mapped", as soon as a single page is
+ mapped.
Shmem
Total memory used by shared memory (shmem) and tmpfs
KReclaimable
diff --git a/Documentation/mm/transhuge.rst b/Documentation/mm/transhuge.rst
index baa17d718a762..0e7f8e4cd2e33 100644
--- a/Documentation/mm/transhuge.rst
+++ b/Documentation/mm/transhuge.rst
@@ -116,23 +116,28 @@ pages:
succeeds on tail pages.
- map/unmap of a PMD entry for the whole THP increment/decrement
- folio->_entire_mapcount, increment/decrement folio->_large_mapcount
- and also increment/decrement folio->_nr_pages_mapped by ENTIRELY_MAPPED
- when _entire_mapcount goes from -1 to 0 or 0 to -1.
+ folio->_entire_mapcount and folio->_large_mapcount.
We also maintain the two slots for tracking MM owners (MM ID and
corresponding mapcount), and the current status ("maybe mapped shared" vs.
"mapped exclusively").
+ With CONFIG_PAGE_MAPCOUNT, we also increment/decrement
+ folio->_nr_pages_mapped by ENTIRELY_MAPPED when _entire_mapcount goes
+ from -1 to 0 or 0 to -1.
+
- map/unmap of individual pages with PTE entry increment/decrement
- page->_mapcount, increment/decrement folio->_large_mapcount and also
- increment/decrement folio->_nr_pages_mapped when page->_mapcount goes
- from -1 to 0 or 0 to -1 as this counts the number of pages mapped by PTE.
+ folio->_large_mapcount.
We also maintain the two slots for tracking MM owners (MM ID and
corresponding mapcount), and the current status ("maybe mapped shared" vs.
"mapped exclusively").
+ With CONFIG_PAGE_MAPCOUNT, we also increment/decrement
+ page->_mapcount and increment/decrement folio->_nr_pages_mapped when
+ page->_mapcount goes from -1 to 0 or 0 to -1 as this counts the number
+ of pages mapped by PTE.
+
split_huge_page internally has to distribute the refcounts in the head
page to the tail pages before clearing all PG_head/tail bits from the page
structures. It can be done easily for refcounts taken by page table
@@ -159,8 +164,8 @@ clear where references should go after split: it will stay on the head page.
Note that split_huge_pmd() doesn't have any limitations on refcounting:
pmd can be split at any point and never fails.
-Partial unmap and deferred_split_folio()
-========================================
+Partial unmap and deferred_split_folio() (anon THP only)
+========================================================
Unmapping part of THP (with munmap() or other way) is not going to free
memory immediately. Instead, we detect that a subpage of THP is not in use
@@ -175,3 +180,13 @@ a THP crosses a VMA boundary.
The function deferred_split_folio() is used to queue a folio for splitting.
The splitting itself will happen when we get memory pressure via shrinker
interface.
+
+With CONFIG_PAGE_MAPCOUNT, we reliably detect partial mappings based on
+folio->_nr_pages_mapped.
+
+With CONFIG_NO_PAGE_MAPCOUNT, we detect partial mappings based on the
+average per-page mapcount in a THP: if the average is < 1, an anon THP is
+certainly partially mapped. As long as only a single process maps a THP,
+this detection is reliable. With long-running child processes, there can
+be scenarios where partial mappings can currently not be detected, and
+might need asynchronous detection during memory reclaim in the future.
diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index c131b0efff0fa..6b82b618846ee 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -240,7 +240,7 @@ static __always_inline void folio_set_large_mapcount(struct folio *folio,
folio_set_mm_id(folio, 0, vma->vm_mm->mm_id);
}
-static __always_inline void folio_add_large_mapcount(struct folio *folio,
+static __always_inline int folio_add_return_large_mapcount(struct folio *folio,
int diff, struct vm_area_struct *vma)
{
const mm_id_t mm_id = vma->vm_mm->mm_id;
@@ -286,9 +286,11 @@ static __always_inline void folio_add_large_mapcount(struct folio *folio,
folio->_mm_ids |= FOLIO_MM_IDS_SHARED_BIT;
}
folio_unlock_large_mapcount(folio);
+ return new_mapcount_val + 1;
}
+#define folio_add_large_mapcount folio_add_return_large_mapcount
-static __always_inline void folio_sub_large_mapcount(struct folio *folio,
+static __always_inline int folio_sub_return_large_mapcount(struct folio *folio,
int diff, struct vm_area_struct *vma)
{
const mm_id_t mm_id = vma->vm_mm->mm_id;
@@ -331,7 +333,9 @@ static __always_inline void folio_sub_large_mapcount(struct folio *folio,
folio->_mm_ids &= ~FOLIO_MM_IDS_SHARED_BIT;
out:
folio_unlock_large_mapcount(folio);
+ return new_mapcount_val + 1;
}
+#define folio_sub_large_mapcount folio_sub_return_large_mapcount
#else /* !CONFIG_MM_ID */
/*
* See __folio_rmap_sanity_checks(), we might map large folios even without
@@ -350,17 +354,33 @@ static inline void folio_add_large_mapcount(struct folio *folio,
atomic_add(diff, &folio->_large_mapcount);
}
+static inline int folio_add_return_large_mapcount(struct folio *folio,
+ int diff, struct vm_area_struct *vma)
+{
+ BUILD_BUG();
+}
+
static inline void folio_sub_large_mapcount(struct folio *folio,
int diff, struct vm_area_struct *vma)
{
atomic_sub(diff, &folio->_large_mapcount);
}
+
+static inline int folio_sub_return_large_mapcount(struct folio *folio,
+ int diff, struct vm_area_struct *vma)
+{
+ BUILD_BUG();
+}
#endif /* CONFIG_MM_ID */
#define folio_inc_large_mapcount(folio, vma) \
folio_add_large_mapcount(folio, 1, vma)
+#define folio_inc_return_large_mapcount(folio, vma) \
+ folio_add_return_large_mapcount(folio, 1, vma)
#define folio_dec_large_mapcount(folio, vma) \
folio_sub_large_mapcount(folio, 1, vma)
+#define folio_dec_return_large_mapcount(folio, vma) \
+ folio_sub_return_large_mapcount(folio, 1, vma)
/* RMAP flags, currently only relevant for some anon rmap operations. */
typedef int __bitwise rmap_t;
@@ -538,9 +558,11 @@ static __always_inline void __folio_dup_file_rmap(struct folio *folio,
break;
}
- do {
- atomic_inc(&page->_mapcount);
- } while (page++, --nr_pages > 0);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT)) {
+ do {
+ atomic_inc(&page->_mapcount);
+ } while (page++, --nr_pages > 0);
+ }
folio_add_large_mapcount(folio, orig_nr_pages, dst_vma);
break;
case RMAP_LEVEL_PMD:
@@ -638,7 +660,8 @@ static __always_inline int __folio_try_dup_anon_rmap(struct folio *folio,
do {
if (PageAnonExclusive(page))
ClearPageAnonExclusive(page);
- atomic_inc(&page->_mapcount);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ atomic_inc(&page->_mapcount);
} while (page++, --nr_pages > 0);
folio_add_large_mapcount(folio, orig_nr_pages, dst_vma);
break;
diff --git a/mm/internal.h b/mm/internal.h
index e33a1fc5ed667..bbedb49f18230 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -84,6 +84,8 @@ void page_writeback_init(void);
*/
static inline int folio_nr_pages_mapped(const struct folio *folio)
{
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT))
+ return -1;
return atomic_read(&folio->_nr_pages_mapped) & FOLIO_PAGES_MAPPED;
}
@@ -719,7 +721,8 @@ static inline void prep_compound_head(struct page *page, unsigned int order)
folio_set_order(folio, order);
atomic_set(&folio->_large_mapcount, -1);
- atomic_set(&folio->_nr_pages_mapped, 0);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ atomic_set(&folio->_nr_pages_mapped, 0);
if (IS_ENABLED(CONFIG_MM_ID)) {
folio->_mm_ids = 0;
folio->_mm_id_mapcount[0] = -1;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e3b8bfdd0b756..bd65ff649c115 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -955,7 +955,8 @@ static int free_tail_page_prepare(struct page *head_page, struct page *page)
bad_page(page, "nonzero large_mapcount");
goto out;
}
- if (unlikely(atomic_read(&folio->_nr_pages_mapped))) {
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT) &&
+ unlikely(atomic_read(&folio->_nr_pages_mapped))) {
bad_page(page, "nonzero nr_pages_mapped");
goto out;
}
diff --git a/mm/rmap.c b/mm/rmap.c
index 8de415157bc8d..67bb273dfb80d 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1258,6 +1258,16 @@ static __always_inline unsigned int __folio_add_rmap(struct folio *folio,
break;
}
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
+ nr = folio_add_return_large_mapcount(folio, orig_nr_pages, vma);
+ if (nr == orig_nr_pages)
+ /* Was completely unmapped. */
+ nr = folio_large_nr_pages(folio);
+ else
+ nr = 0;
+ break;
+ }
+
do {
first += atomic_inc_and_test(&page->_mapcount);
} while (page++, --nr_pages > 0);
@@ -1271,6 +1281,18 @@ static __always_inline unsigned int __folio_add_rmap(struct folio *folio,
case RMAP_LEVEL_PMD:
case RMAP_LEVEL_PUD:
first = atomic_inc_and_test(&folio->_entire_mapcount);
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
+ if (level == RMAP_LEVEL_PMD && first)
+ *nr_pmdmapped = folio_large_nr_pages(folio);
+ nr = folio_inc_return_large_mapcount(folio, vma);
+ if (nr == 1)
+ /* Was completely unmapped. */
+ nr = folio_large_nr_pages(folio);
+ else
+ nr = 0;
+ break;
+ }
+
if (first) {
nr = atomic_add_return_relaxed(ENTIRELY_MAPPED, mapped);
if (likely(nr < ENTIRELY_MAPPED + ENTIRELY_MAPPED)) {
@@ -1436,13 +1458,23 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio,
break;
}
}
+
+ VM_WARN_ON_FOLIO(!folio_test_large(folio) && PageAnonExclusive(page) &&
+ atomic_read(&folio->_mapcount) > 0, folio);
for (i = 0; i < nr_pages; i++) {
struct page *cur_page = page + i;
- /* While PTE-mapping a THP we have a PMD and a PTE mapping. */
- VM_WARN_ON_FOLIO((atomic_read(&cur_page->_mapcount) > 0 ||
- (folio_test_large(folio) &&
- folio_entire_mapcount(folio) > 1)) &&
+ VM_WARN_ON_FOLIO(folio_test_large(folio) &&
+ folio_entire_mapcount(folio) > 1 &&
+ PageAnonExclusive(cur_page), folio);
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT))
+ continue;
+
+ /*
+ * While PTE-mapping a THP we have a PMD and a PTE
+ * mapping.
+ */
+ VM_WARN_ON_FOLIO(atomic_read(&cur_page->_mapcount) > 0 &&
PageAnonExclusive(cur_page), folio);
}
@@ -1548,20 +1580,23 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
for (i = 0; i < nr; i++) {
struct page *page = folio_page(folio, i);
- /* increment count (starts at -1) */
- atomic_set(&page->_mapcount, 0);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ /* increment count (starts at -1) */
+ atomic_set(&page->_mapcount, 0);
if (exclusive)
SetPageAnonExclusive(page);
}
folio_set_large_mapcount(folio, nr, vma);
- atomic_set(&folio->_nr_pages_mapped, nr);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ atomic_set(&folio->_nr_pages_mapped, nr);
} else {
nr = folio_large_nr_pages(folio);
/* increment count (starts at -1) */
atomic_set(&folio->_entire_mapcount, 0);
folio_set_large_mapcount(folio, 1, vma);
- atomic_set(&folio->_nr_pages_mapped, ENTIRELY_MAPPED);
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ atomic_set(&folio->_nr_pages_mapped, ENTIRELY_MAPPED);
if (exclusive)
SetPageAnonExclusive(&folio->page);
nr_pmdmapped = nr;
@@ -1665,6 +1700,19 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
break;
}
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
+ nr = folio_sub_return_large_mapcount(folio, nr_pages, vma);
+ if (!nr) {
+ /* Now completely unmapped. */
+ nr = folio_nr_pages(folio);
+ } else {
+ partially_mapped = nr < folio_large_nr_pages(folio) &&
+ !folio_entire_mapcount(folio);
+ nr = 0;
+ }
+ break;
+ }
+
folio_sub_large_mapcount(folio, nr_pages, vma);
do {
last += atomic_add_negative(-1, &page->_mapcount);
@@ -1678,6 +1726,22 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
break;
case RMAP_LEVEL_PMD:
case RMAP_LEVEL_PUD:
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
+ last = atomic_add_negative(-1, &folio->_entire_mapcount);
+ if (level == RMAP_LEVEL_PMD && last)
+ nr_pmdmapped = folio_large_nr_pages(folio);
+ nr = folio_dec_return_large_mapcount(folio, vma);
+ if (!nr) {
+ /* Now completely unmapped. */
+ nr = folio_large_nr_pages(folio);
+ } else {
+ partially_mapped = last &&
+ nr < folio_large_nr_pages(folio);
+ nr = 0;
+ }
+ break;
+ }
+
folio_dec_large_mapcount(folio, vma);
last = atomic_add_negative(-1, &folio->_entire_mapcount);
if (last) {
--
2.48.1
^ permalink raw reply related
* [PATCH v3 19/20] fs/proc/task_mmu: remove per-page mapcount dependency for smaps/smaps_rollup (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Let's implement an alternative when per-page mapcounts in large folios are
no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
When computing the output for smaps / smaps_rollups, in particular when
calculating the USS (Unique Set Size) and the PSS (Proportional Set Size),
we still rely on per-page mapcounts.
To determine private vs. shared, we'll use folio_likely_mapped_shared(),
similar to how we handle PM_MMAP_EXCLUSIVE. Similarly, we might now
under-estimate the USS and count pages towards "shared" that are
actually "private" ("exclusively mapped").
When calculating the PSS, we'll now also use the average per-page
mapcount for large folios: this can result in both, an over-estimation
and an under-estimation of the PSS. The difference is not expected to
matter much in practice, but we'll have to learn as we go.
We can now provide folio_precise_page_mapcount() only with
CONFIG_PAGE_MAPCOUNT, and remove one of the last users of per-page
mapcounts when CONFIG_NO_PAGE_MAPCOUNT is enabled.
Document the new behavior.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/filesystems/proc.rst | 22 +++++++++++++++++++---
fs/proc/internal.h | 8 ++++++++
fs/proc/task_mmu.c | 17 +++++++++++++++--
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 1aa190017f796..c9e62e8e0685e 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -502,9 +502,25 @@ process, its PSS will be 1500. "Pss_Dirty" is the portion of PSS which
consists of dirty pages. ("Pss_Clean" is not included, but it can be
calculated by subtracting "Pss_Dirty" from "Pss".)
-Note that even a page which is part of a MAP_SHARED mapping, but has only
-a single pte mapped, i.e. is currently used by only one process, is accounted
-as private and not as shared.
+Traditionally, a page is accounted as "private" if it is mapped exactly once,
+and a page is accounted as "shared" when mapped multiple times, even when
+mapped in the same process multiple times. Note that this accounting is
+independent of MAP_SHARED.
+
+In some kernel configurations, the semantics of pages part of a larger
+allocation (e.g., THP) can differ: a page is accounted as "private" if all
+pages part of the corresponding large allocation are *certainly* mapped in the
+same process, even if the page is mapped multiple times in that process. A
+page is accounted as "shared" if any page page of the larger allocation
+is *maybe* mapped in a different process. In some cases, a large allocation
+might be treated as "maybe mapped by multiple processes" even though this
+is no longer the case.
+
+Some kernel configurations do not track the precise number of times a page part
+of a larger allocation is mapped. In this case, when calculating the PSS, the
+average number of mappings per page in this larger allocation might be used
+as an approximation for the number of mappings of a page. The PSS calculation
+will be imprecise in this case.
"Referenced" indicates the amount of memory currently marked as referenced or
accessed.
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 96ea58e843114..8c921bc8652d9 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -143,6 +143,7 @@ unsigned name_to_int(const struct qstr *qstr);
/* Worst case buffer size needed for holding an integer. */
#define PROC_NUMBUF 13
+#ifdef CONFIG_PAGE_MAPCOUNT
/**
* folio_precise_page_mapcount() - Number of mappings of this folio page.
* @folio: The folio.
@@ -173,6 +174,13 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
return mapcount;
}
+#else /* !CONFIG_PAGE_MAPCOUNT */
+static inline int folio_precise_page_mapcount(struct folio *folio,
+ struct page *page)
+{
+ BUILD_BUG();
+}
+#endif /* CONFIG_PAGE_MAPCOUNT */
/**
* folio_average_page_mapcount() - Average number of mappings per page in this
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 5043376ebd476..061f16b767118 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -707,6 +707,8 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
struct folio *folio = page_folio(page);
int i, nr = compound ? compound_nr(page) : 1;
unsigned long size = nr * PAGE_SIZE;
+ bool exclusive;
+ int mapcount;
/*
* First accumulate quantities that depend only on |size| and the type
@@ -747,18 +749,29 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
dirty, locked, present);
return;
}
+
+ if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
+ mapcount = folio_average_page_mapcount(folio);
+ exclusive = !folio_maybe_mapped_shared(folio);
+ }
+
/*
* We obtain a snapshot of the mapcount. Without holding the folio lock
* this snapshot can be slightly wrong as we cannot always read the
* mapcount atomically.
*/
for (i = 0; i < nr; i++, page++) {
- int mapcount = folio_precise_page_mapcount(folio, page);
unsigned long pss = PAGE_SIZE << PSS_SHIFT;
+
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT)) {
+ mapcount = folio_precise_page_mapcount(folio, page);
+ exclusive = mapcount < 2;
+ }
+
if (mapcount >= 2)
pss /= mapcount;
smaps_page_accumulate(mss, folio, PAGE_SIZE, pss,
- dirty, locked, mapcount < 2);
+ dirty, locked, exclusive);
}
}
--
2.48.1
^ permalink raw reply related
* [PATCH v3 17/20] fs/proc/task_mmu: remove per-page mapcount dependency for PM_MMAP_EXCLUSIVE (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-03-03 16:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, 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-1-david@redhat.com>
Let's implement an alternative when per-page mapcounts in large folios are
no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
PM_MMAP_EXCLUSIVE will now be set if folio_likely_mapped_shared() is
true -- when the folio is considered "mapped shared", including when
it once was "mapped shared" but no longer is, as documented.
This might result in and under-indication of "exclusively mapped", which
is considered better than over-indicating it: under-estimating the USS
(Unique Set Size) is better than over-estimating it.
As an alternative, we could simply remove that flag with
CONFIG_NO_PAGE_MAPCOUNT completely, but there might be value to it. So,
let's keep it like that and document the behavior.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/admin-guide/mm/pagemap.rst | 11 +++++++++++
fs/proc/task_mmu.c | 11 +++++++++--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
index d6647daca9122..afce291649dd6 100644
--- a/Documentation/admin-guide/mm/pagemap.rst
+++ b/Documentation/admin-guide/mm/pagemap.rst
@@ -38,6 +38,17 @@ There are four components to pagemap:
precisely which pages are mapped (or in swap) and comparing mapped
pages between processes.
+ Traditionally, bit 56 indicates that a page is mapped exactly once and bit
+ 56 is clear when a page is mapped multiple times, even when mapped in the
+ same process multiple times. In some kernel configurations, the semantics
+ for pages part of a larger allocation (e.g., THP) can differ: bit 56 is set
+ if all pages part of the corresponding large allocation are *certainly*
+ mapped in the same process, even if the page is mapped multiple times in that
+ process. Bit 56 is clear when any page page of the larger allocation
+ is *maybe* mapped in a different process. In some cases, a large allocation
+ might be treated as "maybe mapped by multiple processes" even though this
+ is no longer the case.
+
Efficient users of this interface will use ``/proc/pid/maps`` to
determine which areas of memory are actually mapped and llseek to
skip over unmapped regions.
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 1162f0e72df2e..f937c2df7b3f4 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1652,6 +1652,13 @@ static int add_to_pagemap(pagemap_entry_t *pme, struct pagemapread *pm)
return 0;
}
+static bool __folio_page_mapped_exclusively(struct folio *folio, struct page *page)
+{
+ if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
+ return folio_precise_page_mapcount(folio, page) == 1;
+ return !folio_maybe_mapped_shared(folio);
+}
+
static int pagemap_pte_hole(unsigned long start, unsigned long end,
__always_unused int depth, struct mm_walk *walk)
{
@@ -1742,7 +1749,7 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
if (!folio_test_anon(folio))
flags |= PM_FILE;
if ((flags & PM_PRESENT) &&
- folio_precise_page_mapcount(folio, page) == 1)
+ __folio_page_mapped_exclusively(folio, page))
flags |= PM_MMAP_EXCLUSIVE;
}
if (vma->vm_flags & VM_SOFTDIRTY)
@@ -1817,7 +1824,7 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
pagemap_entry_t pme;
if (folio && (flags & PM_PRESENT) &&
- folio_precise_page_mapcount(folio, page + idx) == 1)
+ __folio_page_mapped_exclusively(folio, page))
cur_flags |= PM_MMAP_EXCLUSIVE;
pme = make_pme(frame, cur_flags);
--
2.48.1
^ permalink raw reply related
* Re: [PATCH v3 00/20] mm: MM owner tracking for large folios (!hugetlb) + CONFIG_NO_PAGE_MAPCOUNT
From: Andrew Morton @ 2025-03-03 22:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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-1-david@redhat.com>
On Mon, 3 Mar 2025 17:29:53 +0100 David Hildenbrand <david@redhat.com> wrote:
> Some smaller change based on Zi Yan's feedback (thanks!).
>
>
> Let's add an "easy" way to decide -- without false positives, without
> page-mapcounts and without page table/rmap scanning -- whether a large
> folio is "certainly mapped exclusively" into a single MM, or whether it
> "maybe mapped shared" into multiple MMs.
>
> Use that information to implement Copy-on-Write reuse, to convert
> folio_likely_mapped_shared() to folio_maybe_mapped_share(), and to
> introduce a kernel config option that let's us not use+maintain
> per-page mapcounts in large folios anymore.
>
> ...
>
> The goal is to make CONFIG_NO_PAGE_MAPCOUNT the default at some point,
> to then slowly make it the only option, as we learn about real-life
> impacts and possible ways to mitigate them.
I expect that we'll get very little runtime testing this way, and we
won't hear about that testing unless there's a failure.
Part of me wants to make it default on right now, but that's perhaps a
bit mean to linux-next testers.
Or perhaps default-off for now and switch to default-y for 6.15-rcX?
I suggest this just to push things along more aggressively - we may
choose to return to default-off after a few weeks of -rcX.
^ permalink raw reply
* Re: [PATCH v3 00/20] mm: MM owner tracking for large folios (!hugetlb) + CONFIG_NO_PAGE_MAPCOUNT
From: David Hildenbrand @ 2025-03-04 10:21 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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: <20250303144332.4cb51677966b515ee0c89a44@linux-foundation.org>
On 03.03.25 23:43, Andrew Morton wrote:
> On Mon, 3 Mar 2025 17:29:53 +0100 David Hildenbrand <david@redhat.com> wrote:
>
>> Some smaller change based on Zi Yan's feedback (thanks!).
>>
>>
>> Let's add an "easy" way to decide -- without false positives, without
>> page-mapcounts and without page table/rmap scanning -- whether a large
>> folio is "certainly mapped exclusively" into a single MM, or whether it
>> "maybe mapped shared" into multiple MMs.
>>
>> Use that information to implement Copy-on-Write reuse, to convert
>> folio_likely_mapped_shared() to folio_maybe_mapped_share(), and to
>> introduce a kernel config option that let's us not use+maintain
>> per-page mapcounts in large folios anymore.
>>
>> ...
>>
>> The goal is to make CONFIG_NO_PAGE_MAPCOUNT the default at some point,
>> to then slowly make it the only option, as we learn about real-life
>> impacts and possible ways to mitigate them.
>
> I expect that we'll get very little runtime testing this way, and we
> won't hear about that testing unless there's a failure.
>
> Part of me wants to make it default on right now, but that's perhaps a
> bit mean to linux-next testers.
Yes, letting this sit at least for some time before we enable it in
linux-next as default might make sense.
> > Or perhaps default-off for now and switch to default-y for 6.15-rcX?
Maybe default-off for now, until we rebase mm-unstable to 6.15-rcX.
Then, default on first in linux-next, and then upstream (6.16).
>
> I suggest this just to push things along more aggressively - we may
> choose to return to default-off after a few weeks of -rcX.
Yeah, I'm usually very careful; sometimes a bit too careful :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 03/20] mm: let _folio_nr_pages overlay memcg_data in first tail page
From: David Hildenbrand @ 2025-03-05 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: 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, Kirill A. Shutemov, Stephen Rothwell
In-Reply-To: <20250303163014.1128035-4-david@redhat.com>
On 03.03.25 17:29, David Hildenbrand wrote:
> Let's free up some more of the "unconditionally available on 64BIT"
> space in order-1 folios by letting _folio_nr_pages overlay memcg_data in
> the first tail page (second folio page). Consequently, we have the
> optimization now whenever we have CONFIG_MEMCG, independent of 64BIT.
>
> We have to make sure that page->memcg on tail pages does not return
> "surprises". page_memcg_check() already properly refuses PageTail().
> Let's do that earlier in print_page_owner_memcg() to avoid printing
> wrong "Slab cache page" information. No other code should touch that
> field on tail pages of compound pages.
>
> Reset the "_nr_pages" to 0 when splitting folios, or when freeing them
> back to the buddy (to avoid false page->memcg_data "bad page" reports).
>
> Note that in __split_huge_page(), folio_nr_pages() would stop working
> already as soon as we start messing with the subpages.
>
> Most kernel configs should have at least CONFIG_MEMCG enabled, even if
> disabled at runtime. 64byte "struct memmap" is what we usually have
> on 64BIT.
>
> While at it, rename "_folio_nr_pages" to "_nr_pages".
>
> Hopefully memdescs / dynamically allocating "strut folio" in the future
> will further clean this up, e.g., making _nr_pages available in all
> configs and maybe even in small folios. Doing that should be fairly easy
> on top of this change.
>
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> include/linux/mm.h | 4 ++--
> include/linux/mm_types.h | 30 ++++++++++++++++++++++--------
> mm/huge_memory.c | 16 +++++++++++++---
> mm/internal.h | 4 ++--
> mm/page_alloc.c | 6 +++++-
> mm/page_owner.c | 2 +-
> 6 files changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a743321dc1a5d..694704217df8a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1199,10 +1199,10 @@ static inline unsigned int folio_large_order(const struct folio *folio)
> return folio->_flags_1 & 0xff;
> }
>
> -#ifdef CONFIG_64BIT
> +#ifdef NR_PAGES_IN_LARGE_FOLIO
> static inline long folio_large_nr_pages(const struct folio *folio)
> {
> - return folio->_folio_nr_pages;
> + return folio->_nr_pages;
> }
> #else
> static inline long folio_large_nr_pages(const struct folio *folio)
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 689b2a7461892..e81be20bbabc6 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -287,6 +287,11 @@ typedef struct {
> unsigned long val;
> } swp_entry_t;
>
> +#if defined(CONFIG_MEMCG) || defined(CONFIG_SLAB_OBJ_EXT)
> +/* We have some extra room after the refcount in tail pages. */
> +#define NR_PAGES_IN_LARGE_FOLIO
> +#endif
> +
> /**
> * struct folio - Represents a contiguous set of bytes.
> * @flags: Identical to the page flags.
> @@ -312,7 +317,7 @@ typedef struct {
> * @_large_mapcount: Do not use directly, call folio_mapcount().
> * @_nr_pages_mapped: Do not use outside of rmap and debug code.
> * @_pincount: Do not use directly, call folio_maybe_dma_pinned().
> - * @_folio_nr_pages: Do not use directly, call folio_nr_pages().
> + * @_nr_pages: Do not use directly, call folio_nr_pages().
> * @_hugetlb_subpool: Do not use directly, use accessor in hugetlb.h.
> * @_hugetlb_cgroup: Do not use directly, use accessor in hugetlb_cgroup.h.
> * @_hugetlb_cgroup_rsvd: Do not use directly, use accessor in hugetlb_cgroup.h.
> @@ -377,13 +382,20 @@ struct folio {
> unsigned long _flags_1;
> unsigned long _head_1;
> /* public: */
> - atomic_t _large_mapcount;
> - atomic_t _entire_mapcount;
> - atomic_t _nr_pages_mapped;
> - atomic_t _pincount;
> -#ifdef CONFIG_64BIT
> - unsigned int _folio_nr_pages;
> -#endif
> + union {
> + struct {
> + atomic_t _large_mapcount;
> + atomic_t _entire_mapcount;
> + atomic_t _nr_pages_mapped;
> + atomic_t _pincount;
> + };
> + unsigned long _usable_1[4];
> + };
> + atomic_t _mapcount_1;
> + atomic_t _refcount_1;
> +#ifdef NR_PAGES_IN_LARGE_FOLIO
> + unsigned int _nr_pages;
> +#endif /* NR_PAGES_IN_LARGE_FOLIO */
> /* private: the union with struct page is transitional */
@Andrew
The following on top to make htmldoc happy.
There will be two conflicts to be resolved in two patches because of
the added "/* private: " under "_pincount".
It's fairly straight forward to resolve (just keep "/* private:" below
any changes), but let me know if I should resend a v4 instead for this.
From 9d9ff38c2ea14f1b4dab29f099ec0f6be683f3fe Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 5 Mar 2025 11:14:52 +0100
Subject: [PATCH] fixup: mm: let _folio_nr_pages overlay memcg_data in first
tail page
Make "make htmldoc" happy by marking non-private placeholder entries
private.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/mm_types.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index e81be20bbabc..6062c12c3871 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -381,18 +381,20 @@ struct folio {
struct {
unsigned long _flags_1;
unsigned long _head_1;
- /* public: */
union {
struct {
+ /* public: */
atomic_t _large_mapcount;
atomic_t _entire_mapcount;
atomic_t _nr_pages_mapped;
atomic_t _pincount;
+ /* private: the union with struct page is transitional */
};
unsigned long _usable_1[4];
};
atomic_t _mapcount_1;
atomic_t _refcount_1;
+ /* public: */
#ifdef NR_PAGES_IN_LARGE_FOLIO
unsigned int _nr_pages;
#endif /* NR_PAGES_IN_LARGE_FOLIO */
--
2.48.1
--
Cheers,
David / dhildenb
^ permalink raw reply related
* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-03-06 10:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andy Lutomirski, Kees Cook, Steven Rostedt, Masami Hiramatsu,
Oleg Nesterov, Peter Zijlstra, Andrii Nakryiko, Eyal Birger,
stable, Jann Horn, linux-kernel, linux-trace-kernel, linux-api,
x86, bpf, Thomas Gleixner, Ingo Molnar, Deepak Gupta,
Stephen Rothwell
In-Reply-To: <Z7MnB3yf2u9eR1yp@krava>
On Mon, Feb 17, 2025 at 01:09:43PM +0100, Jiri Olsa wrote:
> On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:
> > On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > >
> > > > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > > > address near the bottom of the address space that is allowed to
> > > > > call into the syscall if uretprobes are not set up.
> > > > >
> > > > > Though the mmap minimum address restrictions will typically prevent
> > > > > creating mappings there, let's make sure uretprobe syscall checks
> > > > > for that.
> > > >
> > > > It would be a layering violation, but we could perhaps do better here:
> > > >
> > > > > - if (regs->ip != trampoline_check_ip())
> > > > > + /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > > > + if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > > > > goto sigill;
> > > >
> > > > Instead of SIGILL, perhaps this should do the seccomp action? So the
> > > > logic in seccomp would be (sketchily, with some real mode1 mess):
> > > >
> > > > if (is_a_real_uretprobe())
> > > > skip seccomp;
> > >
> > > IIUC you want to move the address check earlier to the seccomp path..
> > > with the benefit that we would kill not allowed caller sooner?
> >
> > The benefit would be that seccomp users that want to do something
> > other than killing a process (returning an error code, getting
> > notified, etc) could retain that functionality without the new
> > automatic hole being poked for uretprobe() in cases where uprobes
> > aren't in use or where the calling address doesn't match the uprobe
> > trampoline. IOW it would reduce the scope to which we're making
> > seccomp behave unexpectedly.
>
> Kees, any thoughts about this approach?
ping, any idea?
thanks,
jirka
>
> thanks,
> jirka
>
>
> >
> > >
> > > jirka
> > >
> > > >
> > > > where is_a_real_uretprobe() is only true if the nr and arch match
> > > > uretprobe *and* the address is right.
> > > >
> > > > --Andy
> > >
^ permalink raw reply
* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Ingo Molnar @ 2025-03-06 11:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andy Lutomirski, Kees Cook, Steven Rostedt, Masami Hiramatsu,
Oleg Nesterov, Peter Zijlstra, Andrii Nakryiko, Eyal Birger,
stable, Jann Horn, linux-kernel, linux-trace-kernel, linux-api,
x86, bpf, Thomas Gleixner, Ingo Molnar, Deepak Gupta,
Stephen Rothwell, Alexei Starovoitov
In-Reply-To: <Z8l_ipCn8tBE1d9Q@krava>
* Jiri Olsa <olsajiri@gmail.com> wrote:
> On Mon, Feb 17, 2025 at 01:09:43PM +0100, Jiri Olsa wrote:
> > On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:
> > > On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > > > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > >
> > > > > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > > > > address near the bottom of the address space that is allowed to
> > > > > > call into the syscall if uretprobes are not set up.
> > > > > >
> > > > > > Though the mmap minimum address restrictions will typically prevent
> > > > > > creating mappings there, let's make sure uretprobe syscall checks
> > > > > > for that.
> > > > >
> > > > > It would be a layering violation, but we could perhaps do better here:
> > > > >
> > > > > > - if (regs->ip != trampoline_check_ip())
> > > > > > + /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > > > > + if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > > > > > goto sigill;
> > > > >
> > > > > Instead of SIGILL, perhaps this should do the seccomp action? So the
> > > > > logic in seccomp would be (sketchily, with some real mode1 mess):
> > > > >
> > > > > if (is_a_real_uretprobe())
> > > > > skip seccomp;
> > > >
> > > > IIUC you want to move the address check earlier to the seccomp path..
> > > > with the benefit that we would kill not allowed caller sooner?
> > >
> > > The benefit would be that seccomp users that want to do something
> > > other than killing a process (returning an error code, getting
> > > notified, etc) could retain that functionality without the new
> > > automatic hole being poked for uretprobe() in cases where uprobes
> > > aren't in use or where the calling address doesn't match the uprobe
> > > trampoline. IOW it would reduce the scope to which we're making
> > > seccomp behave unexpectedly.
> >
> > Kees, any thoughts about this approach?
>
> ping, any idea?
So in any case I think the seccomp QoL tie-in suggested by Andy should
be done in a separate patch, and I've applied the -v3 patch to
tip:perf/core as-is.
( I've added Alexei's Acked-by too, which as I've read the v2 thread's
discussion was a given as long as his ~0 suggestion was implemented,
which you did. )
Thanks,
Ingo
^ permalink raw reply
* Re: [RFC PATCH 5/9] Define user structure for events and responses.
From: Mickaël Salaün @ 2025-03-08 19:07 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Jan Kara, linux-security-module,
Amir Goldstein, Matthew Bobrowski, linux-fsdevel, Tycho Andersen,
Christian Brauner, Kees Cook, Jann Horn, Andy Lutomirski,
Paul Moore, linux-api
In-Reply-To: <fbb8e557-0b63-4bbe-b8ac-3f7ba2983146@maowtm.org>
On Thu, Mar 06, 2025 at 03:05:10AM +0000, Tingmao Wang wrote:
> On 3/4/25 19:49, Mickaël Salaün wrote:
> > On Tue, Mar 04, 2025 at 01:13:01AM +0000, Tingmao Wang wrote:
> [...]
> > > + /**
> > > + * @cookie: Opaque identifier to be included in the response.
> > > + */
> > > + __u32 cookie;
> >
> > I guess we could use a __u64 index counter per layer instead. That
> > would also help to order requests if they are treated by different
> > supervisor threads.
>
> I don't immediately see a use for ordering requests (if we get more than one
> event at once, they are coming from different threads anyway so there can't
> be any dependencies between them, and the supervisor threads can use
> timestamps), but I think making it a __u64 is probably a good idea
> regardless, as it means we don't have to do some sort of ID allocation, and
> can just increment an atomic.
Indeed, we should follow the seccomp unotify approach with a random u64
incremented per request.
>
> > > +};
> > > +
> > > +struct landlock_supervise_event {
> > > + struct landlock_supervise_event_hdr hdr;
> > > + __u64 access_request;
> > > + __kernel_pid_t accessor;
> > > + union {
> > > + struct {
> > > + /**
> > > + * @fd1: An open file descriptor for the file (open,
> > > + * delete, execute, link, readdir, rename, truncate),
> > > + * or the parent directory (for create operations
> > > + * targeting its child) being accessed. Must be
> > > + * closed by the reader.
> > > + *
> > > + * If this points to a parent directory, @destname
> > > + * will contain the target filename. If @destname is
> > > + * empty, this points to the target file.
> > > + */
> > > + int fd1;
> > > + /**
> > > + * @fd2: For link or rename requests, a second file
> > > + * descriptor for the target parent directory. Must
> > > + * be closed by the reader. @destname contains the
> > > + * destination filename. This field is -1 if not
> > > + * used.
> > > + */
> > > + int fd2;
> >
> > Can we just use one FD but identify the requested access instead and
> > send one event for each, like for the audit patch series?
>
> I haven't managed to read or test out the audit patch yet (I will do), but I
> think having the ability to specifically tell whether the child is trying to
> move / rename / create a hard link of an existing file, and what it's trying
> to use as destination, might be useful (either for security, or purely for
> UX)?
>
> For example, imagine something trying to link or move ~/.ssh/id_ecdsa to
> /tmp/innocent-tmp-file then read the latter. The supervisor can warn the
> user on the initial link attempt, and the shenanigan will probably be
> stopped there (although still, being able to say "[program] wants to link
> ~/.ssh/id_ecdsa to /tmp/innocent-tmp-file" seems better than just "[program]
> wants to create a link for ~/.ssh/id_ecdsa"), but even if somehow this ends
> up allowed, later on for the read request it could say something like
>
> [program] wants to read /tmp/innocent-tmp-file
> (previously moved from ~/.ssh/id_ecdsa)
>
> Maybe this is a bit silly, but there might be other use cases for knowing
> the exact details of a rename/link request, either for at-the-time decision
> making, or tracking stuff for future requests?
This pattern looks like datagram packets. I think we should use the
netlink attributes. There were concern about using a netlink socket for
the seccomp unotification though:
https://lore.kernel.org/all/CALCETrXeZZfVzXh7SwKhyB=+ySDk5fhrrdrXrcABsQ=JpQT7Tg@mail.gmail.com/
There are two main differences with seccomp unotify:
- the supervisor should be able to receive arbitrary-sized data (e.g.
file name, not path);
- the supervisor should be able to receive file descriptors (instead of
path).
Sockets are created with socket(2) whereas in our case we should only
get a supervisor FD (indirectly) through landlock_restrict_self(2),
which clearly identifies a kernel object. Another issue would be to
deal with network namespaces, probably by creating a private one.
Sockets are powerful but we don't needs all the routing complexity.
Moreover, we should only need a blocking communication channel to avoid
issues managing in-flight object references (transformed to FDs when
received). That makes me think that a socket might not be the right
construct, but we can still rely on the NLA macros to define a proper
protocol with dynamically-sized events, received and send with dedicated
IOCTL commands.
Netlink already provides a way to send a cookie, and
netlink_attribute_type defines the types we'll need, including string.
For instance, a link request/event could include 3 packets, one for each
of these properties:
1. the source file FD;
2. the destination directory FD;
3. the destination filename string.
This way we would avoid the union defined in this patch.
There is still the question about receiving FDs though. It would be nice
to have a (set of?) dedicated IOCTL(s) to receive an FD, but I'm not
sure how this could be properly handled wrt NLA.
>
> I will try out the audit patch to see how things like these appears in the
> log before commenting further on this. Maybe there is a way to achieve this
> while still simplifying the event structure?
>
> >
> > > + /**
> > > + * @destname: A filename for a file creation target.
> > > + *
> > > + * If either of fd1 or fd2 points to a parent
> > > + * directory rather than the target file, this is the
> > > + * NULL-terminated name of the file that will be
> > > + * newly created.
> > > + *
> > > + * Counting the NULL terminator, this field will
> > > + * contain one or more NULL padding at the end so
> > > + * that the length of the whole struct
> > > + * landlock_supervise_event is a multiple of 8 bytes.
> > > + *
> > > + * This is a variable length member, and the length
> > > + * including the terminating NULL(s) can be derived
> > > + * from hdr.length - offsetof(struct
> > > + * landlock_supervise_event, destname).
> > > + */
> > > + char destname[];
> >
> > I'd prefer to avoid sending file names for now. I don't think it's
> > necessary, and that could encourage supervisors to filter access
> > according to names.
> >
>
> This is also motivated by the potential UX I'm thinking of. For example, if
> a newly installed application tries to create ~/.app-name, it will be much
> more reassuring and convenient to the user if we can show something like
>
> [program] wants to mkdir ~/.app-name. Allow this and future
> access to the new directory?
>
> rather than just "[program] wants to mkdir under ~". (The "Allow this and
> future access to the new directory" bit is made possible by the supervisor
> knowing the name of the file/directory being created, and can remember them
> / write them out to a persistent profile etc)
>
> Note that this is just the filename under the dir represented by fd - this
> isn't a path or anything that can be subject to symlink-related attacks,
> etc. If a program calls e.g.
> mkdirat or openat (dfd -> "/some/", pathname="dir/stuff", O_CREAT)
> my understanding is that fd1 will point to /some/dir, and destname would be
> "stuff"
Right, this file name information would be useful. In the case of
audit, the goal is to efficiently and asynchronously log security events
(and align with other LSM logs and related limitations), not primarily
to debug sandboxed apps nor to enrich this information for decision
making, but the supervisor feature would help here. The patch message
should include this rationale.
>
> Actually, in case your question is "why not send a fd to represent the newly
> created file, instead of sending the name" -- I'm not sure whether you can
> open even an O_PATH fd to a non-existent file.
That would not be possible because it would not exist yet, a file name
(not file path) is OK for this case.
>
> > > + };
> > > + struct {
> > > + __u16 port;
> > > + };
> > > + };
> > > +};
> > > +
> >
> > [...]
>
>
^ permalink raw reply
* Re: [RFC PATCH 5/9] Define user structure for events and responses.
From: Tingmao Wang @ 2025-03-10 0:39 UTC (permalink / raw)
To: Mickaël Salaün, Tycho Andersen
Cc: Günther Noack, Jan Kara, linux-security-module,
Amir Goldstein, Matthew Bobrowski, linux-fsdevel,
Christian Brauner, Kees Cook, Jann Horn, Andy Lutomirski,
Paul Moore, linux-api
In-Reply-To: <20250306.aej5ieg1Hi6j@digikod.net>
On 3/8/25 19:07, Mickaël Salaün wrote:
> On Thu, Mar 06, 2025 at 03:05:10AM +0000, Tingmao Wang wrote:
>> On 3/4/25 19:49, Mickaël Salaün wrote:
>>> On Tue, Mar 04, 2025 at 01:13:01AM +0000, Tingmao Wang wrote:
>> [...]
>>>> + /**
>>>> + * @cookie: Opaque identifier to be included in the response.
>>>> + */
>>>> + __u32 cookie;
>>>
>>> I guess we could use a __u64 index counter per layer instead. That
>>> would also help to order requests if they are treated by different
>>> supervisor threads.
>>
>> I don't immediately see a use for ordering requests (if we get more than one
>> event at once, they are coming from different threads anyway so there can't
>> be any dependencies between them, and the supervisor threads can use
>> timestamps), but I think making it a __u64 is probably a good idea
>> regardless, as it means we don't have to do some sort of ID allocation, and
>> can just increment an atomic.
>
> Indeed, we should follow the seccomp unotify approach with a random u64
> incremented per request.
Do you mean a random starting value, incremented by one per request, or
something like the landlock_id in the audit patch (random increments too)?
>
>>
>>>> +};
>>>> +
>>>> +struct landlock_supervise_event {
>>>> + struct landlock_supervise_event_hdr hdr;
>>>> + __u64 access_request;
>>>> + __kernel_pid_t accessor;
>>>> + union {
>>>> + struct {
>>>> + /**
>>>> + * @fd1: An open file descriptor for the file (open,
>>>> + * delete, execute, link, readdir, rename, truncate),
>>>> + * or the parent directory (for create operations
>>>> + * targeting its child) being accessed. Must be
>>>> + * closed by the reader.
>>>> + *
>>>> + * If this points to a parent directory, @destname
>>>> + * will contain the target filename. If @destname is
>>>> + * empty, this points to the target file.
>>>> + */
>>>> + int fd1;
>>>> + /**
>>>> + * @fd2: For link or rename requests, a second file
>>>> + * descriptor for the target parent directory. Must
>>>> + * be closed by the reader. @destname contains the
>>>> + * destination filename. This field is -1 if not
>>>> + * used.
>>>> + */
>>>> + int fd2;
>>>
>>> Can we just use one FD but identify the requested access instead and
>>> send one event for each, like for the audit patch series?
>>
>> I haven't managed to read or test out the audit patch yet (I will do), but I
>> think having the ability to specifically tell whether the child is trying to
>> move / rename / create a hard link of an existing file, and what it's trying
>> to use as destination, might be useful (either for security, or purely for
>> UX)?
>>
>> For example, imagine something trying to link or move ~/.ssh/id_ecdsa to
>> /tmp/innocent-tmp-file then read the latter. The supervisor can warn the
>> user on the initial link attempt, and the shenanigan will probably be
>> stopped there (although still, being able to say "[program] wants to link
>> ~/.ssh/id_ecdsa to /tmp/innocent-tmp-file" seems better than just "[program]
>> wants to create a link for ~/.ssh/id_ecdsa"), but even if somehow this ends
>> up allowed, later on for the read request it could say something like
>>
>> [program] wants to read /tmp/innocent-tmp-file
>> (previously moved from ~/.ssh/id_ecdsa)
>>
>> Maybe this is a bit silly, but there might be other use cases for knowing
>> the exact details of a rename/link request, either for at-the-time decision
>> making, or tracking stuff for future requests?
>
> This pattern looks like datagram packets. I think we should use the
> netlink attributes. There were concern about using a netlink socket for
> the seccomp unotification though:
> https://lore.kernel.org/all/CALCETrXeZZfVzXh7SwKhyB=+ySDk5fhrrdrXrcABsQ=JpQT7Tg@mail.gmail.com/
>
> There are two main differences with seccomp unotify:
> - the supervisor should be able to receive arbitrary-sized data (e.g.
> file name, not path);
> - the supervisor should be able to receive file descriptors (instead of
> path).
>
> Sockets are created with socket(2) whereas in our case we should only
> get a supervisor FD (indirectly) through landlock_restrict_self(2),
> which clearly identifies a kernel object. Another issue would be to
> deal with network namespaces, probably by creating a private one.
> Sockets are powerful but we don't needs all the routing complexity.
> Moreover, we should only need a blocking communication channel to avoid
> issues managing in-flight object references (transformed to FDs when
> received). That makes me think that a socket might not be the right
> construct, but we can still rely on the NLA macros to define a proper
> protocol with dynamically-sized events, received and send with dedicated
> IOCTL commands.
>
> Netlink already provides a way to send a cookie, and
> netlink_attribute_type defines the types we'll need, including string.
>
> For instance, a link request/event could include 3 packets, one for each
> of these properties:
> 1. the source file FD;
> 2. the destination directory FD;
> 3. the destination filename string.
>
> This way we would avoid the union defined in this patch.
I had no idea about netlink - I will take a look. Do you know if there
is any existing code which uses it in a similar way (i.e. not creating
an actual socket, but using netlink messages)?
I think in the end seccomp-unotify went with an ioctl with a custom
struct seccomp_notif due to friction with the NL API [1] - do you think
we will face the same problem here? (I will take a deeper look at
netlink after sending this.)
(Tycho - could you weigh in?)
[1]:
https://lore.kernel.org/all/CAGXu5jKsLDSBjB74SrvCvmGy_RTEjBsMtR5dk1CcRFrHEQfM_g@mail.gmail.com/
>
> There is still the question about receiving FDs though. It would be nice
> to have a (set of?) dedicated IOCTL(s) to receive an FD, but I'm not
> sure how this could be properly handled wrt NLA.
Also, if we go with netlink messages, why do we need additional IOCTLs?
Can we open the fd when we write out the message? (Maybe I will end up
realizing the reason for this after reading netlink code, but I would )
>
>>
>> I will try out the audit patch to see how things like these appears in the
>> log before commenting further on this. Maybe there is a way to achieve this
>> while still simplifying the event structure?
>>
>>>
>>>> + /**
>>>> + * @destname: A filename for a file creation target.
>>>> + *
>>>> + * If either of fd1 or fd2 points to a parent
>>>> + * directory rather than the target file, this is the
>>>> + * NULL-terminated name of the file that will be
>>>> + * newly created.
>>>> + *
>>>> + * Counting the NULL terminator, this field will
>>>> + * contain one or more NULL padding at the end so
>>>> + * that the length of the whole struct
>>>> + * landlock_supervise_event is a multiple of 8 bytes.
>>>> + *
>>>> + * This is a variable length member, and the length
>>>> + * including the terminating NULL(s) can be derived
>>>> + * from hdr.length - offsetof(struct
>>>> + * landlock_supervise_event, destname).
>>>> + */
>>>> + char destname[];
>>>
>>> I'd prefer to avoid sending file names for now. I don't think it's
>>> necessary, and that could encourage supervisors to filter access
>>> according to names.
>>>
>>
>> This is also motivated by the potential UX I'm thinking of. For example, if
>> a newly installed application tries to create ~/.app-name, it will be much
>> more reassuring and convenient to the user if we can show something like
>>
>> [program] wants to mkdir ~/.app-name. Allow this and future
>> access to the new directory?
>>
>> rather than just "[program] wants to mkdir under ~". (The "Allow this and
>> future access to the new directory" bit is made possible by the supervisor
>> knowing the name of the file/directory being created, and can remember them
>> / write them out to a persistent profile etc)
>>
>> Note that this is just the filename under the dir represented by fd - this
>> isn't a path or anything that can be subject to symlink-related attacks,
>> etc. If a program calls e.g.
>> mkdirat or openat (dfd -> "/some/", pathname="dir/stuff", O_CREAT)
>> my understanding is that fd1 will point to /some/dir, and destname would be
>> "stuff"
>
> Right, this file name information would be useful. In the case of
> audit, the goal is to efficiently and asynchronously log security events
> (and align with other LSM logs and related limitations), not primarily
> to debug sandboxed apps nor to enrich this information for decision
> making, but the supervisor feature would help here. The patch message
> should include this rationale.
Will do
>
>>
>> Actually, in case your question is "why not send a fd to represent the newly
>> created file, instead of sending the name" -- I'm not sure whether you can
>> open even an O_PATH fd to a non-existent file.
>
> That would not be possible because it would not exist yet, a file name
> (not file path) is OK for this case.
>
>>
>>>> + };
>>>> + struct {
>>>> + __u16 port;
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>
>>> [...]
>>
>>
^ permalink raw reply
* Add sysctl for tcp_delayed_ack
From: Andrew Easton @ 2025-03-10 1:20 UTC (permalink / raw)
To: David S. Miller, David S. Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Simon Horman, Linux Kernel Mailing List, Network Subsystem,
Linux Kernel Mailing List, Sysctl API ABI
[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]
Subject: Add sysctl for tcp_delayed_ack
Hi everyone,
this is a proposed patch for adding a sysctl for
disabling TCP delayed ACK (IETF RFC 1122) without
having to patch software to constantly poke sockets
with TCP_QUICKACK which apparently resets on
subsequent operations, see tcp(7).
For my personal computer networks experimenting with
globally disabling TCP delayed ACK across two other
operating systems seems to have considerably improved
congestion control. (While I propose only anecdotal
evidence, there is more to it. Am open to the
ensuing technical discussion, but only if that turns
out to be a good use of other people's time.)
This is my first proposed kernel patch and it is
likely missing a whole bunch of details. For
example:
1. Where is the TCP ACK delay computed for IPv6?
Could not identify this in file net/ipv6/tcp_ipv6.c .
2. Perhaps, adding kernel configuration options for
the ncurses interface is desireable. What is a good
example to learn from?
3. Perhaps, setting constants in file
include/uapi/linux/sysctl.h may be unnecessary, but I
have not found any guidelines on when these CTL
numbers are necessary. Likely, because I did not
read the documentation carefully enough. Any
pointers are appreciated.
4. The default should probably be a value like
net.ipv4.tcp_delayed_ack=1 that preserves the current
behavior and hence is backwards compatible for user
space. A value of net.ipv4.tcp_delayed_ack=0 should
globally (for IPv4) disable TCP delayed ACK. Would
also like to add the option for IPv6, but see point
(1).
In case a similar sysctl has already been proposed
and rejected in the past, please point me to the
mailing list archives, if that is not too
inconvenient.
Which questions have I failed to ask that I should
have asked?
Errors and lack of research are on me.
Thank you for sharing your time.
Andrew
[-- Attachment #2: 0001-Add-sysctl-net.ipv4.tcp_delayed_ack-for-disabling-TC.patch --]
[-- Type: application/octet-stream, Size: 6655 bytes --]
From 5e92e3f9dafe58d363b433356f5d6ca1c9e14aba Mon Sep 17 00:00:00 2001
From: Andrew Easton <Andrew@Easton24.com>
Date: Sun, 9 Mar 2025 19:52:28 -0500
Subject: [PATCH] Add sysctl net.ipv4.tcp_delayed_ack for disabling TCP delayed
acknowledgements from IETF RFC 1122.
---
.../networking/net_cachelines/netns_ipv4_sysctl.rst | 1 +
include/net/netns/ipv4.h | 1 +
include/net/netns/ipv6.h | 1 +
include/uapi/linux/sysctl.h | 2 ++
net/core/net_namespace.c | 2 ++
net/ipv4/sysctl_net_ipv4.c | 12 ++++++++++++
net/ipv4/tcp_input.c | 3 ++-
net/ipv6/tcp_ipv6.c | 5 +++++
8 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/net_cachelines/netns_ipv4_sysctl.rst b/Documentation/networking/net_cachelines/netns_ipv4_sysctl.rst
index de0263302f16..88eaed807c94 100644
--- a/Documentation/networking/net_cachelines/netns_ipv4_sysctl.rst
+++ b/Documentation/networking/net_cachelines/netns_ipv4_sysctl.rst
@@ -110,6 +110,7 @@ int sysctl_tcp_min_rtt_wlen rea
u8 sysctl_tcp_min_tso_segs unlikely(icsk_ca_ops-written)
u8 sysctl_tcp_tso_rtt_log read_mostly tcp_tso_autosize
u8 sysctl_tcp_autocorking read_mostly tcp_push/tcp_should_autocork
+u8 sysctl_tcp_delayed_ack read_mostly tcp_in_quickack_mode
u8 sysctl_tcp_reflect_tos tcp_v(4/6)_send_synack
int sysctl_tcp_invalid_ratelimit
int sysctl_tcp_pacing_ss_ratio default_cong_cont(tcp_update_pacing_rate)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 46452da35206..5777f95af02e 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -59,6 +59,7 @@ struct netns_ipv4 {
u8 sysctl_tcp_tso_win_divisor;
u8 sysctl_tcp_tso_rtt_log;
u8 sysctl_tcp_autocorking;
+ u8 sysctl_tcp_delayed_ack;
int sysctl_tcp_min_snd_mss;
unsigned int sysctl_tcp_notsent_lowat;
int sysctl_tcp_limit_output_bytes;
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index 5f2cfd84570a..ec8b891e09ca 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -56,6 +56,7 @@ struct netns_sysctl_ipv6 {
u8 skip_notify_on_dev_down;
u8 fib_notify_on_flag_change;
u8 icmpv6_error_anycast_as_unicast;
+ u8 tcp_delayed_ack;
};
struct netns_ipv6 {
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 8981f00204db..669b3775bd31 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -426,6 +426,7 @@ enum
NET_TCP_ALLOWED_CONG_CONTROL=123,
NET_TCP_MAX_SSTHRESH=124,
NET_TCP_FRTO_RESPONSE=125,
+ NET_IPV4_TCP_DELAYED_ACK=126, /* TCP_QUICKACK but globally. */
};
enum {
@@ -530,6 +531,7 @@ enum {
NET_IPV6_IP6FRAG_TIME=23,
NET_IPV6_IP6FRAG_SECRET_INTERVAL=24,
NET_IPV6_MLD_MAX_MSF=25,
+ NET_IPV6_TCP_DELAYED_ACK=26, /* TCP_QUICKACK but globally. */
};
enum {
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 4303f2a49262..b101707ab9e4 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -1155,6 +1155,8 @@ static void __init netns_ipv4_struct_check(void)
sysctl_tcp_tso_rtt_log);
CACHELINE_ASSERT_GROUP_MEMBER(struct netns_ipv4, netns_ipv4_read_tx,
sysctl_tcp_autocorking);
+ CACHELINE_ASSERT_GROUP_MEMBER(struct netns_ipv4, netns_ipv4_read_tx,
+ sysctl_tcp_delayed_ack);
CACHELINE_ASSERT_GROUP_MEMBER(struct netns_ipv4, netns_ipv4_read_tx,
sysctl_tcp_min_snd_mss);
CACHELINE_ASSERT_GROUP_MEMBER(struct netns_ipv4, netns_ipv4_read_tx,
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 42cb5dc9cb24..8aa00eee8247 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -28,6 +28,8 @@ static int tcp_adv_win_scale_max = 31;
static int tcp_app_win_max = 31;
static int tcp_min_snd_mss_min = TCP_MIN_SND_MSS;
static int tcp_min_snd_mss_max = 65535;
+static int tcp_delayed_ack_min = 0;
+static int tcp_delayed_ack_max = 255;
static int ip_privileged_port_min;
static int ip_privileged_port_max = 65535;
static int ip_ttl_min = 1;
@@ -959,6 +961,16 @@ static struct ctl_table ipv4_net_table[] = {
.mode = 0644,
.proc_handler = proc_allowed_congestion_control,
},
+ {
+ .ctl_name = NET_IPV4_TCP_DELAYED_ACK,
+ .procname = "tcp_delayed_ack",
+ .data = &init_net.ipv4.sysctl_tcp_delayed_ack,
+ .maxlen = sizeof(u8),
+ .mode = 0644,
+ .proc_handler = proc_dou8vec_minmax,
+ .extra1 = &tcp_delayed_ack_min,
+ .extra2 = &tcp_delayed_ack_max
+ },
{
.procname = "tcp_keepalive_time",
.data = &init_net.ipv4.sysctl_tcp_keepalive_time,
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0cbf81bf3d45..5c5e934816cd 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -334,7 +334,8 @@ static bool tcp_in_quickack_mode(struct sock *sk)
const struct dst_entry *dst = __sk_dst_get(sk);
return (dst && dst_metric(dst, RTAX_QUICKACK)) ||
- (icsk->icsk_ack.quick && !inet_csk_in_pingpong_mode(sk));
+ (icsk->icsk_ack.quick && !inet_csk_in_pingpong_mode(sk) ||
+ sk->sk_net.ipv4.sysctl_tcp_delayed_ack > 0);
}
static void tcp_ecn_queue_cwr(struct tcp_sock *tp)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2debdf085a3b..b040ceea9f22 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1149,6 +1149,11 @@ static void tcp_v6_send_ack(const struct sock *sk, struct sk_buff *skb, u32 seq,
struct tcp_key *key, u8 tclass,
__be32 label, u32 priority, u32 txhash)
{
+ /* Where does this condition need to go?
+ * It doesn't seem clear where exactly the
+ * TCP ACK is delayed (IETF RFC 1122) for
+ * IPv6.
+ * (sk->net.ipv6.sysctl.tcp_delayed_ack > 0) */
tcp_v6_send_response(sk, skb, seq, ack, win, tsval, tsecr, oif, 0,
tclass, label, priority, txhash, key);
}
--
2.48.0
[-- Attachment #3: Type: text/plain, Size: 2 bytes --]
^ permalink raw reply related
* Re: Add sysctl for tcp_delayed_ack
From: Jason Xing @ 2025-03-10 5:17 UTC (permalink / raw)
To: Andrew Easton
Cc: David S. Miller, David S. Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman,
Linux Kernel Mailing List, Network Subsystem,
Linux Kernel Mailing List, Sysctl API ABI
In-Reply-To: <FC9BF302-0724-49F3-AD7C-6761D65024A1@Easton24.com>
Hi Andrew,
On Mon, Mar 10, 2025 at 2:33 AM Andrew Easton <Andrew@easton24.com> wrote:
>
> Subject: Add sysctl for tcp_delayed_ack
>
> Hi everyone,
>
> this is a proposed patch for adding a sysctl for
> disabling TCP delayed ACK (IETF RFC 1122) without
> having to patch software to constantly poke sockets
> with TCP_QUICKACK which apparently resets on
> subsequent operations, see tcp(7).
>
> For my personal computer networks experimenting with
> globally disabling TCP delayed ACK across two other
> operating systems seems to have considerably improved
> congestion control. (While I propose only anecdotal
> evidence, there is more to it. Am open to the
> ensuing technical discussion, but only if that turns
> out to be a good use of other people's time.)
>
> This is my first proposed kernel patch and it is
Thanks for proposing this patch :)
As to the idea itself, my personal feelings are:
1) It might be suitable for local kernels instead of public kernels,
even though we internally have a similar patch a few years ago
already.
2) The reason why I hesitated to submit a patch like this before is it
may change/override the default socket behavior which may bring
unexpected impacts. It's a global knob...
3) To be frank, the delayed ack mechanism prevails for so many years
and truly solves too many pure ack packets on the wire issue. And I
believe only a minority of clients try to turn it off.
4) Recently, I was thinking of implementing a delayed ack max timeout
(which you can refer to tcp_delack_max() and see how it works). As I
mentioned, I also hesitate to do so.
Of course, no matter what my thoughts are, it finally depends on the
TCP maintainer's call :)
Thanks,
Jason
> likely missing a whole bunch of details. For
> example:
>
> 1. Where is the TCP ACK delay computed for IPv6?
> Could not identify this in file net/ipv6/tcp_ipv6.c .
>
> 2. Perhaps, adding kernel configuration options for
> the ncurses interface is desireable. What is a good
> example to learn from?
>
> 3. Perhaps, setting constants in file
> include/uapi/linux/sysctl.h may be unnecessary, but I
> have not found any guidelines on when these CTL
> numbers are necessary. Likely, because I did not
> read the documentation carefully enough. Any
> pointers are appreciated.
>
> 4. The default should probably be a value like
> net.ipv4.tcp_delayed_ack=1 that preserves the current
> behavior and hence is backwards compatible for user
> space. A value of net.ipv4.tcp_delayed_ack=0 should
> globally (for IPv4) disable TCP delayed ACK. Would
> also like to add the option for IPv6, but see point
> (1).
>
>
> In case a similar sysctl has already been proposed
> and rejected in the past, please point me to the
> mailing list archives, if that is not too
> inconvenient.
>
>
> Which questions have I failed to ask that I should
> have asked?
>
> Errors and lack of research are on me.
>
> Thank you for sharing your time.
>
> Andrew
>
>
>
>
>
^ permalink raw reply
* Re: Add sysctl for tcp_delayed_ack
From: Eric Dumazet @ 2025-03-10 7:02 UTC (permalink / raw)
To: Andrew Easton
Cc: David S. Miller, David S. Ahern, Jakub Kicinski, Paolo Abeni,
Simon Horman, Linux Kernel Mailing List, Network Subsystem,
Linux Kernel Mailing List, Sysctl API ABI
In-Reply-To: <FC9BF302-0724-49F3-AD7C-6761D65024A1@Easton24.com>
On Mon, Mar 10, 2025 at 2:20 AM Andrew Easton <Andrew@easton24.com> wrote:
>
> Subject: Add sysctl for tcp_delayed_ack
>
> Hi everyone,
Hi Andrew
>
> this is a proposed patch for adding a sysctl for
> disabling TCP delayed ACK (IETF RFC 1122) without
> having to patch software to constantly poke sockets
> with TCP_QUICKACK which apparently resets on
> subsequent operations, see tcp(7).
>
> For my personal computer networks experimenting with
> globally disabling TCP delayed ACK across two other
> operating systems seems to have considerably improved
> congestion control. (While I propose only anecdotal
> evidence, there is more to it. Am open to the
> ensuing technical discussion, but only if that turns
> out to be a good use of other people's time.)
>
> This is my first proposed kernel patch and it is
> likely missing a whole bunch of details. For
> example:
>
> 1. Where is the TCP ACK delay computed for IPv6?
> Could not identify this in file net/ipv6/tcp_ipv6.c .
No need to change tcp_ipv6.c
Generating ACK is generic, thus code is in net/ipv4
>
> 2. Perhaps, adding kernel configuration options for
> the ncurses interface is desireable. What is a good
> example to learn from?
No need. per net-ns sysctl and/or per-socket options are far better
for this case.
>
> 3. Perhaps, setting constants in file
> include/uapi/linux/sysctl.h may be unnecessary, but I
> have not found any guidelines on when these CTL
> numbers are necessary. Likely, because I did not
> read the documentation carefully enough. Any
> pointers are appreciated.
sysctl.h is absolutely deprecated.
No need for NET_IPV4_TCP_DELAYED_ACK,
No ctl_name in 'struct ctl_table'
>
> 4. The default should probably be a value like
> net.ipv4.tcp_delayed_ack=1 that preserves the current
> behavior and hence is backwards compatible for user
> space. A value of net.ipv4.tcp_delayed_ack=0 should
> globally (for IPv4) disable TCP delayed ACK. Would
> also like to add the option for IPv6, but see point
> (1).
>
>
> In case a similar sysctl has already been proposed
> and rejected in the past, please point me to the
> mailing list archives, if that is not too
> inconvenient.
>
>
> Which questions have I failed to ask that I should
> have asked?
>
> Errors and lack of research are on me.
>
Make sure to compile/test your patch on top of net-next tree, and send
it inline,
not as an attachment, so that we can comment on it.
Also next time add benchmark results
like netperf -t TCP_RR (200 flows)
of netper/tcp_rr -F 1000
No delaying ACK for small RPC is essentially doubling the number of
packets to send and receive.
> Thank you for sharing your time.
>
> Andrew
>
>
>
>
>
^ permalink raw reply
* Re: [RFC PATCH 5/9] Define user structure for events and responses.
From: Mickaël Salaün @ 2025-03-11 19:29 UTC (permalink / raw)
To: Tingmao Wang
Cc: Tycho Andersen, Günther Noack, Jan Kara,
linux-security-module, Amir Goldstein, Matthew Bobrowski,
linux-fsdevel, Christian Brauner, Kees Cook, Jann Horn,
Andy Lutomirski, Paul Moore, linux-api
In-Reply-To: <4b0b693d-a152-42c0-bb2c-73e705c3c9b0@maowtm.org>
On Mon, Mar 10, 2025 at 12:39:08AM +0000, Tingmao Wang wrote:
> On 3/8/25 19:07, Mickaël Salaün wrote:
> > On Thu, Mar 06, 2025 at 03:05:10AM +0000, Tingmao Wang wrote:
> > > On 3/4/25 19:49, Mickaël Salaün wrote:
> > > > On Tue, Mar 04, 2025 at 01:13:01AM +0000, Tingmao Wang wrote:
> > > [...]
> > > > > + /**
> > > > > + * @cookie: Opaque identifier to be included in the response.
> > > > > + */
> > > > > + __u32 cookie;
> > > >
> > > > I guess we could use a __u64 index counter per layer instead. That
> > > > would also help to order requests if they are treated by different
> > > > supervisor threads.
> > >
> > > I don't immediately see a use for ordering requests (if we get more than one
> > > event at once, they are coming from different threads anyway so there can't
> > > be any dependencies between them, and the supervisor threads can use
> > > timestamps), but I think making it a __u64 is probably a good idea
> > > regardless, as it means we don't have to do some sort of ID allocation, and
> > > can just increment an atomic.
> >
> > Indeed, we should follow the seccomp unotify approach with a random u64
> > incremented per request.
>
> Do you mean a random starting value, incremented by one per request, or
Yes
> something like the landlock_id in the audit patch (random increments too)?
There is no need for that because the supervisor is more privileged than
the sandbox.
>
> >
> > >
> > > > > +};
> > > > > +
> > > > > +struct landlock_supervise_event {
> > > > > + struct landlock_supervise_event_hdr hdr;
> > > > > + __u64 access_request;
> > > > > + __kernel_pid_t accessor;
> > > > > + union {
> > > > > + struct {
> > > > > + /**
> > > > > + * @fd1: An open file descriptor for the file (open,
> > > > > + * delete, execute, link, readdir, rename, truncate),
> > > > > + * or the parent directory (for create operations
> > > > > + * targeting its child) being accessed. Must be
> > > > > + * closed by the reader.
> > > > > + *
> > > > > + * If this points to a parent directory, @destname
> > > > > + * will contain the target filename. If @destname is
> > > > > + * empty, this points to the target file.
> > > > > + */
> > > > > + int fd1;
> > > > > + /**
> > > > > + * @fd2: For link or rename requests, a second file
> > > > > + * descriptor for the target parent directory. Must
> > > > > + * be closed by the reader. @destname contains the
> > > > > + * destination filename. This field is -1 if not
> > > > > + * used.
> > > > > + */
> > > > > + int fd2;
> > > >
> > > > Can we just use one FD but identify the requested access instead and
> > > > send one event for each, like for the audit patch series?
> > >
> > > I haven't managed to read or test out the audit patch yet (I will do), but I
> > > think having the ability to specifically tell whether the child is trying to
> > > move / rename / create a hard link of an existing file, and what it's trying
> > > to use as destination, might be useful (either for security, or purely for
> > > UX)?
> > >
> > > For example, imagine something trying to link or move ~/.ssh/id_ecdsa to
> > > /tmp/innocent-tmp-file then read the latter. The supervisor can warn the
> > > user on the initial link attempt, and the shenanigan will probably be
> > > stopped there (although still, being able to say "[program] wants to link
> > > ~/.ssh/id_ecdsa to /tmp/innocent-tmp-file" seems better than just "[program]
> > > wants to create a link for ~/.ssh/id_ecdsa"), but even if somehow this ends
> > > up allowed, later on for the read request it could say something like
> > >
> > > [program] wants to read /tmp/innocent-tmp-file
> > > (previously moved from ~/.ssh/id_ecdsa)
> > >
> > > Maybe this is a bit silly, but there might be other use cases for knowing
> > > the exact details of a rename/link request, either for at-the-time decision
> > > making, or tracking stuff for future requests?
> >
> > This pattern looks like datagram packets. I think we should use the
> > netlink attributes. There were concern about using a netlink socket for
> > the seccomp unotification though:
> > https://lore.kernel.org/all/CALCETrXeZZfVzXh7SwKhyB=+ySDk5fhrrdrXrcABsQ=JpQT7Tg@mail.gmail.com/
> >
> > There are two main differences with seccomp unotify:
> > - the supervisor should be able to receive arbitrary-sized data (e.g.
> > file name, not path);
> > - the supervisor should be able to receive file descriptors (instead of
> > path).
> >
> > Sockets are created with socket(2) whereas in our case we should only
> > get a supervisor FD (indirectly) through landlock_restrict_self(2),
> > which clearly identifies a kernel object. Another issue would be to
> > deal with network namespaces, probably by creating a private one.
> > Sockets are powerful but we don't needs all the routing complexity.
> > Moreover, we should only need a blocking communication channel to avoid
> > issues managing in-flight object references (transformed to FDs when
> > received). That makes me think that a socket might not be the right
> > construct, but we can still rely on the NLA macros to define a proper
> > protocol with dynamically-sized events, received and send with dedicated
> > IOCTL commands.
> >
> > Netlink already provides a way to send a cookie, and
> > netlink_attribute_type defines the types we'll need, including string.
> >
> > For instance, a link request/event could include 3 packets, one for each
> > of these properties:
> > 1. the source file FD;
> > 2. the destination directory FD;
> > 3. the destination filename string.
> >
> > This way we would avoid the union defined in this patch.
>
> I had no idea about netlink - I will take a look. Do you know if there is
> any existing code which uses it in a similar way (i.e. not creating an
> actual socket, but using netlink messages)?
I don't know.
>
> I think in the end seccomp-unotify went with an ioctl with a custom struct
> seccomp_notif due to friction with the NL API [1] - do you think we will
> face the same problem here? (I will take a deeper look at netlink after
> sending this.)
>
> (Tycho - could you weigh in?)
>
> [1]: https://lore.kernel.org/all/CAGXu5jKsLDSBjB74SrvCvmGy_RTEjBsMtR5dk1CcRFrHEQfM_g@mail.gmail.com/
We need to check if the NLA API could work. Kees's answer was missing
explanation. Otherwise we should get inspiration from fanotify
messages.
>
> >
> > There is still the question about receiving FDs though. It would be nice
> > to have a (set of?) dedicated IOCTL(s) to receive an FD, but I'm not
> > sure how this could be properly handled wrt NLA.
>
> Also, if we go with netlink messages, why do we need additional IOCTLs? Can
> we open the fd when we write out the message? (Maybe I will end up realizing
> the reason for this after reading netlink code, but I would )
It's much easier to have static-sized struct, both for developers and
for introspection tools (e.g. strace). However, in this case we also
would also have variable-lenght data. See my other reply discussing the
IOCTL idea.
>
> >
> > >
> > > I will try out the audit patch to see how things like these appears in the
> > > log before commenting further on this. Maybe there is a way to achieve this
> > > while still simplifying the event structure?
> > >
> > > >
> > > > > + /**
> > > > > + * @destname: A filename for a file creation target.
> > > > > + *
> > > > > + * If either of fd1 or fd2 points to a parent
> > > > > + * directory rather than the target file, this is the
> > > > > + * NULL-terminated name of the file that will be
> > > > > + * newly created.
> > > > > + *
> > > > > + * Counting the NULL terminator, this field will
> > > > > + * contain one or more NULL padding at the end so
> > > > > + * that the length of the whole struct
> > > > > + * landlock_supervise_event is a multiple of 8 bytes.
> > > > > + *
> > > > > + * This is a variable length member, and the length
> > > > > + * including the terminating NULL(s) can be derived
> > > > > + * from hdr.length - offsetof(struct
> > > > > + * landlock_supervise_event, destname).
> > > > > + */
> > > > > + char destname[];
> > > >
> > > > I'd prefer to avoid sending file names for now. I don't think it's
> > > > necessary, and that could encourage supervisors to filter access
> > > > according to names.
> > > >
> > >
> > > This is also motivated by the potential UX I'm thinking of. For example, if
> > > a newly installed application tries to create ~/.app-name, it will be much
> > > more reassuring and convenient to the user if we can show something like
> > >
> > > [program] wants to mkdir ~/.app-name. Allow this and future
> > > access to the new directory?
> > >
> > > rather than just "[program] wants to mkdir under ~". (The "Allow this and
> > > future access to the new directory" bit is made possible by the supervisor
> > > knowing the name of the file/directory being created, and can remember them
> > > / write them out to a persistent profile etc)
> > >
> > > Note that this is just the filename under the dir represented by fd - this
> > > isn't a path or anything that can be subject to symlink-related attacks,
> > > etc. If a program calls e.g.
> > > mkdirat or openat (dfd -> "/some/", pathname="dir/stuff", O_CREAT)
> > > my understanding is that fd1 will point to /some/dir, and destname would be
> > > "stuff"
> >
> > Right, this file name information would be useful. In the case of
> > audit, the goal is to efficiently and asynchronously log security events
> > (and align with other LSM logs and related limitations), not primarily
> > to debug sandboxed apps nor to enrich this information for decision
> > making, but the supervisor feature would help here. The patch message
> > should include this rationale.
>
> Will do
>
> >
> > >
> > > Actually, in case your question is "why not send a fd to represent the newly
> > > created file, instead of sending the name" -- I'm not sure whether you can
> > > open even an O_PATH fd to a non-existent file.
> >
> > That would not be possible because it would not exist yet, a file name
> > (not file path) is OK for this case.
> >
> > >
> > > > > + };
> > > > > + struct {
> > > > > + __u16 port;
> > > > > + };
> > > > > + };
> > > > > +};
> > > > > +
> > > >
> > > > [...]
> > >
> > >
>
^ permalink raw reply
* [RFC -next 00/10] Add ZC notifications to splice and sendfile
From: Joe Damato @ 2025-03-19 0:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, asml.silence, linux-fsdevel, edumazet, pabeni,
horms, linux-api, linux-arch, viro, jack, kuba, shuah, sdf, mingo,
arnd, brauner, akpm, tglx, jolsa, linux-kselftest, Joe Damato
Greetings:
Welcome to the RFC.
Currently, when a user app uses sendfile the user app has no way to know
if the bytes were transmit; sendfile simply returns, but it is possible
that a slow client on the other side may take time to receive and ACK
the bytes. In the meantime, the user app which called sendfile has no
way to know whether it can overwrite the data on disk that it just
sendfile'd.
One way to fix this is to add zerocopy notifications to sendfile similar
to how MSG_ZEROCOPY works with sendmsg. This is possible thanks to the
extensive work done by Pavel [1].
To support this, two important user ABI changes are proposed:
- A new splice flag, SPLICE_F_ZC, which allows users to signal that
splice should generate zerocopy notifications if possible.
- A new system call, sendfile2, which is similar to sendfile64 except
that it takes an additional argument, flags, which allows the user
to specify either a "regular" sendfile or a sendfile with zerocopy
notifications enabled.
In either case, user apps can read notifications from the error queue
(like they would with MSG_ZEROCOPY) to determine when their call to
sendfile has completed.
I tested this RFC using the selftest modified in the last patch and also
by using the selftest between two different physical hosts:
# server
./msg_zerocopy -4 -i eth0 -t 2 -v -r tcp
# client (does the sendfiling)
dd if=/dev/zero of=sendfile_data bs=1M count=8
./msg_zerocopy -4 -i eth0 -D $SERVER_IP -v -l 1 -t 2 -z -f sendfile_data tcp
I would love to get high level feedback from folks on a few things:
- Is this functionality, at a high level, something that would be
desirable / useful? I think so, but I'm of course I am biased ;)
- Is this approach generally headed in the right direction? Are the
proposed user ABI changes reasonable?
If the above two points are generally agreed upon then I'd welcome
feedback on the patches themselves :)
This is kind of a net thing, but also kind of a splice thing so hope I
am sending this to right places to get appropriate feedback. I based my
code on the vfs/for-next tree, but am happy to rebase on another tree if
desired. The cc-list got a little out of control, so I manually trimmed
it down quite a bit; sorry if I missed anyone I should have CC'd in the
process.
Thanks,
Joe
[1]: https://lore.kernel.org/netdev/cover.1657643355.git.asml.silence@gmail.com/
Joe Damato (10):
splice: Add ubuf_info to prepare for ZC
splice: Add helper that passes through splice_desc
splice: Factor splice_socket into a helper
splice: Add SPLICE_F_ZC and attach ubuf
fs: Add splice_write_sd to file operations
fs: Extend do_sendfile to take a flags argument
fs: Add sendfile2 which accepts a flags argument
fs: Add sendfile flags for sendfile2
fs: Add sendfile2 syscall
selftests: Add sendfile zerocopy notification test
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/tools/syscall_32.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/read_write.c | 40 +++++++---
fs/splice.c | 87 +++++++++++++++++----
include/linux/fs.h | 2 +
include/linux/sendfile.h | 10 +++
include/linux/splice.h | 7 +-
include/linux/syscalls.h | 2 +
include/uapi/asm-generic/unistd.h | 4 +-
net/socket.c | 1 +
scripts/syscall.tbl | 1 +
tools/testing/selftests/net/msg_zerocopy.c | 54 ++++++++++++-
tools/testing/selftests/net/msg_zerocopy.sh | 5 ++
27 files changed, 200 insertions(+), 29 deletions(-)
create mode 100644 include/linux/sendfile.h
base-commit: 2e72b1e0aac24a12f3bf3eec620efaca7ab7d4de
--
2.43.0
^ permalink raw reply
* [RFC -next 01/10] splice: Add ubuf_info to prepare for ZC
From: Joe Damato @ 2025-03-19 0:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, asml.silence, linux-fsdevel, edumazet, pabeni,
horms, linux-api, linux-arch, viro, jack, kuba, shuah, sdf, mingo,
arnd, brauner, akpm, tglx, jolsa, linux-kselftest, Joe Damato
In-Reply-To: <20250319001521.53249-1-jdamato@fastly.com>
Update struct splice_desc to include ubuf_info to prepare splice for
zero copy notifications.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
include/linux/splice.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/splice.h b/include/linux/splice.h
index 9dec4861d09f..7477df3916e2 100644
--- a/include/linux/splice.h
+++ b/include/linux/splice.h
@@ -10,6 +10,7 @@
#define SPLICE_H
#include <linux/pipe_fs_i.h>
+#include <linux/skbuff.h>
/*
* Flags passed in from splice/tee/vmsplice
@@ -43,6 +44,7 @@ struct splice_desc {
loff_t *opos; /* sendfile: output position */
size_t num_spliced; /* number of bytes already spliced */
bool need_wakeup; /* need to wake up writer */
+ struct ubuf_info *ubuf_info; /* zerocopy infrastructure */
};
struct partial_page {
--
2.43.0
^ permalink raw reply related
* [RFC -next 02/10] splice: Add helper that passes through splice_desc
From: Joe Damato @ 2025-03-19 0:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, asml.silence, linux-fsdevel, edumazet, pabeni,
horms, linux-api, linux-arch, viro, jack, kuba, shuah, sdf, mingo,
arnd, brauner, akpm, tglx, jolsa, linux-kselftest, Joe Damato
In-Reply-To: <20250319001521.53249-1-jdamato@fastly.com>
Add do_splice_from_sd which takes splice_desc as an argument. This
helper is just a wrapper around splice_write but will be extended. Use
the helper from existing splice code.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/splice.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/splice.c b/fs/splice.c
index 2898fa1e9e63..9575074a1296 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -941,6 +941,15 @@ static ssize_t do_splice_from(struct pipe_inode_info *pipe, struct file *out,
return out->f_op->splice_write(pipe, out, ppos, len, flags);
}
+static ssize_t do_splice_from_sd(struct pipe_inode_info *pipe, struct file *out,
+ struct splice_desc *sd)
+{
+ if (unlikely(!out->f_op->splice_write))
+ return warn_unsupported(out, "write");
+ return out->f_op->splice_write(pipe, out, sd->opos, sd->total_len,
+ sd->flags);
+}
+
/*
* Indicate to the caller that there was a premature EOF when reading from the
* source and the caller didn't indicate they would be sending more data after
@@ -1161,7 +1170,7 @@ static int direct_splice_actor(struct pipe_inode_info *pipe,
long ret;
file_start_write(file);
- ret = do_splice_from(pipe, file, sd->opos, sd->total_len, sd->flags);
+ ret = do_splice_from_sd(pipe, file, sd);
file_end_write(file);
return ret;
}
@@ -1171,7 +1180,7 @@ static int splice_file_range_actor(struct pipe_inode_info *pipe,
{
struct file *file = sd->u.file;
- return do_splice_from(pipe, file, sd->opos, sd->total_len, sd->flags);
+ return do_splice_from_sd(pipe, file, sd);
}
static void direct_file_splice_eof(struct splice_desc *sd)
--
2.43.0
^ permalink raw reply related
* [RFC -next 03/10] splice: Factor splice_socket into a helper
From: Joe Damato @ 2025-03-19 0:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, asml.silence, linux-fsdevel, edumazet, pabeni,
horms, linux-api, linux-arch, viro, jack, kuba, shuah, sdf, mingo,
arnd, brauner, akpm, tglx, jolsa, linux-kselftest, Joe Damato
In-Reply-To: <20250319001521.53249-1-jdamato@fastly.com>
splice_socket becomes a wrapper around splice_socket_generic which takes
a ubuf pointer to prepare for zerocopy notifications.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/splice.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/fs/splice.c b/fs/splice.c
index 9575074a1296..1f27ce6d1c34 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -37,6 +37,8 @@
#include <linux/socket.h>
#include <linux/sched/signal.h>
+#include <net/sock.h>
+
#include "internal.h"
/*
@@ -783,21 +785,10 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
EXPORT_SYMBOL(iter_file_splice_write);
#ifdef CONFIG_NET
-/**
- * splice_to_socket - splice data from a pipe to a socket
- * @pipe: pipe to splice from
- * @out: socket to write to
- * @ppos: position in @out
- * @len: number of bytes to splice
- * @flags: splice modifier flags
- *
- * Description:
- * Will send @len bytes from the pipe to a network socket. No data copying
- * is involved.
- *
- */
-ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
- loff_t *ppos, size_t len, unsigned int flags)
+static ssize_t splice_socket_generic(struct pipe_inode_info *pipe,
+ struct file *out, loff_t *ppos,
+ size_t len, unsigned int flags,
+ struct ubuf_info *ubuf_info)
{
struct socket *sock = sock_from_file(out);
struct bio_vec bvec[16];
@@ -920,6 +911,25 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
wakeup_pipe_writers(pipe);
return spliced ?: ret;
}
+
+/**
+ * splice_to_socket - splice data from a pipe to a socket
+ * @pipe: pipe to splice from
+ * @out: socket to write to
+ * @ppos: position in @out
+ * @len: number of bytes to splice
+ * @flags: splice modifier flags
+ *
+ * Description:
+ * Will send @len bytes from the pipe to a network socket. No data copying
+ * is involved.
+ *
+ */
+ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
+ loff_t *ppos, size_t len, unsigned int flags)
+{
+ return splice_socket_generic(pipe, out, ppos, len, flags, NULL);
+}
#endif
static int warn_unsupported(struct file *file, const char *op)
--
2.43.0
^ permalink raw reply related
* [RFC -next 04/10] splice: Add SPLICE_F_ZC and attach ubuf
From: Joe Damato @ 2025-03-19 0:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, asml.silence, linux-fsdevel, edumazet, pabeni,
horms, linux-api, linux-arch, viro, jack, kuba, shuah, sdf, mingo,
arnd, brauner, akpm, tglx, jolsa, linux-kselftest, Joe Damato
In-Reply-To: <20250319001521.53249-1-jdamato@fastly.com>
Add the SPLICE_F_ZC flag and when it is set, allocate a ubuf and attach
it to generate zerocopy notifications.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/splice.c | 20 ++++++++++++++++++++
include/linux/splice.h | 3 ++-
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/fs/splice.c b/fs/splice.c
index 1f27ce6d1c34..6dc60f47f84e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -875,6 +875,11 @@ static ssize_t splice_socket_generic(struct pipe_inode_info *pipe,
if (out->f_flags & O_NONBLOCK)
msg.msg_flags |= MSG_DONTWAIT;
+ if (unlikely(flags & SPLICE_F_ZC) && ubuf_info) {
+ msg.msg_flags = MSG_ZEROCOPY;
+ msg.msg_ubuf = ubuf_info;
+ }
+
iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, bvec, bc,
len - remain);
ret = sock_sendmsg(sock, &msg);
@@ -1223,12 +1228,27 @@ static ssize_t do_splice_direct_actor(struct file *in, loff_t *ppos,
if (unlikely(out->f_flags & O_APPEND))
return -EINVAL;
+ if (unlikely(flags & SPLICE_F_ZC)) {
+ struct socket *sock = sock_from_file(out);
+ struct sock *sk = sock->sk;
+ struct ubuf_info *ubuf_info;
+
+ ubuf_info = msg_zerocopy_realloc(sk, len, NULL);
+ if (!ubuf_info)
+ return -ENOMEM;
+ sd.ubuf_info = ubuf_info;
+ }
+
ret = splice_direct_to_actor(in, &sd, actor);
if (ret > 0)
*ppos = sd.pos;
+ if (unlikely(flags & SPLICE_F_ZC))
+ refcount_dec(&sd.ubuf_info->refcnt);
+
return ret;
}
+
/**
* do_splice_direct - splices data directly between two files
* @in: file to splice from
diff --git a/include/linux/splice.h b/include/linux/splice.h
index 7477df3916e2..a88588cf2754 100644
--- a/include/linux/splice.h
+++ b/include/linux/splice.h
@@ -21,8 +21,9 @@
/* from/to, of course */
#define SPLICE_F_MORE (0x04) /* expect more data */
#define SPLICE_F_GIFT (0x08) /* pages passed in are a gift */
+#define SPLICE_F_ZC (0x10) /* generate zero copy notifications */
-#define SPLICE_F_ALL (SPLICE_F_MOVE|SPLICE_F_NONBLOCK|SPLICE_F_MORE|SPLICE_F_GIFT)
+#define SPLICE_F_ALL (SPLICE_F_MOVE|SPLICE_F_NONBLOCK|SPLICE_F_MORE|SPLICE_F_GIFT|SPLICE_F_ZC)
/*
* Passed to the actors
--
2.43.0
^ permalink raw reply related
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