| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8b1854b5-5c0a-43b1-aed7-aa4e8b8e8a1a@suse.cz>
Date: Fri, 25 Oct 2024 20:13:26 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Suren Baghdasaryan <surenb@...gle.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Matthew Wilcox <willy@...radead.org>, "Paul E . McKenney"
<paulmck@...nel.org>, Jann Horn <jannh@...gle.com>,
David Hildenbrand <david@...hat.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Muchun Song <muchun.song@...ux.dev>,
Richard Henderson <richard.henderson@...aro.org>,
Matt Turner <mattst88@...il.com>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
"James E . J . Bottomley" <James.Bottomley@...senPartnership.com>,
Helge Deller <deller@....de>, Chris Zankel <chris@...kel.net>,
Max Filippov <jcmvbkbc@...il.com>, Arnd Bergmann <arnd@...nel.org>,
linux-alpha@...r.kernel.org, linux-mips@...r.kernel.org,
linux-parisc@...r.kernel.org, linux-arch@...r.kernel.org,
Shuah Khan <shuah@...nel.org>, Christian Brauner <brauner@...nel.org>,
linux-kselftest@...r.kernel.org, Sidhartha Kumar
<sidhartha.kumar@...cle.com>, Jeff Xu <jeffxu@...omium.org>,
Christoph Hellwig <hch@...radead.org>, linux-api@...r.kernel.org,
John Hubbard <jhubbard@...dia.com>
Subject: Re: [PATCH v3 1/5] mm: pagewalk: add the ability to install PTEs
On 10/23/24 18:24, Lorenzo Stoakes wrote:
> The existing generic pagewalk logic permits the walking of page tables,
> invoking callbacks at individual page table levels via user-provided
> mm_walk_ops callbacks.
>
> This is useful for traversing existing page table entries, but precludes
> the ability to establish new ones.
>
> Existing mechanism for performing a walk which also installs page table
> entries if necessary are heavily duplicated throughout the kernel, each
> with semantic differences from one another and largely unavailable for use
> elsewhere.
>
> Rather than add yet another implementation, we extend the generic pagewalk
> logic to enable the installation of page table entries by adding a new
> install_pte() callback in mm_walk_ops. If this is specified, then upon
> encountering a missing page table entry, we allocate and install a new one
> and continue the traversal.
>
> If a THP huge page is encountered at either the PMD or PUD level we split
> it only if there are ops->pte_entry() (or ops->pmd_entry at PUD level),
> otherwise if there is only an ops->install_pte(), we avoid the unnecessary
> split.
>
> We do not support hugetlb at this stage.
>
> If this function returns an error, or an allocation fails during the
> operation, we abort the operation altogether. It is up to the caller to
> deal appropriately with partially populated page table ranges.
>
> If install_pte() is defined, the semantics of pte_entry() change - this
> callback is then only invoked if the entry already exists. This is a useful
> property, as it allows a caller to handle existing PTEs while installing
> new ones where necessary in the specified range.
>
> If install_pte() is not defined, then there is no functional difference to
> this patch, so all existing logic will work precisely as it did before.
>
> As we only permit the installation of PTEs where a mapping does not already
> exist there is no need for TLB management, however we do invoke
> update_mmu_cache() for architectures which require manual maintenance of
> mappings for other CPUs.
>
> We explicitly do not allow the existing page walk API to expose this
> feature as it is dangerous and intended for internal mm use only. Therefore
> we provide a new walk_page_range_mm() function exposed only to
> mm/internal.h.
>
> Reviewed-by: Jann Horn <jannh@...gle.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
Just a small subjective suggestion in case you agree and there's a respin or
followups:
> @@ -109,18 +131,19 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
>
> if (walk->action == ACTION_AGAIN)
> goto again;
> -
> - /*
> - * Check this here so we only break down trans_huge
> - * pages when we _need_ to
> - */
> - if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
> - walk->action == ACTION_CONTINUE ||
> - !(ops->pte_entry))
> + if (walk->action == ACTION_CONTINUE)
> continue;
> + if (!ops->install_pte && !ops->pte_entry)
> + continue; /* Nothing to do. */
> + if (!ops->pte_entry && ops->install_pte &&
> + pmd_present(*pmd) &&
> + (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
> + continue; /* Avoid unnecessary split. */
Much better now, thanks, but maybe the last 2 parts could be:
if (!ops->pte_entry) {
if (!ops->install_pte)
continue; /* Nothing to do. */
else if (pmd_present(*pmd)
&& (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)))
continue; /* Avoid unnecessary split. */
}
Or at least put !ops->pte_entry first in both conditions?
> if (walk->vma)
> split_huge_pmd(walk->vma, pmd, addr);
> + else if (pmd_leaf(*pmd) || !pmd_present(*pmd))
> + continue; /* Nothing to do. */
>
> err = walk_pte_range(pmd, addr, next, walk);
> if (err)
> @@ -148,11 +171,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> again:
> next = pud_addr_end(addr, end);
> if (pud_none(*pud)) {
> - if (ops->pte_hole)
> + if (ops->install_pte)
> + err = __pmd_alloc(walk->mm, pud, addr);
> + else if (ops->pte_hole)
> err = ops->pte_hole(addr, next, depth, walk);
> if (err)
> break;
> - continue;
> + if (!ops->install_pte)
> + continue;
> }
>
> walk->action = ACTION_SUBTREE;
> @@ -164,14 +190,20 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
>
> if (walk->action == ACTION_AGAIN)
> goto again;
> -
> - if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
> - walk->action == ACTION_CONTINUE ||
> - !(ops->pmd_entry || ops->pte_entry))
> + if (walk->action == ACTION_CONTINUE)
> continue;
> + if (!ops->install_pte && !ops->pte_entry && !ops->pmd_entry)
> + continue; /* Nothing to do. */
> + if (!ops->pmd_entry && !ops->pte_entry && ops->install_pte &&
> + pud_present(*pud) &&
> + (pud_trans_huge(*pud) || pud_devmap(*pud)))
> + continue; /* Avoid unnecessary split. */
Ditto.
Thanks!
Powered by blists - more mailing lists