| 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: <fb548cd0-4a6a-4e90-92b8-24c7b35df416@lucifer.local>
Date: Wed, 4 Jun 2025 15:48:58 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
Jason Gunthorpe <jgg@...pe.ca>, John Hubbard <jhubbard@...dia.com>,
Peter Xu <peterx@...hat.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v1] mm/gup: remove (VM_)BUG_ONs
+Linus in case he has an opinion about BUG_ON() in general...
On Wed, Jun 04, 2025 at 04:05:44PM +0200, David Hildenbrand wrote:
> Especially once we hit one of the assertions in
> sanity_check_pinned_pages(), observing follow-up assertions failing
> in other code can give good clues about what went wrong, so use
> VM_WARN_ON_ONCE instead.
I guess the situation where you'd actually want a BUG_ON() is one where
carrying on might cause further corruption so you just want things to stop.
But usually we're already pretty screwed if the thing happened right? So
it's rare if ever that this would be legit?
Linus's point of view is that we shouldn't use them _at all_ right? So
maybe even this situation isn't one where we'd want to use one?
>
> While at it, let's just convert all VM_BUG_ON to VM_WARN_ON_ONCE as
> well. Add one comment for the pfn_valid() check.
Yeah VM_BUG_ON() is just _weird_. Maybe we should get rid of all of them
full stop?
>
> We have to introduce VM_WARN_ON_ONCE_VMA() to make that fly.
I checked the implementation vs. the other VM_WARN_ON_ONCE_*()'s and it
looks good.
I wonder if we can find a way to not duplicate this code... but one for a
follow up I think :>)
>
> Drop the BUG_ON after mmap_read_lock_killable(), if that ever returns
> something > 0 we're in bigger trouble. Convert the other BUG_ON's into
> VM_WARN_ON_ONCE as well, they are in a similar domain "should never
> happen", but more reasonable to check for during early testing.
>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: Mike Rapoport <rppt@...nel.org>
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Cc: John Hubbard <jhubbard@...dia.com>
> Cc: Peter Xu <peterx@...hat.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
LGTM so,
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
One nit below.
> ---
>
> Wanted to do this for a long time, but my todo list keeps growing ...
Sounds familiar :) Merge window a chance to do some of these things...
>
> Based on mm/mm-unstable
>
> ---
> include/linux/mmdebug.h | 12 ++++++++++++
> mm/gup.c | 41 +++++++++++++++++++----------------------
> 2 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index a0a3894900ed4..14a45979cccc9 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -89,6 +89,17 @@ void vma_iter_dump_tree(const struct vma_iterator *vmi);
> } \
> unlikely(__ret_warn_once); \
> })
> +#define VM_WARN_ON_ONCE_VMA(cond, vma) ({ \
> + static bool __section(".data..once") __warned; \
> + int __ret_warn_once = !!(cond); \
> + \
> + if (unlikely(__ret_warn_once && !__warned)) { \
> + dump_vma(vma); \
> + __warned = true; \
> + WARN_ON(1); \
> + } \
> + unlikely(__ret_warn_once); \
> +})
An aside, I wonder if we could somehow make this generic for various
WARN_ON_ONCE()'s?
> #define VM_WARN_ON_VMG(cond, vmg) ({ \
> int __ret_warn = !!(cond); \
> \
> @@ -115,6 +126,7 @@ void vma_iter_dump_tree(const struct vma_iterator *vmi);
> #define VM_WARN_ON_FOLIO(cond, folio) BUILD_BUG_ON_INVALID(cond)
> #define VM_WARN_ON_ONCE_FOLIO(cond, folio) BUILD_BUG_ON_INVALID(cond)
> #define VM_WARN_ON_ONCE_MM(cond, mm) BUILD_BUG_ON_INVALID(cond)
> +#define VM_WARN_ON_ONCE_VMA(cond, vma) BUILD_BUG_ON_INVALID(cond)
> #define VM_WARN_ON_VMG(cond, vmg) BUILD_BUG_ON_INVALID(cond)
> #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
> #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
> diff --git a/mm/gup.c b/mm/gup.c
> index e065a49842a87..3c3931fcdd820 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -64,11 +64,11 @@ static inline void sanity_check_pinned_pages(struct page **pages,
> !folio_test_anon(folio))
> continue;
> if (!folio_test_large(folio) || folio_test_hugetlb(folio))
> - VM_BUG_ON_PAGE(!PageAnonExclusive(&folio->page), page);
> + VM_WARN_ON_ONCE_PAGE(!PageAnonExclusive(&folio->page), page);
> else
> /* Either a PTE-mapped or a PMD-mapped THP. */
> - VM_BUG_ON_PAGE(!PageAnonExclusive(&folio->page) &&
> - !PageAnonExclusive(page), page);
> + VM_WARN_ON_ONCE_PAGE(!PageAnonExclusive(&folio->page) &&
> + !PageAnonExclusive(page), page);
Nit but wouldn't VM_WARN_ON_ONCE_FOLIO() work better here?
> }
> }
>
> @@ -760,8 +760,8 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> if (!pmd_write(pmdval) && gup_must_unshare(vma, flags, page))
> return ERR_PTR(-EMLINK);
>
> - VM_BUG_ON_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
> - !PageAnonExclusive(page), page);
> + VM_WARN_ON_ONCE_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
> + !PageAnonExclusive(page), page);
>
> ret = try_grab_folio(page_folio(page), 1, flags);
> if (ret)
> @@ -899,8 +899,8 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
> goto out;
> }
>
> - VM_BUG_ON_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
> - !PageAnonExclusive(page), page);
> + VM_WARN_ON_ONCE_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
> + !PageAnonExclusive(page), page);
>
> /* try_grab_folio() does nothing unless FOLL_GET or FOLL_PIN is set. */
> ret = try_grab_folio(folio, 1, flags);
> @@ -1180,7 +1180,7 @@ static int faultin_page(struct vm_area_struct *vma,
> if (unshare) {
> fault_flags |= FAULT_FLAG_UNSHARE;
> /* FAULT_FLAG_WRITE and FAULT_FLAG_UNSHARE are incompatible */
> - VM_BUG_ON(fault_flags & FAULT_FLAG_WRITE);
> + VM_WARN_ON_ONCE(fault_flags & FAULT_FLAG_WRITE);
> }
>
> ret = handle_mm_fault(vma, address, fault_flags, NULL);
> @@ -1760,10 +1760,7 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> }
>
> /* VM_FAULT_RETRY or VM_FAULT_COMPLETED cannot return errors */
> - if (!*locked) {
> - BUG_ON(ret < 0);
> - BUG_ON(ret >= nr_pages);
> - }
> + VM_WARN_ON_ONCE(!*locked && (ret < 0 || ret >= nr_pages));
Yeah this is neater too :)
>
> if (ret > 0) {
> nr_pages -= ret;
> @@ -1808,7 +1805,6 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
>
> ret = mmap_read_lock_killable(mm);
> if (ret) {
> - BUG_ON(ret > 0);
This is a really weird one... not sure why this was ever there...
> if (!pages_done)
> pages_done = ret;
> break;
> @@ -1819,11 +1815,11 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> pages, locked);
> if (!*locked) {
> /* Continue to retry until we succeeded */
> - BUG_ON(ret != 0);
> + VM_WARN_ON_ONCE(ret != 0);
> goto retry;
> }
> if (ret != 1) {
> - BUG_ON(ret > 1);
> + VM_WARN_ON_ONCE(ret > 1);
> if (!pages_done)
> pages_done = ret;
> break;
> @@ -1885,10 +1881,10 @@ long populate_vma_page_range(struct vm_area_struct *vma,
> int gup_flags;
> long ret;
>
> - VM_BUG_ON(!PAGE_ALIGNED(start));
> - VM_BUG_ON(!PAGE_ALIGNED(end));
> - VM_BUG_ON_VMA(start < vma->vm_start, vma);
> - VM_BUG_ON_VMA(end > vma->vm_end, vma);
> + VM_WARN_ON_ONCE(!PAGE_ALIGNED(start));
> + VM_WARN_ON_ONCE(!PAGE_ALIGNED(end));
> + VM_WARN_ON_ONCE_VMA(start < vma->vm_start, vma);
> + VM_WARN_ON_ONCE_VMA(end > vma->vm_end, vma);
> mmap_assert_locked(mm);
>
> /*
> @@ -1957,8 +1953,8 @@ long faultin_page_range(struct mm_struct *mm, unsigned long start,
> int gup_flags;
> long ret;
>
> - VM_BUG_ON(!PAGE_ALIGNED(start));
> - VM_BUG_ON(!PAGE_ALIGNED(end));
> + VM_WARN_ON_ONCE(!PAGE_ALIGNED(start));
> + VM_WARN_ON_ONCE(!PAGE_ALIGNED(end));
> mmap_assert_locked(mm);
>
> /*
> @@ -2908,7 +2904,8 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
> } else if (pte_special(pte))
> goto pte_unmap;
>
> - VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
> + /* If it's not marked as special it must have a valid memmap. */
Nice to add a bit of documentation here too!
> + VM_WARN_ON_ONCE(!pfn_valid(pte_pfn(pte)));
> page = pte_page(pte);
>
> folio = try_grab_folio_fast(page, 1, flags);
>
> base-commit: 2d0c297637e7d59771c1533847c666cdddc19884
> --
> 2.49.0
>
Powered by blists - more mailing lists