[PATCH v3 0/2] Avoid memory corruption caused by per-VMA locks

[PATCH v3 0/2] Avoid memory corruption caused by per-VMA locks

[Suren Baghdasaryan]

A memory corruption was reported in [1] with bisection pointing to the
patch [2] enabling per-VMA locks for x86. Based on the reproducer
provided in [1] we suspect this is caused by the lack of VMA locking
while forking a child process.

Patch 1/2 in the series implements proper VMA locking during fork.
I tested the fix locally using the reproducer and was unable to reproduce
the memory corruption problem.
This fix can potentially regress some fork-heavy workloads. Kernel build
time did not show noticeable regression on a 56-core machine while a
stress test mapping 10000 VMAs and forking 5000 times in a tight loop
shows ~5% regression. If such fork time regression is unacceptable,
disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
optimizations are possible if this regression proves to be problematic.

Patch 2/2 disabled per-VMA locks until the fix is tested and verified.

Both patches apply cleanly over Linus' ToT and stable 6.4.y branch.

Changes from v2 posted at [3]:
- Move VMA locking before flush_cache_dup_mm, per David Hildenbrand

[1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
[2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
[3] https://lore.kernel.org/all/20230705063711.2670599-1-surenb@google.com/

Suren Baghdasaryan (2):
  fork: lock VMAs of the parent process when forking
  mm: disable CONFIG_PER_VMA_LOCK until its fixed

 kernel/fork.c | 6 ++++++
 mm/Kconfig    | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Suren Baghdasaryan]

When forking a child process, parent write-protects an anonymous page
and COW-shares it with the child being forked using copy_present_pte().
Parent's TLB is flushed right before we drop the parent's mmap_lock in
dup_mmap(). If we get a write-fault before that TLB flush in the parent,
and we end up replacing that anonymous page in the parent process in
do_wp_page() (because, COW-shared with the child), this might lead to
some stale writable TLB entries targeting the wrong (old) page.
Similar issue happened in the past with userfaultfd (see flush_tlb_page()
call inside do_wp_page()).
Lock VMAs of the parent process when forking a child, which prevents
concurrent page faults during fork operation and avoids this issue.
This fix can potentially regress some fork-heavy workloads. Kernel build
time did not show noticeable regression on a 56-core machine while a
stress test mapping 10000 VMAs and forking 5000 times in a tight loop
shows ~5% regression. If such fork time regression is unacceptable,
disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
optimizations are possible if this regression proves to be problematic.

Suggested-by: David Hildenbrand <david@redhat.com>
Reported-by: Jiri Slaby <jirislaby@kernel.org>
Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
Reported-by: Jacob Young <jacobly.alt@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
Cc: stable@vger.kernel.org
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 kernel/fork.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index b85814e614a5..403bc2b72301 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
 		retval = -EINTR;
 		goto fail_uprobe_end;
 	}
+#ifdef CONFIG_PER_VMA_LOCK
+	/* Disallow any page faults before calling flush_cache_dup_mm */
+	for_each_vma(old_vmi, mpnt)
+		vma_start_write(mpnt);
+	vma_iter_init(&old_vmi, oldmm, 0);
+#endif
 	flush_cache_dup_mm(oldmm);
 	uprobe_dup_mmap(oldmm, mm);
 	/*
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

A memory corruption was reported in [1] with bisection pointing to the
patch [2] enabling per-VMA locks for x86.
Disable per-VMA locks config to prevent this issue while the problem is
being investigated. This is expected to be a temporary measure.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
[2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
Reported-by: Jacob Young <jacobly.alt@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
Cc: stable@vger.kernel.org
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 mm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/Kconfig b/mm/Kconfig
index 09130434e30d..0abc6c71dd89 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
        def_bool n
 
 config PER_VMA_LOCK
-	def_bool y
+	bool "Enable per-vma locking during page fault handling."
 	depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
+	depends on BROKEN
 	help
 	  Allow per-vma locking during page fault handling.
 
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[David Hildenbrand]

On 05.07.23 19:12, Suren Baghdasaryan wrote:
> When forking a child process, parent write-protects an anonymous page
> and COW-shares it with the child being forked using copy_present_pte().
> Parent's TLB is flushed right before we drop the parent's mmap_lock in
> dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> and we end up replacing that anonymous page in the parent process in
> do_wp_page() (because, COW-shared with the child), this might lead to
> some stale writable TLB entries targeting the wrong (old) page.
> Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> call inside do_wp_page()).
> Lock VMAs of the parent process when forking a child, which prevents
> concurrent page faults during fork operation and avoids this issue.
> This fix can potentially regress some fork-heavy workloads. Kernel build
> time did not show noticeable regression on a 56-core machine while a
> stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> shows ~5% regression. If such fork time regression is unacceptable,
> disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> optimizations are possible if this regression proves to be problematic.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> Reported-by: Jacob Young <jacobly.alt@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> Cc: stable@vger.kernel.org
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>   kernel/fork.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b85814e614a5..403bc2b72301 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>   		retval = -EINTR;
>   		goto fail_uprobe_end;
>   	}
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* Disallow any page faults before calling flush_cache_dup_mm */
> +	for_each_vma(old_vmi, mpnt)
> +		vma_start_write(mpnt);
> +	vma_iter_init(&old_vmi, oldmm, 0);
> +#endif
>   	flush_cache_dup_mm(oldmm);
>   	uprobe_dup_mmap(oldmm, mm);
>   	/*

The old version was most probably fine as well, but this certainly looks 
even safer.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[David Hildenbrand]

On 05.07.23 19:12, Suren Baghdasaryan wrote:
> A memory corruption was reported in [1] with bisection pointing to the
> patch [2] enabling per-VMA locks for x86.
> Disable per-VMA locks config to prevent this issue while the problem is
> being investigated. This is expected to be a temporary measure.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> 
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> Reported-by: Jacob Young <jacobly.alt@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> Cc: stable@vger.kernel.org
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>   mm/Kconfig | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 09130434e30d..0abc6c71dd89 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
>          def_bool n
>   
>   config PER_VMA_LOCK
> -	def_bool y
> +	bool "Enable per-vma locking during page fault handling."
>   	depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> +	depends on BROKEN
>   	help
>   	  Allow per-vma locking during page fault handling.
>   
Do we have any testing results (that don't reveal other issues :) ) for 
patch #1? Not sure if we really want to mark it broken if patch #1 fixes 
the issue.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > A memory corruption was reported in [1] with bisection pointing to the
> > patch [2] enabling per-VMA locks for x86.
> > Disable per-VMA locks config to prevent this issue while the problem is
> > being investigated. This is expected to be a temporary measure.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> >
> > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >   mm/Kconfig | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 09130434e30d..0abc6c71dd89 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> >          def_bool n
> >
> >   config PER_VMA_LOCK
> > -     def_bool y
> > +     bool "Enable per-vma locking during page fault handling."
> >       depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> > +     depends on BROKEN
> >       help
> >         Allow per-vma locking during page fault handling.
> >
> Do we have any testing results (that don't reveal other issues :) ) for
> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> the issue.

I tested the fix using the only reproducer provided in the reports
plus kernel compilation and my fork stress test. All looked good and
stable but I don't know if other reports had the same issue or
something different.
I think the urgency to disable the feature stems from the timeline
being very close to when distributions will start using the 6.4 stable
kernel version.

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 10:14 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > When forking a child process, parent write-protects an anonymous page
> > and COW-shares it with the child being forked using copy_present_pte().
> > Parent's TLB is flushed right before we drop the parent's mmap_lock in
> > dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> > and we end up replacing that anonymous page in the parent process in
> > do_wp_page() (because, COW-shared with the child), this might lead to
> > some stale writable TLB entries targeting the wrong (old) page.
> > Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> > call inside do_wp_page()).
> > Lock VMAs of the parent process when forking a child, which prevents
> > concurrent page faults during fork operation and avoids this issue.
> > This fix can potentially regress some fork-heavy workloads. Kernel build
> > time did not show noticeable regression on a 56-core machine while a
> > stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> > shows ~5% regression. If such fork time regression is unacceptable,
> > disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> > optimizations are possible if this regression proves to be problematic.
> >
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> > Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >   kernel/fork.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index b85814e614a5..403bc2b72301 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >               retval = -EINTR;
> >               goto fail_uprobe_end;
> >       }
> > +#ifdef CONFIG_PER_VMA_LOCK
> > +     /* Disallow any page faults before calling flush_cache_dup_mm */
> > +     for_each_vma(old_vmi, mpnt)
> > +             vma_start_write(mpnt);
> > +     vma_iter_init(&old_vmi, oldmm, 0);
> > +#endif
> >       flush_cache_dup_mm(oldmm);
> >       uprobe_dup_mmap(oldmm, mm);
> >       /*
>
> The old version was most probably fine as well, but this certainly looks
> even safer.
>
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[David Hildenbrand]

On 05.07.23 19:22, Suren Baghdasaryan wrote:
> On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 05.07.23 19:12, Suren Baghdasaryan wrote:
>>> A memory corruption was reported in [1] with bisection pointing to the
>>> patch [2] enabling per-VMA locks for x86.
>>> Disable per-VMA locks config to prevent this issue while the problem is
>>> being investigated. This is expected to be a temporary measure.
>>>
>>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
>>> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
>>>
>>> Reported-by: Jiri Slaby <jirislaby@kernel.org>
>>> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
>>> Reported-by: Jacob Young <jacobly.alt@gmail.com>
>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
>>> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>>> ---
>>>    mm/Kconfig | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>> index 09130434e30d..0abc6c71dd89 100644
>>> --- a/mm/Kconfig
>>> +++ b/mm/Kconfig
>>> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
>>>           def_bool n
>>>
>>>    config PER_VMA_LOCK
>>> -     def_bool y
>>> +     bool "Enable per-vma locking during page fault handling."
>>>        depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
>>> +     depends on BROKEN
>>>        help
>>>          Allow per-vma locking during page fault handling.
>>>
>> Do we have any testing results (that don't reveal other issues :) ) for
>> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
>> the issue.
> 
> I tested the fix using the only reproducer provided in the reports
> plus kernel compilation and my fork stress test. All looked good and
> stable but I don't know if other reports had the same issue or
> something different.

Can you point me at the other reports, so I can quickly scan them?

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 10:24 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 05.07.23 19:22, Suren Baghdasaryan wrote:
> > On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 05.07.23 19:12, Suren Baghdasaryan wrote:
> >>> A memory corruption was reported in [1] with bisection pointing to the
> >>> patch [2] enabling per-VMA locks for x86.
> >>> Disable per-VMA locks config to prevent this issue while the problem is
> >>> being investigated. This is expected to be a temporary measure.
> >>>
> >>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> >>> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> >>>
> >>> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> >>> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> >>> Reported-by: Jacob Young <jacobly.alt@gmail.com>
> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> >>> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >>> ---
> >>>    mm/Kconfig | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/mm/Kconfig b/mm/Kconfig
> >>> index 09130434e30d..0abc6c71dd89 100644
> >>> --- a/mm/Kconfig
> >>> +++ b/mm/Kconfig
> >>> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> >>>           def_bool n
> >>>
> >>>    config PER_VMA_LOCK
> >>> -     def_bool y
> >>> +     bool "Enable per-vma locking during page fault handling."
> >>>        depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> >>> +     depends on BROKEN
> >>>        help
> >>>          Allow per-vma locking during page fault handling.
> >>>
> >> Do we have any testing results (that don't reveal other issues :) ) for
> >> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> >> the issue.
> >
> > I tested the fix using the only reproducer provided in the reports
> > plus kernel compilation and my fork stress test. All looked good and
> > stable but I don't know if other reports had the same issue or
> > something different.
>
> Can you point me at the other reports, so I can quickly scan them?

by Jacob Young: https://bugzilla.kernel.org/show_bug.cgi?id=217624
by Jiri Slaby: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
by Holger Hoffstätte:
https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
only saying that Firefox started crashing after upgrading to 6.4.1

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 11:09 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 5, 2023 at 10:24 AM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 05.07.23 19:22, Suren Baghdasaryan wrote:
> > > On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> > >>
> > >> On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > >>> A memory corruption was reported in [1] with bisection pointing to the
> > >>> patch [2] enabling per-VMA locks for x86.
> > >>> Disable per-VMA locks config to prevent this issue while the problem is
> > >>> being investigated. This is expected to be a temporary measure.
> > >>>
> > >>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > >>> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> > >>>
> > >>> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > >>> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > >>> Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > >>> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > >>> Cc: stable@vger.kernel.org
> > >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > >>> ---
> > >>>    mm/Kconfig | 3 ++-
> > >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/mm/Kconfig b/mm/Kconfig
> > >>> index 09130434e30d..0abc6c71dd89 100644
> > >>> --- a/mm/Kconfig
> > >>> +++ b/mm/Kconfig
> > >>> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> > >>>           def_bool n
> > >>>
> > >>>    config PER_VMA_LOCK
> > >>> -     def_bool y
> > >>> +     bool "Enable per-vma locking during page fault handling."
> > >>>        depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> > >>> +     depends on BROKEN
> > >>>        help
> > >>>          Allow per-vma locking during page fault handling.
> > >>>
> > >> Do we have any testing results (that don't reveal other issues :) ) for
> > >> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> > >> the issue.
> > >
> > > I tested the fix using the only reproducer provided in the reports
> > > plus kernel compilation and my fork stress test. All looked good and
> > > stable but I don't know if other reports had the same issue or
> > > something different.
> >
> > Can you point me at the other reports, so I can quickly scan them?
>
> by Jacob Young: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> by Jiri Slaby: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/

From strace in https://lore.kernel.org/all/f7ad7a42-13c8-a486-d0b7-01d5acf01e13@kernel.org/
looks like clone3() was involved, so this seems quite likely to be the
same issue I think.

> by Holger Hoffstätte:
> https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> only saying that Firefox started crashing after upgrading to 6.4.1
>
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Peter Xu]

On Wed, Jul 05, 2023 at 10:22:27AM -0700, Suren Baghdasaryan wrote:
> On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > A memory corruption was reported in [1] with bisection pointing to the
> > > patch [2] enabling per-VMA locks for x86.
> > > Disable per-VMA locks config to prevent this issue while the problem is
> > > being investigated. This is expected to be a temporary measure.
> > >
> > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> > >
> > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >   mm/Kconfig | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/Kconfig b/mm/Kconfig
> > > index 09130434e30d..0abc6c71dd89 100644
> > > --- a/mm/Kconfig
> > > +++ b/mm/Kconfig
> > > @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> > >          def_bool n
> > >
> > >   config PER_VMA_LOCK
> > > -     def_bool y
> > > +     bool "Enable per-vma locking during page fault handling."
> > >       depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> > > +     depends on BROKEN
> > >       help
> > >         Allow per-vma locking during page fault handling.
> > >
> > Do we have any testing results (that don't reveal other issues :) ) for
> > patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> > the issue.
> 
> I tested the fix using the only reproducer provided in the reports
> plus kernel compilation and my fork stress test. All looked good and
> stable but I don't know if other reports had the same issue or
> something different.

The commit log seems slightly confusing.  It mostly says the bug was still
not solved, but I assume patch 1 is the current "fix", it's just not clear
whether there's any other potential issues?

According to the stable tree rules:

 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.

I think it means vma lock will never be fixed in 6.4, and it can't (because
after this patch it'll be BROKEN, and this patch copies stable, and we
can't fix BROKEN things in stables).

Totally no problem I see, just to make sure this is what you wanted..

There'll still try to be a final fix, am I right?  As IIRC allowing page
faults during fork() is one of the major goals of vma lock.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 1:25 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jul 05, 2023 at 10:22:27AM -0700, Suren Baghdasaryan wrote:
> > On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > > A memory corruption was reported in [1] with bisection pointing to the
> > > > patch [2] enabling per-VMA locks for x86.
> > > > Disable per-VMA locks config to prevent this issue while the problem is
> > > > being investigated. This is expected to be a temporary measure.
> > > >
> > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > > [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> > > >
> > > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > ---
> > > >   mm/Kconfig | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/Kconfig b/mm/Kconfig
> > > > index 09130434e30d..0abc6c71dd89 100644
> > > > --- a/mm/Kconfig
> > > > +++ b/mm/Kconfig
> > > > @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> > > >          def_bool n
> > > >
> > > >   config PER_VMA_LOCK
> > > > -     def_bool y
> > > > +     bool "Enable per-vma locking during page fault handling."
> > > >       depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> > > > +     depends on BROKEN
> > > >       help
> > > >         Allow per-vma locking during page fault handling.
> > > >
> > > Do we have any testing results (that don't reveal other issues :) ) for
> > > patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> > > the issue.
> >
> > I tested the fix using the only reproducer provided in the reports
> > plus kernel compilation and my fork stress test. All looked good and
> > stable but I don't know if other reports had the same issue or
> > something different.
>
> The commit log seems slightly confusing.  It mostly says the bug was still
> not solved, but I assume patch 1 is the current "fix", it's just not clear
> whether there's any other potential issues?
>
> According to the stable tree rules:
>
>  - It must fix a problem that causes a build error (but not for things
>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>    security issue, or some "oh, that's not good" issue.  In short, something
>    critical.
>
> I think it means vma lock will never be fixed in 6.4, and it can't (because
> after this patch it'll be BROKEN, and this patch copies stable, and we
> can't fix BROKEN things in stables).

I was hoping we could re-enable VMA locks in 6.4 once we get more
confirmations that the problem is gone. Is that not possible once the
BROKEN dependency is merged?

>
> Totally no problem I see, just to make sure this is what you wanted..
>
> There'll still try to be a final fix, am I right?  As IIRC allowing page
> faults during fork() is one of the major goals of vma lock.

I think we can further optimize the locking rules here (see discussion
in https://lore.kernel.org/all/20230703182150.2193578-1-surenb@google.com/)
but for now we want the most effective and simple way to fix the
memory corruption problem.
Thanks,
Suren.

>
> Thanks,
>
> --
> Peter Xu
>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[David Hildenbrand]

On 05.07.23 22:25, Peter Xu wrote:
> On Wed, Jul 05, 2023 at 10:22:27AM -0700, Suren Baghdasaryan wrote:
>> On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 05.07.23 19:12, Suren Baghdasaryan wrote:
>>>> A memory corruption was reported in [1] with bisection pointing to the
>>>> patch [2] enabling per-VMA locks for x86.
>>>> Disable per-VMA locks config to prevent this issue while the problem is
>>>> being investigated. This is expected to be a temporary measure.
>>>>
>>>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
>>>> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
>>>>
>>>> Reported-by: Jiri Slaby <jirislaby@kernel.org>
>>>> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
>>>> Reported-by: Jacob Young <jacobly.alt@gmail.com>
>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
>>>> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>>>> ---
>>>>    mm/Kconfig | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>>> index 09130434e30d..0abc6c71dd89 100644
>>>> --- a/mm/Kconfig
>>>> +++ b/mm/Kconfig
>>>> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
>>>>           def_bool n
>>>>
>>>>    config PER_VMA_LOCK
>>>> -     def_bool y
>>>> +     bool "Enable per-vma locking during page fault handling."
>>>>        depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
>>>> +     depends on BROKEN
>>>>        help
>>>>          Allow per-vma locking during page fault handling.
>>>>
>>> Do we have any testing results (that don't reveal other issues :) ) for
>>> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
>>> the issue.
>>
>> I tested the fix using the only reproducer provided in the reports
>> plus kernel compilation and my fork stress test. All looked good and
>> stable but I don't know if other reports had the same issue or
>> something different.
> 
> The commit log seems slightly confusing.  It mostly says the bug was still
> not solved, but I assume patch 1 is the current "fix", it's just not clear
> whether there's any other potential issues?
> 
> According to the stable tree rules:
> 
>   - It must fix a problem that causes a build error (but not for things
>     marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>     security issue, or some "oh, that's not good" issue.  In short, something
>     critical.
> 
> I think it means vma lock will never be fixed in 6.4, and it can't (because
> after this patch it'll be BROKEN, and this patch copies stable, and we
> can't fix BROKEN things in stables).
> 
> Totally no problem I see, just to make sure this is what you wanted..
> 
> There'll still try to be a final fix, am I right?  As IIRC allowing page
> faults during fork() is one of the major goals of vma lock.

At least not that I am aware of (and people who care about that should 
really work on scalable fork() alternatives, like that io_uring fork() 
thingy).

My understanding is that CONFIG_PER_VMA_LOCK wants to speed up page 
concurrent page faults *after* fork() [or rather, after new process 
creation], IOW, when we have a lot of mmap() activity going on while 
some threads of the new process are already active and don't actually 
touch what's getting newly mmaped.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 1:37 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 05.07.23 22:25, Peter Xu wrote:
> > On Wed, Jul 05, 2023 at 10:22:27AM -0700, Suren Baghdasaryan wrote:
> >> On Wed, Jul 5, 2023 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 05.07.23 19:12, Suren Baghdasaryan wrote:
> >>>> A memory corruption was reported in [1] with bisection pointing to the
> >>>> patch [2] enabling per-VMA locks for x86.
> >>>> Disable per-VMA locks config to prevent this issue while the problem is
> >>>> being investigated. This is expected to be a temporary measure.
> >>>>
> >>>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217624
> >>>> [2] https://lore.kernel.org/all/20230227173632.3292573-30-surenb@google.com
> >>>>
> >>>> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> >>>> Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> >>>> Reported-by: Jacob Young <jacobly.alt@gmail.com>
> >>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> >>>> Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> >>>> Cc: stable@vger.kernel.org
> >>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >>>> ---
> >>>>    mm/Kconfig | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/mm/Kconfig b/mm/Kconfig
> >>>> index 09130434e30d..0abc6c71dd89 100644
> >>>> --- a/mm/Kconfig
> >>>> +++ b/mm/Kconfig
> >>>> @@ -1224,8 +1224,9 @@ config ARCH_SUPPORTS_PER_VMA_LOCK
> >>>>           def_bool n
> >>>>
> >>>>    config PER_VMA_LOCK
> >>>> -     def_bool y
> >>>> +     bool "Enable per-vma locking during page fault handling."
> >>>>        depends on ARCH_SUPPORTS_PER_VMA_LOCK && MMU && SMP
> >>>> +     depends on BROKEN
> >>>>        help
> >>>>          Allow per-vma locking during page fault handling.
> >>>>
> >>> Do we have any testing results (that don't reveal other issues :) ) for
> >>> patch #1? Not sure if we really want to mark it broken if patch #1 fixes
> >>> the issue.
> >>
> >> I tested the fix using the only reproducer provided in the reports
> >> plus kernel compilation and my fork stress test. All looked good and
> >> stable but I don't know if other reports had the same issue or
> >> something different.
> >
> > The commit log seems slightly confusing.  It mostly says the bug was still
> > not solved, but I assume patch 1 is the current "fix", it's just not clear
> > whether there's any other potential issues?
> >
> > According to the stable tree rules:
> >
> >   - It must fix a problem that causes a build error (but not for things
> >     marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
> >     security issue, or some "oh, that's not good" issue.  In short, something
> >     critical.
> >
> > I think it means vma lock will never be fixed in 6.4, and it can't (because
> > after this patch it'll be BROKEN, and this patch copies stable, and we
> > can't fix BROKEN things in stables).
> >
> > Totally no problem I see, just to make sure this is what you wanted..
> >
> > There'll still try to be a final fix, am I right?  As IIRC allowing page
> > faults during fork() is one of the major goals of vma lock.
>
> At least not that I am aware of (and people who care about that should
> really work on scalable fork() alternatives, like that io_uring fork()
> thingy).
>
> My understanding is that CONFIG_PER_VMA_LOCK wants to speed up page
> concurrent page faults *after* fork() [or rather, after new process
> creation], IOW, when we have a lot of mmap() activity going on while
> some threads of the new process are already active and don't actually
> touch what's getting newly mmaped.

Getting as much concurrency as we can is the goal. If we can allow
some page faults during fork, I would take that too. But for now let's
deploy the simplest and safest approach.

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Matthew Wilcox]

On Wed, Jul 05, 2023 at 04:25:21PM -0400, Peter Xu wrote:
> There'll still try to be a final fix, am I right?  As IIRC allowing page
> faults during fork() is one of the major goals of vma lock.

Good grief, no.  Why would we want to optimise something that happens
so rarely?  The goal is, as usual, more performance.  Satisfying page
faults while mmap()/munmap()/mprotect() are happening is worthwhile.
Those happen a lot more than fork().

In this case though, there's also a priority-inversion problem that
we're trying to solve where process A (high priority) calls mmap() while
process B (low priority) is reading /proc/$pid/smaps and now (because
rwsems are fair), none of process A's other threads can satisy any page
faults until process B is scheduled.

Where on earth did you get the idea that we cared even a little bit
about the performance of page fault during fork()?

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 2:28 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Jul 05, 2023 at 04:25:21PM -0400, Peter Xu wrote:
> > There'll still try to be a final fix, am I right?  As IIRC allowing page
> > faults during fork() is one of the major goals of vma lock.
>
> Good grief, no.  Why would we want to optimise something that happens
> so rarely?  The goal is, as usual, more performance.  Satisfying page
> faults while mmap()/munmap()/mprotect() are happening is worthwhile.
> Those happen a lot more than fork().
>
> In this case though, there's also a priority-inversion problem that
> we're trying to solve where process A (high priority) calls mmap() while
> process B (low priority) is reading /proc/$pid/smaps and now (because
> rwsems are fair), none of process A's other threads can satisy any page
> faults until process B is scheduled.
>
> Where on earth did you get the idea that we cared even a little bit
> about the performance of page fault during fork()?

I think the original reasoning for Android to improve app launch time
could have made that impression. However the speed up there comes not
from allowing page faults into the parent process (Zygote) while it
forks a child but rather from the child being able to fault pages and
establish its mappings concurrently.

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Peter Xu]

On Wed, Jul 05, 2023 at 10:27:56PM +0100, Matthew Wilcox wrote:
> On Wed, Jul 05, 2023 at 04:25:21PM -0400, Peter Xu wrote:
> > There'll still try to be a final fix, am I right?  As IIRC allowing page
> > faults during fork() is one of the major goals of vma lock.
> 
> Good grief, no.  Why would we want to optimise something that happens
> so rarely?  The goal is, as usual, more performance.  Satisfying page
> faults while mmap()/munmap()/mprotect() are happening is worthwhile.
> Those happen a lot more than fork().
> 
> In this case though, there's also a priority-inversion problem that
> we're trying to solve where process A (high priority) calls mmap() while
> process B (low priority) is reading /proc/$pid/smaps and now (because
> rwsems are fair), none of process A's other threads can satisy any page
> faults until process B is scheduled.

Is it possible to extend vma lock to things like smaps?

> 
> Where on earth did you get the idea that we cared even a little bit
> about the performance of page fault during fork()?

My memory, when I was talking to someone during the conference that
mentioned such a use case.  But my memory can be just wrong, in that case
it's my fault, but I hope it's still fine to just ask here.

-- 
Peter Xu

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Liam R. Howlett]

* Suren Baghdasaryan <surenb@google.com> [230705 13:24]:
> On Wed, Jul 5, 2023 at 10:14 AM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > When forking a child process, parent write-protects an anonymous page
> > > and COW-shares it with the child being forked using copy_present_pte().
> > > Parent's TLB is flushed right before we drop the parent's mmap_lock in
> > > dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> > > and we end up replacing that anonymous page in the parent process in
> > > do_wp_page() (because, COW-shared with the child), this might lead to
> > > some stale writable TLB entries targeting the wrong (old) page.
> > > Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> > > call inside do_wp_page()).
> > > Lock VMAs of the parent process when forking a child, which prevents
> > > concurrent page faults during fork operation and avoids this issue.
> > > This fix can potentially regress some fork-heavy workloads. Kernel build
> > > time did not show noticeable regression on a 56-core machine while a
> > > stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> > > shows ~5% regression. If such fork time regression is unacceptable,
> > > disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> > > optimizations are possible if this regression proves to be problematic.
> > >
> > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> > > Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >   kernel/fork.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index b85814e614a5..403bc2b72301 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> > >               retval = -EINTR;
> > >               goto fail_uprobe_end;
> > >       }
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +     /* Disallow any page faults before calling flush_cache_dup_mm */
> > > +     for_each_vma(old_vmi, mpnt)
> > > +             vma_start_write(mpnt);
> > > +     vma_iter_init(&old_vmi, oldmm, 0);

vma_iter_set(&old_vmi, 0) is probably what you want here.

> > > +#endif
> > >       flush_cache_dup_mm(oldmm);
> > >       uprobe_dup_mmap(oldmm, mm);
> > >       /*
> >
> > The old version was most probably fine as well, but this certainly looks
> > even safer.
> >
> > Acked-by: David Hildenbrand <david@redhat.com>

I think this is overkill and believe setting the vma_start_write() will
synchronize with any readers since it's using the per-vma rw semaphore
in write mode. Anything faulting will need to finish before the fork
continues and faults during the fork will fall back to a read lock of
the mmap_lock.  Is there a possibility of populate happening outside the
mmap_write lock/vma_lock?

Was your benchmarking done with this loop at the start?

Thanks,
Liam

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 4:07 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Suren Baghdasaryan <surenb@google.com> [230705 13:24]:
> > On Wed, Jul 5, 2023 at 10:14 AM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > > When forking a child process, parent write-protects an anonymous page
> > > > and COW-shares it with the child being forked using copy_present_pte().
> > > > Parent's TLB is flushed right before we drop the parent's mmap_lock in
> > > > dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> > > > and we end up replacing that anonymous page in the parent process in
> > > > do_wp_page() (because, COW-shared with the child), this might lead to
> > > > some stale writable TLB entries targeting the wrong (old) page.
> > > > Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> > > > call inside do_wp_page()).
> > > > Lock VMAs of the parent process when forking a child, which prevents
> > > > concurrent page faults during fork operation and avoids this issue.
> > > > This fix can potentially regress some fork-heavy workloads. Kernel build
> > > > time did not show noticeable regression on a 56-core machine while a
> > > > stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> > > > shows ~5% regression. If such fork time regression is unacceptable,
> > > > disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> > > > optimizations are possible if this regression proves to be problematic.
> > > >
> > > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> > > > Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> > > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > ---
> > > >   kernel/fork.c | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > index b85814e614a5..403bc2b72301 100644
> > > > --- a/kernel/fork.c
> > > > +++ b/kernel/fork.c
> > > > @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> > > >               retval = -EINTR;
> > > >               goto fail_uprobe_end;
> > > >       }
> > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > +     /* Disallow any page faults before calling flush_cache_dup_mm */
> > > > +     for_each_vma(old_vmi, mpnt)
> > > > +             vma_start_write(mpnt);
> > > > +     vma_iter_init(&old_vmi, oldmm, 0);
>
> vma_iter_set(&old_vmi, 0) is probably what you want here.

Ok, I send another version with that.

>
> > > > +#endif
> > > >       flush_cache_dup_mm(oldmm);
> > > >       uprobe_dup_mmap(oldmm, mm);
> > > >       /*
> > >
> > > The old version was most probably fine as well, but this certainly looks
> > > even safer.
> > >
> > > Acked-by: David Hildenbrand <david@redhat.com>
>
> I think this is overkill and believe setting the vma_start_write() will
> synchronize with any readers since it's using the per-vma rw semaphore
> in write mode. Anything faulting will need to finish before the fork
> continues and faults during the fork will fall back to a read lock of
> the mmap_lock.  Is there a possibility of populate happening outside the
> mmap_write lock/vma_lock?

Yes, I think we understand the loss of concurrency in the parent's
ability to fault pages while forking. Is that a real problem though?

>
> Was your benchmarking done with this loop at the start?

No, it was done with the initial version where the lock was inside the
existing loop. I just reran the benchmark and while kernel compilation
times did not change, the stress test shows ~7% regression now,
probably due to that additional tree walk. I'll update that number in
the new patch.
Thanks!

>
> Thanks,
> Liam

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Andrew Morton]

On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> I was hoping we could re-enable VMA locks in 6.4 once we get more
> confirmations that the problem is gone. Is that not possible once the
> BROKEN dependency is merged?

I think "no".  By doing this we're effectively backporting a minor
performance optimization, which isn't a thing we'd normally do.

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 5:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > I was hoping we could re-enable VMA locks in 6.4 once we get more
> > confirmations that the problem is gone. Is that not possible once the
> > BROKEN dependency is merged?
>
> I think "no".  By doing this we're effectively backporting a minor
> performance optimization, which isn't a thing we'd normally do.

In that case, maybe for 6.4 we send the fix and only disable it by
default without marking BROKEN? That way we still have a way to enable
it if desired?

>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 5:30 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 5, 2023 at 5:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > > I was hoping we could re-enable VMA locks in 6.4 once we get more
> > > confirmations that the problem is gone. Is that not possible once the
> > > BROKEN dependency is merged?
> >
> > I think "no".  By doing this we're effectively backporting a minor
> > performance optimization, which isn't a thing we'd normally do.
>
> In that case, maybe for 6.4 we send the fix and only disable it by
> default without marking BROKEN? That way we still have a way to enable
> it if desired?

I'm preparing the next version with Liam's corrections. If the above
option I suggested is acceptable I can send a modified second patch
which would not have BROKEN dependency.

>
> >

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Liam R. Howlett]

* Suren Baghdasaryan <surenb@google.com> [230705 20:20]:
> On Wed, Jul 5, 2023 at 4:07 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Suren Baghdasaryan <surenb@google.com> [230705 13:24]:
> > > On Wed, Jul 5, 2023 at 10:14 AM David Hildenbrand <david@redhat.com> wrote:
> > > >
> > > > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > > > When forking a child process, parent write-protects an anonymous page
> > > > > and COW-shares it with the child being forked using copy_present_pte().
> > > > > Parent's TLB is flushed right before we drop the parent's mmap_lock in
> > > > > dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> > > > > and we end up replacing that anonymous page in the parent process in
> > > > > do_wp_page() (because, COW-shared with the child), this might lead to
> > > > > some stale writable TLB entries targeting the wrong (old) page.
> > > > > Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> > > > > call inside do_wp_page()).
> > > > > Lock VMAs of the parent process when forking a child, which prevents
> > > > > concurrent page faults during fork operation and avoids this issue.
> > > > > This fix can potentially regress some fork-heavy workloads. Kernel build
> > > > > time did not show noticeable regression on a 56-core machine while a
> > > > > stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> > > > > shows ~5% regression. If such fork time regression is unacceptable,
> > > > > disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> > > > > optimizations are possible if this regression proves to be problematic.
> > > > >
> > > > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > > > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> > > > > Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> > > > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > ---
> > > > >   kernel/fork.c | 6 ++++++
> > > > >   1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > > index b85814e614a5..403bc2b72301 100644
> > > > > --- a/kernel/fork.c
> > > > > +++ b/kernel/fork.c
> > > > > @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> > > > >               retval = -EINTR;
> > > > >               goto fail_uprobe_end;
> > > > >       }
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +     /* Disallow any page faults before calling flush_cache_dup_mm */
> > > > > +     for_each_vma(old_vmi, mpnt)
> > > > > +             vma_start_write(mpnt);
> > > > > +     vma_iter_init(&old_vmi, oldmm, 0);
> >
> > vma_iter_set(&old_vmi, 0) is probably what you want here.
> 
> Ok, I send another version with that.
> 
> >
> > > > > +#endif
> > > > >       flush_cache_dup_mm(oldmm);
> > > > >       uprobe_dup_mmap(oldmm, mm);
> > > > >       /*
> > > >
> > > > The old version was most probably fine as well, but this certainly looks
> > > > even safer.
> > > >
> > > > Acked-by: David Hildenbrand <david@redhat.com>
> >
> > I think this is overkill and believe setting the vma_start_write() will
> > synchronize with any readers since it's using the per-vma rw semaphore
> > in write mode. Anything faulting will need to finish before the fork
> > continues and faults during the fork will fall back to a read lock of
> > the mmap_lock.  Is there a possibility of populate happening outside the
> > mmap_write lock/vma_lock?
> 
> Yes, I think we understand the loss of concurrency in the parent's
> ability to fault pages while forking. Is that a real problem though?

No, I don't think that part is an issue at all.  I wanted to be sure I
didn't miss something.

> 
> >
> > Was your benchmarking done with this loop at the start?
> 
> No, it was done with the initial version where the lock was inside the
> existing loop. I just reran the benchmark and while kernel compilation
> times did not change, the stress test shows ~7% regression now,
> probably due to that additional tree walk. I'll update that number in
> the new patch.

..but I expected a performance hit and didn't understand why you updated
the patch this way.  It would probably only happen on really big trees
though and, ah, the largest trees I see are from the android side.  I'd
wager the impact will be felt more when larger trees encounter smaller
CPU cache.

Thanks,
Liam

Re: [PATCH v3 1/2] fork: lock VMAs of the parent process when forking

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 5:33 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Suren Baghdasaryan <surenb@google.com> [230705 20:20]:
> > On Wed, Jul 5, 2023 at 4:07 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > >
> > > * Suren Baghdasaryan <surenb@google.com> [230705 13:24]:
> > > > On Wed, Jul 5, 2023 at 10:14 AM David Hildenbrand <david@redhat.com> wrote:
> > > > >
> > > > > On 05.07.23 19:12, Suren Baghdasaryan wrote:
> > > > > > When forking a child process, parent write-protects an anonymous page
> > > > > > and COW-shares it with the child being forked using copy_present_pte().
> > > > > > Parent's TLB is flushed right before we drop the parent's mmap_lock in
> > > > > > dup_mmap(). If we get a write-fault before that TLB flush in the parent,
> > > > > > and we end up replacing that anonymous page in the parent process in
> > > > > > do_wp_page() (because, COW-shared with the child), this might lead to
> > > > > > some stale writable TLB entries targeting the wrong (old) page.
> > > > > > Similar issue happened in the past with userfaultfd (see flush_tlb_page()
> > > > > > call inside do_wp_page()).
> > > > > > Lock VMAs of the parent process when forking a child, which prevents
> > > > > > concurrent page faults during fork operation and avoids this issue.
> > > > > > This fix can potentially regress some fork-heavy workloads. Kernel build
> > > > > > time did not show noticeable regression on a 56-core machine while a
> > > > > > stress test mapping 10000 VMAs and forking 5000 times in a tight loop
> > > > > > shows ~5% regression. If such fork time regression is unacceptable,
> > > > > > disabling CONFIG_PER_VMA_LOCK should restore its performance. Further
> > > > > > optimizations are possible if this regression proves to be problematic.
> > > > > >
> > > > > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > > > > Reported-by: Jiri Slaby <jirislaby@kernel.org>
> > > > > > Closes: https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf51b@kernel.org/
> > > > > > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> > > > > > Closes: https://lore.kernel.org/all/b198d649-f4bf-b971-31d0-e8433ec2a34c@applied-asynchrony.com/
> > > > > > Reported-by: Jacob Young <jacobly.alt@gmail.com>
> > > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217624
> > > > > > Fixes: 0bff0aaea03e ("x86/mm: try VMA lock-based page fault handling first")
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > ---
> > > > > >   kernel/fork.c | 6 ++++++
> > > > > >   1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > > > index b85814e614a5..403bc2b72301 100644
> > > > > > --- a/kernel/fork.c
> > > > > > +++ b/kernel/fork.c
> > > > > > @@ -658,6 +658,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> > > > > >               retval = -EINTR;
> > > > > >               goto fail_uprobe_end;
> > > > > >       }
> > > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > > +     /* Disallow any page faults before calling flush_cache_dup_mm */
> > > > > > +     for_each_vma(old_vmi, mpnt)
> > > > > > +             vma_start_write(mpnt);
> > > > > > +     vma_iter_init(&old_vmi, oldmm, 0);
> > >
> > > vma_iter_set(&old_vmi, 0) is probably what you want here.
> >
> > Ok, I send another version with that.
> >
> > >
> > > > > > +#endif
> > > > > >       flush_cache_dup_mm(oldmm);
> > > > > >       uprobe_dup_mmap(oldmm, mm);
> > > > > >       /*
> > > > >
> > > > > The old version was most probably fine as well, but this certainly looks
> > > > > even safer.
> > > > >
> > > > > Acked-by: David Hildenbrand <david@redhat.com>
> > >
> > > I think this is overkill and believe setting the vma_start_write() will
> > > synchronize with any readers since it's using the per-vma rw semaphore
> > > in write mode. Anything faulting will need to finish before the fork
> > > continues and faults during the fork will fall back to a read lock of
> > > the mmap_lock.  Is there a possibility of populate happening outside the
> > > mmap_write lock/vma_lock?
> >
> > Yes, I think we understand the loss of concurrency in the parent's
> > ability to fault pages while forking. Is that a real problem though?
>
> No, I don't think that part is an issue at all.  I wanted to be sure I
> didn't miss something.
>
> >
> > >
> > > Was your benchmarking done with this loop at the start?
> >
> > No, it was done with the initial version where the lock was inside the
> > existing loop. I just reran the benchmark and while kernel compilation
> > times did not change, the stress test shows ~7% regression now,
> > probably due to that additional tree walk. I'll update that number in
> > the new patch.
>
> ..but I expected a performance hit and didn't understand why you updated
> the patch this way.  It would probably only happen on really big trees
> though and, ah, the largest trees I see are from the android side.  I'd
> wager the impact will be felt more when larger trees encounter smaller
> CPU cache.

My test has 10000 vmas and even for Android that's a stretch (the
highest number I've seen was ~4000).
We can think of a less restrictive solution if this proves to be a
problem for some workloads but for now I would prefer to fix this in a
safe way and possibly improve that later. The alternative is to revert
this completely and we get no more testing until the next release.

>
> Thanks,
> Liam

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Andrew Morton]

On Wed, 5 Jul 2023 17:32:09 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> On Wed, Jul 5, 2023 at 5:30 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Wed, Jul 5, 2023 at 5:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > > I was hoping we could re-enable VMA locks in 6.4 once we get more
> > > > confirmations that the problem is gone. Is that not possible once the
> > > > BROKEN dependency is merged?
> > >
> > > I think "no".  By doing this we're effectively backporting a minor
> > > performance optimization, which isn't a thing we'd normally do.
> >
> > In that case, maybe for 6.4 we send the fix and only disable it by
> > default without marking BROKEN? That way we still have a way to enable
> > it if desired?
> 
> I'm preparing the next version with Liam's corrections. If the above
> option I suggested is acceptable I can send a modified second patch
> which would not have BROKEN dependency.

I think just mark it broken and move on.  At some later time we can
consider backporting the fixes into 6.4.x and reenabling, but I don't
think it's likely that we'll do this.

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 5:44 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 5 Jul 2023 17:32:09 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > On Wed, Jul 5, 2023 at 5:30 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > On Wed, Jul 5, 2023 at 5:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> > > >
> > > > > I was hoping we could re-enable VMA locks in 6.4 once we get more
> > > > > confirmations that the problem is gone. Is that not possible once the
> > > > > BROKEN dependency is merged?
> > > >
> > > > I think "no".  By doing this we're effectively backporting a minor
> > > > performance optimization, which isn't a thing we'd normally do.
> > >
> > > In that case, maybe for 6.4 we send the fix and only disable it by
> > > default without marking BROKEN? That way we still have a way to enable
> > > it if desired?
> >
> > I'm preparing the next version with Liam's corrections. If the above
> > option I suggested is acceptable I can send a modified second patch
> > which would not have BROKEN dependency.
>
> I think just mark it broken and move on.  At some later time we can
> consider backporting the fixes into 6.4.x and reenabling, but I don't
> think it's likely that we'll do this.

Uh, ok. I'll send the next version shortly with the patch fixing the
issue and another one marking it BROKEN. Hopefully in the next version
we can roll it our more carefully, removing BROKEN dependency but
keeping it disabled by default?

>

Re: [PATCH v3 2/2] mm: disable CONFIG_PER_VMA_LOCK until its fixed

[Suren Baghdasaryan]

On Wed, Jul 5, 2023 at 5:49 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 5, 2023 at 5:44 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 5 Jul 2023 17:32:09 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > > On Wed, Jul 5, 2023 at 5:30 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > > >
> > > > On Wed, Jul 5, 2023 at 5:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > >
> > > > > On Wed, 5 Jul 2023 13:33:26 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> > > > >
> > > > > > I was hoping we could re-enable VMA locks in 6.4 once we get more
> > > > > > confirmations that the problem is gone. Is that not possible once the
> > > > > > BROKEN dependency is merged?
> > > > >
> > > > > I think "no".  By doing this we're effectively backporting a minor
> > > > > performance optimization, which isn't a thing we'd normally do.
> > > >
> > > > In that case, maybe for 6.4 we send the fix and only disable it by
> > > > default without marking BROKEN? That way we still have a way to enable
> > > > it if desired?
> > >
> > > I'm preparing the next version with Liam's corrections. If the above
> > > option I suggested is acceptable I can send a modified second patch
> > > which would not have BROKEN dependency.
> >
> > I think just mark it broken and move on.  At some later time we can
> > consider backporting the fixes into 6.4.x and reenabling, but I don't
> > think it's likely that we'll do this.
>
> Uh, ok. I'll send the next version shortly with the patch fixing the
> issue and another one marking it BROKEN. Hopefully in the next version
> we can roll it our more carefully, removing BROKEN dependency but
> keeping it disabled by default?

v4 is posted at
https://lore.kernel.org/all/20230706011400.2949242-1-surenb@google.com/
Thanks,
Suren.

>
> >