| Message ID | 20241209221028.1644210-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mm: fix vma_copy for !CONFIG_PER_VMA_LOCK | expand |
On 2024-12-09 23:10, Suren Baghdasaryan wrote: > vma_copy() function for !CONFIG_PER_VMA_LOCK configuration copies all > fields using memcpy() as opposed to CONFIG_PER_VMA_LOCK version which > copies only required fields. anon_vma_chain field should not be copied > and new vma should instead initialize it to an empty list. Fix this > by initializing anon_vma_chain inside vma_copy() function. The version > of vma_copy() for CONFIG_PER_VMA_LOCK is fine since it does not change > that field and anon_vma_chain of any new vma is already initialized and > empty. > > Fixes: 85ad413389ae ("mm: make vma cache SLAB_TYPESAFE_BY_RCU") > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202412082208.db1fb2c9-lkp@intel.com > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/all/d0ae7609-aca4-4497-9188-bb09e96e7768@gmail.com/ > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > Applies over mm-unstable > > kernel/fork.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/fork.c b/kernel/fork.c > index fec32aa06135..d532f893e977 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -524,6 +524,7 @@ static void vma_copy(const struct vm_area_struct *src, struct vm_area_struct *de > * will be reinitialized. > */ > data_race(memcpy(dest, src, sizeof(*dest))); > + INIT_LIST_HEAD(&dest->anon_vma_chain); > } > > #endif /* CONFIG_PER_VMA_LOCK */ > > base-commit: 6e165f54437931f329d09dca6c19d99af08a36e1 This fixes the issue for me. Thanks, Tested-by: Klara Modin <klarasmodin@gmail.com>
On 09.12.24 23:10, Suren Baghdasaryan wrote: > vma_copy() function for !CONFIG_PER_VMA_LOCK configuration copies all > fields using memcpy() as opposed to CONFIG_PER_VMA_LOCK version which > copies only required fields. anon_vma_chain field should not be copied > and new vma should instead initialize it to an empty list. Fix this > by initializing anon_vma_chain inside vma_copy() function. The version > of vma_copy() for CONFIG_PER_VMA_LOCK is fine since it does not change > that field and anon_vma_chain of any new vma is already initialized and > empty. I'm wondering if there is sufficient reason to have two implementations to do the copying. How expensive would it be to simply use the CONFIG_PER_VMA_LOCK variant unconditionally? Is it even measurable in fork() micro-benchmarks?
On Mon, Dec 09, 2024 at 02:10:28PM -0800, Suren Baghdasaryan wrote: > vma_copy() function for !CONFIG_PER_VMA_LOCK configuration copies all > fields using memcpy() as opposed to CONFIG_PER_VMA_LOCK version which > copies only required fields. anon_vma_chain field should not be copied > and new vma should instead initialize it to an empty list. Fix this > by initializing anon_vma_chain inside vma_copy() function. The version > of vma_copy() for CONFIG_PER_VMA_LOCK is fine since it does not change > that field and anon_vma_chain of any new vma is already initialized and > empty. Yeah ouch, good spot though. This anon_vma stuff is landmine after landmine... > > Fixes: 85ad413389ae ("mm: make vma cache SLAB_TYPESAFE_BY_RCU") > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202412082208.db1fb2c9-lkp@intel.com > Reported-by: Klara Modin <klarasmodin@gmail.com> > Closes: https://lore.kernel.org/all/d0ae7609-aca4-4497-9188-bb09e96e7768@gmail.com/ > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > Applies over mm-unstable > > kernel/fork.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/fork.c b/kernel/fork.c > index fec32aa06135..d532f893e977 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c Unrelated to patch but it _really_ truly sucks to have this in kernel/fork.c. Maybe unavoidable given we put a bunch of this kind of logic there, but would be good to change this at some point... > @@ -524,6 +524,7 @@ static void vma_copy(const struct vm_area_struct *src, struct vm_area_struct *de > * will be reinitialized. > */ > data_race(memcpy(dest, src, sizeof(*dest))); > + INIT_LIST_HEAD(&dest->anon_vma_chain); Nit, but might be worth a comment here. Though I suppose it's kind of implicit. > } > > #endif /* CONFIG_PER_VMA_LOCK */ > > base-commit: 6e165f54437931f329d09dca6c19d99af08a36e1 > -- > 2.47.0.338.g60cca15819-goog >
On Tue, Dec 10, 2024 at 1:06 AM David Hildenbrand <david@redhat.com> wrote: > > On 09.12.24 23:10, Suren Baghdasaryan wrote: > > vma_copy() function for !CONFIG_PER_VMA_LOCK configuration copies all > > fields using memcpy() as opposed to CONFIG_PER_VMA_LOCK version which > > copies only required fields. anon_vma_chain field should not be copied > > and new vma should instead initialize it to an empty list. Fix this > > by initializing anon_vma_chain inside vma_copy() function. The version > > of vma_copy() for CONFIG_PER_VMA_LOCK is fine since it does not change > > that field and anon_vma_chain of any new vma is already initialized and > > empty. > > I'm wondering if there is sufficient reason to have two implementations > to do the copying. > > How expensive would it be to simply use the CONFIG_PER_VMA_LOCK variant > unconditionally? Is it even measurable in fork() micro-benchmarks? Yeah, the benefit if any would be tiny. I'll try measuring the difference and if it's not visible will remove the !CONFIG_PER_VMA_LOCK version. Thanks! > > -- > Cheers, > > David / dhildenb >
diff --git a/kernel/fork.c b/kernel/fork.c index fec32aa06135..d532f893e977 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -524,6 +524,7 @@ static void vma_copy(const struct vm_area_struct *src, struct vm_area_struct *de * will be reinitialized. */ data_race(memcpy(dest, src, sizeof(*dest))); + INIT_LIST_HEAD(&dest->anon_vma_chain); } #endif /* CONFIG_PER_VMA_LOCK */
vma_copy() function for !CONFIG_PER_VMA_LOCK configuration copies all fields using memcpy() as opposed to CONFIG_PER_VMA_LOCK version which copies only required fields. anon_vma_chain field should not be copied and new vma should instead initialize it to an empty list. Fix this by initializing anon_vma_chain inside vma_copy() function. The version of vma_copy() for CONFIG_PER_VMA_LOCK is fine since it does not change that field and anon_vma_chain of any new vma is already initialized and empty. Fixes: 85ad413389ae ("mm: make vma cache SLAB_TYPESAFE_BY_RCU") Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202412082208.db1fb2c9-lkp@intel.com Reported-by: Klara Modin <klarasmodin@gmail.com> Closes: https://lore.kernel.org/all/d0ae7609-aca4-4497-9188-bb09e96e7768@gmail.com/ Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- Applies over mm-unstable kernel/fork.c | 1 + 1 file changed, 1 insertion(+) base-commit: 6e165f54437931f329d09dca6c19d99af08a36e1