diff mbox series

[3/3] mm: check for VMA being detached before destroying it

Message ID 20230620235726.3873043-3-surenb@google.com (mailing list archive)
State New
Headers show
Series [1/3] mm: change vma_start_read to fail if VMA got detached from under it | expand

Commit Message

Suren Baghdasaryan June 20, 2023, 11:57 p.m. UTC 
By the time VMA is freed it has to be detached with the exception of
exit_mmap which is destroying the whole VMA tree. Enforce this
requirement before freeing the VMA. exit_mmap in the only user calling
__vm_area_free directly, therefore it won't trigger the new check.
Change VMA initialization to mark new VMAs as detached and change that
flag once the VMA is added into a tree.

Suggested-by: Linus Torvalds <torvalds@linuxfoundation.org>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm.h | 4 ++--
 kernel/fork.c      | 2 ++
 mm/internal.h      | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

Comments

Suren Baghdasaryan June 21, 2023, 12:05 a.m. UTC | #1 
On Tue, Jun 20, 2023 at 4:57 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> By the time VMA is freed it has to be detached with the exception of
> exit_mmap which is destroying the whole VMA tree. Enforce this
> requirement before freeing the VMA. exit_mmap in the only user calling
> __vm_area_free directly, therefore it won't trigger the new check.
> Change VMA initialization to mark new VMAs as detached and change that
> flag once the VMA is added into a tree.
>
> Suggested-by: Linus Torvalds <torvalds@linuxfoundation.org>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

My tests did not generate the warning but the test coverage is far
from perfect, so if someone can run extensive testing on this one that
would be greatly appreciated.
Thanks,
Suren.

> ---
>  include/linux/mm.h | 4 ++--
>  kernel/fork.c      | 2 ++
>  mm/internal.h      | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 74e3033c9fc2..9a10fcdb134e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -247,7 +247,7 @@ void setup_initial_init_mm(void *start_code, void *end_code,
>  struct vm_area_struct *vm_area_alloc(struct mm_struct *);
>  struct vm_area_struct *vm_area_dup(struct vm_area_struct *);
>  void vm_area_free(struct vm_area_struct *);
> -/* Use only if VMA has no other users */
> +/* Use only if VMA has no other users and might still be attached to a tree */
>  void __vm_area_free(struct vm_area_struct *vma);
>
>  #ifndef CONFIG_MMU
> @@ -751,7 +751,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>         vma->vm_mm = mm;
>         vma->vm_ops = &dummy_vm_ops;
>         INIT_LIST_HEAD(&vma->anon_vma_chain);
> -       vma_mark_detached(vma, false);
> +       vma->detached = true;
>         vma_numab_state_init(vma);
>  }
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 41c964104b58..000fc429345c 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -540,6 +540,7 @@ static void vm_area_free_rcu_cb(struct rcu_head *head)
>
>         /* The vma should not be locked while being destroyed. */
>         VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock->lock), vma);
> +       WARN_ON_ONCE(!vma->detached);
>         __vm_area_free(vma);
>  }
>  #endif
> @@ -549,6 +550,7 @@ void vm_area_free(struct vm_area_struct *vma)
>  #ifdef CONFIG_PER_VMA_LOCK
>         call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
>  #else
> +       WARN_ON_ONCE(!vma->detached);
>         __vm_area_free(vma);
>  #endif
>  }
> diff --git a/mm/internal.h b/mm/internal.h
> index 68410c6d97ac..728189e6c703 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1068,6 +1068,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
>         vmi->mas.index = vma->vm_start;
>         vmi->mas.last = vma->vm_end - 1;
>         mas_store_prealloc(&vmi->mas, vma);
> +       vma_mark_detached(vma, false);
>  }
>
>  static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
> --
> 2.41.0.162.gfafddb0af9-goog
>
kernel test robot June 21, 2023, 2:15 a.m. UTC | #2 
Hi Suren,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Suren-Baghdasaryan/mm-change-vma_start_read-to-fail-to-lock-a-detached-VMA/20230621-075833
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230620235726.3873043-3-surenb%40google.com
patch subject: [PATCH 3/3] mm: check for VMA being detached before destroying it
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230621/202306211007.hQoEsMrP-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211007.hQoEsMrP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306211007.hQoEsMrP-lkp@intel.com/

All errors (new ones prefixed by >>):

   scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr]
   scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr]
   scripts/genksyms/parse.y: note: rerun with option '-Wcounterexamples' to generate conflict counterexamples
   In file included from include/linux/pid_namespace.h:7,
                    from include/linux/ptrace.h:10,
                    from arch/alpha/kernel/asm-offsets.c:11:
   include/linux/mm.h: In function 'vma_init':
>> include/linux/mm.h:753:12: error: 'struct vm_area_struct' has no member named 'detached'
     753 |         vma->detached = true;
         |            ^~
   arch/alpha/kernel/asm-offsets.c: At top level:
   arch/alpha/kernel/asm-offsets.c:15:6: warning: no previous prototype for 'foo' [-Wmissing-prototypes]
      15 | void foo(void)
         |      ^~~
   make[2]: *** [scripts/Makefile.build:114: arch/alpha/kernel/asm-offsets.s] Error 1
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:1287: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:226: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +753 include/linux/mm.h

   740	
   741	/*
   742	 * WARNING: vma_init does not initialize vma->vm_lock.
   743	 * Use vm_area_alloc()/vm_area_free() if vma needs locking.
   744	 */
   745	static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
   746	{
   747		static const struct vm_operations_struct dummy_vm_ops = {};
   748	
   749		memset(vma, 0, sizeof(*vma));
   750		vma->vm_mm = mm;
   751		vma->vm_ops = &dummy_vm_ops;
   752		INIT_LIST_HEAD(&vma->anon_vma_chain);
 > 753		vma->detached = true;
   754		vma_numab_state_init(vma);
   755	}
   756
kernel test robot June 21, 2023, 5:53 a.m. UTC | #3 
Hi Suren,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Suren-Baghdasaryan/mm-change-vma_start_read-to-fail-to-lock-a-detached-VMA/20230621-075833
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230620235726.3873043-3-surenb%40google.com
patch subject: [PATCH 3/3] mm: check for VMA being detached before destroying it
config: sparc-randconfig-r022-20230620 (https://download.01.org/0day-ci/archive/20230621/202306211345.Xt94CVTt-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211345.Xt94CVTt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306211345.Xt94CVTt-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/sound/pcm.h:15,
                    from sound/drivers/aloop.c:27:
   include/linux/mm.h: In function 'vma_init':
   include/linux/mm.h:753:12: error: 'struct vm_area_struct' has no member named 'detached'
     753 |         vma->detached = true;
         |            ^~
   In file included from include/linux/init.h:5,
                    from sound/drivers/aloop.c:18:
   sound/drivers/aloop.c: At top level:
>> include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                   ^
   include/linux/compiler.h:231:33: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
     231 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                 ^~~~~~~~~~~~~~~~~
   include/linux/kernel.h:56:59: note: in expansion of macro '__must_be_array'
      56 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   include/linux/moduleparam.h:517:20: note: in expansion of macro 'ARRAY_SIZE'
     517 |         = { .max = ARRAY_SIZE(array), .num = nump,                      \
         |                    ^~~~~~~~~~
   include/linux/moduleparam.h:501:9: note: in expansion of macro 'module_param_array_named'
     501 |         module_param_array_named(name, name, type, nump, perm)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/aloop.c:46:1: note: in expansion of macro 'module_param_array'
      46 | module_param_array(index, int, NULL, 0444);
         | ^~~~~~~~~~~~~~~~~~
   sound/drivers/aloop.c:39:12: warning: 'index' defined but not used [-Wunused-variable]
      39 | static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;      /* Index 0-MAX */
         |            ^~~~~
--
   In file included from include/sound/pcm.h:15,
                    from sound/drivers/dummy.c:20:
   include/linux/mm.h: In function 'vma_init':
   include/linux/mm.h:753:12: error: 'struct vm_area_struct' has no member named 'detached'
     753 |         vma->detached = true;
         |            ^~
   In file included from include/linux/init.h:5,
                    from sound/drivers/dummy.c:7:
   sound/drivers/dummy.c: At top level:
>> include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                   ^
   include/linux/compiler.h:231:33: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
     231 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                 ^~~~~~~~~~~~~~~~~
   include/linux/kernel.h:56:59: note: in expansion of macro '__must_be_array'
      56 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   include/linux/moduleparam.h:517:20: note: in expansion of macro 'ARRAY_SIZE'
     517 |         = { .max = ARRAY_SIZE(array), .num = nump,                      \
         |                    ^~~~~~~~~~
   include/linux/moduleparam.h:501:9: note: in expansion of macro 'module_param_array_named'
     501 |         module_param_array_named(name, name, type, nump, perm)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
   sound/drivers/dummy.c:62:1: note: in expansion of macro 'module_param_array'
      62 | module_param_array(index, int, NULL, 0444);
         | ^~~~~~~~~~~~~~~~~~
   sound/drivers/dummy.c:48:12: warning: 'index' defined but not used [-Wunused-variable]
      48 | static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;      /* Index 0-MAX */
         |            ^~~~~


vim +16 include/linux/build_bug.h

bc6245e5efd70c Ian Abbott       2017-07-10   6  
bc6245e5efd70c Ian Abbott       2017-07-10   7  #ifdef __CHECKER__
bc6245e5efd70c Ian Abbott       2017-07-10   8  #define BUILD_BUG_ON_ZERO(e) (0)
bc6245e5efd70c Ian Abbott       2017-07-10   9  #else /* __CHECKER__ */
bc6245e5efd70c Ian Abbott       2017-07-10  10  /*
bc6245e5efd70c Ian Abbott       2017-07-10  11   * Force a compilation error if condition is true, but also produce a
8788994376d84d Rikard Falkeborn 2019-12-04  12   * result (of value 0 and type int), so the expression can be used
bc6245e5efd70c Ian Abbott       2017-07-10  13   * e.g. in a structure initializer (or where-ever else comma expressions
bc6245e5efd70c Ian Abbott       2017-07-10  14   * aren't permitted).
bc6245e5efd70c Ian Abbott       2017-07-10  15   */
8788994376d84d Rikard Falkeborn 2019-12-04 @16  #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
527edbc18a70e7 Masahiro Yamada  2019-01-03  17  #endif /* __CHECKER__ */
527edbc18a70e7 Masahiro Yamada  2019-01-03  18
Suren Baghdasaryan June 21, 2023, 7:01 a.m. UTC | #4 
On Tue, Jun 20, 2023 at 7:15 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Suren,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on akpm-mm/mm-everything]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Suren-Baghdasaryan/mm-change-vma_start_read-to-fail-to-lock-a-detached-VMA/20230621-075833
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link:    https://lore.kernel.org/r/20230620235726.3873043-3-surenb%40google.com
> patch subject: [PATCH 3/3] mm: check for VMA being detached before destroying it
> config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230621/202306211007.hQoEsMrP-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211007.hQoEsMrP-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202306211007.hQoEsMrP-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr]
>    scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr]
>    scripts/genksyms/parse.y: note: rerun with option '-Wcounterexamples' to generate conflict counterexamples
>    In file included from include/linux/pid_namespace.h:7,
>                     from include/linux/ptrace.h:10,
>                     from arch/alpha/kernel/asm-offsets.c:11:
>    include/linux/mm.h: In function 'vma_init':
> >> include/linux/mm.h:753:12: error: 'struct vm_area_struct' has no member named 'detached'
>      753 |         vma->detached = true;
>          |            ^~

Yep, missed #ifdef CONFIG_PER_VMA_LOCK here. Will fix it in the next
version but will wait a bit for possible feedback.

>    arch/alpha/kernel/asm-offsets.c: At top level:
>    arch/alpha/kernel/asm-offsets.c:15:6: warning: no previous prototype for 'foo' [-Wmissing-prototypes]
>       15 | void foo(void)
>          |      ^~~
>    make[2]: *** [scripts/Makefile.build:114: arch/alpha/kernel/asm-offsets.s] Error 1
>    make[2]: Target 'prepare' not remade because of errors.
>    make[1]: *** [Makefile:1287: prepare0] Error 2
>    make[1]: Target 'prepare' not remade because of errors.
>    make: *** [Makefile:226: __sub-make] Error 2
>    make: Target 'prepare' not remade because of errors.
>
>
> vim +753 include/linux/mm.h
>
>    740
>    741  /*
>    742   * WARNING: vma_init does not initialize vma->vm_lock.
>    743   * Use vm_area_alloc()/vm_area_free() if vma needs locking.
>    744   */
>    745  static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>    746  {
>    747          static const struct vm_operations_struct dummy_vm_ops = {};
>    748
>    749          memset(vma, 0, sizeof(*vma));
>    750          vma->vm_mm = mm;
>    751          vma->vm_ops = &dummy_vm_ops;
>    752          INIT_LIST_HEAD(&vma->anon_vma_chain);
>  > 753          vma->detached = true;
>    754          vma_numab_state_init(vma);
>    755  }
>    756
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
diff mbox series

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 74e3033c9fc2..9a10fcdb134e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -247,7 +247,7 @@  void setup_initial_init_mm(void *start_code, void *end_code,
 struct vm_area_struct *vm_area_alloc(struct mm_struct *);
 struct vm_area_struct *vm_area_dup(struct vm_area_struct *);
 void vm_area_free(struct vm_area_struct *);
-/* Use only if VMA has no other users */
+/* Use only if VMA has no other users and might still be attached to a tree */
 void __vm_area_free(struct vm_area_struct *vma);
 
 #ifndef CONFIG_MMU
@@ -751,7 +751,7 @@  static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	vma->vm_mm = mm;
 	vma->vm_ops = &dummy_vm_ops;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
-	vma_mark_detached(vma, false);
+	vma->detached = true;
 	vma_numab_state_init(vma);
 }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 41c964104b58..000fc429345c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -540,6 +540,7 @@  static void vm_area_free_rcu_cb(struct rcu_head *head)
 
 	/* The vma should not be locked while being destroyed. */
 	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock->lock), vma);
+	WARN_ON_ONCE(!vma->detached);
 	__vm_area_free(vma);
 }
 #endif
@@ -549,6 +550,7 @@  void vm_area_free(struct vm_area_struct *vma)
 #ifdef CONFIG_PER_VMA_LOCK
 	call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
 #else
+	WARN_ON_ONCE(!vma->detached);
 	__vm_area_free(vma);
 #endif
 }
diff --git a/mm/internal.h b/mm/internal.h
index 68410c6d97ac..728189e6c703 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1068,6 +1068,7 @@  static inline void vma_iter_store(struct vma_iterator *vmi,
 	vmi->mas.index = vma->vm_start;
 	vmi->mas.last = vma->vm_end - 1;
 	mas_store_prealloc(&vmi->mas, vma);
+	vma_mark_detached(vma, false);
 }
 
 static inline int vma_iter_store_gfp(struct vma_iterator *vmi,