When exit_mmap() removes vmas belonging to an exiting task, it does not
mark them as detached since they can't be reached by other tasks and they
will be freed shortly. Once we introduce vma reuse, all vmas will have to
be in detached state before they are freed to ensure vma when reused is
in a consistent state. Add missing vma_mark_detached() before freeing the
vma.
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/vma.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/mm/vma.c b/mm/vma.c
index b9cf552e120c..93ff42ac2002 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -413,10 +413,12 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
if (vma->vm_file)
fput(vma->vm_file);
mpol_put(vma_policy(vma));
- if (unreachable)
+ if (unreachable) {
+ vma_mark_detached(vma);
__vm_area_free(vma);
- else
+ } else {
vm_area_free(vma);
+ }
}
/*
--
2.47.1.613.gc27f4b7a9f-googOn Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote:
> When exit_mmap() removes vmas belonging to an exiting task, it does not
> mark them as detached since they can't be reached by other tasks and they
> will be freed shortly. Once we introduce vma reuse, all vmas will have to
> be in detached state before they are freed to ensure vma when reused is
> in a consistent state. Add missing vma_mark_detached() before freeing the
> vma.
Hmm this really makes me worry that we'll see bugs from this detached
stuff, do we make this assumption anywhere else I wonder?
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
But regardless, prima facie, this looks fine, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> mm/vma.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index b9cf552e120c..93ff42ac2002 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -413,10 +413,12 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
> if (vma->vm_file)
> fput(vma->vm_file);
> mpol_put(vma_policy(vma));
> - if (unreachable)
> + if (unreachable) {
> + vma_mark_detached(vma);
> __vm_area_free(vma);
> - else
> + } else {
> vm_area_free(vma);
> + }
> }
>
> /*
> --
> 2.47.1.613.gc27f4b7a9f-goog
>
On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote:
> > When exit_mmap() removes vmas belonging to an exiting task, it does not
> > mark them as detached since they can't be reached by other tasks and they
> > will be freed shortly. Once we introduce vma reuse, all vmas will have to
> > be in detached state before they are freed to ensure vma when reused is
> > in a consistent state. Add missing vma_mark_detached() before freeing the
> > vma.
>
> Hmm this really makes me worry that we'll see bugs from this detached
> stuff, do we make this assumption anywhere else I wonder?
This is the only place which does not currently detach the vma before
freeing it. If someone tries adding a case like that in the future,
they will be met with vma_assert_detached() inside vm_area_free().
>
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
> But regardless, prima facie, this looks fine, so:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> > ---
> > mm/vma.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index b9cf552e120c..93ff42ac2002 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -413,10 +413,12 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
> > if (vma->vm_file)
> > fput(vma->vm_file);
> > mpol_put(vma_policy(vma));
> > - if (unreachable)
> > + if (unreachable) {
> > + vma_mark_detached(vma);
> > __vm_area_free(vma);
> > - else
> > + } else {
> > vm_area_free(vma);
> > + }
> > }
> >
> > /*
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
> >
On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote:
> On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote:
> > > When exit_mmap() removes vmas belonging to an exiting task, it does not
> > > mark them as detached since they can't be reached by other tasks and they
> > > will be freed shortly. Once we introduce vma reuse, all vmas will have to
> > > be in detached state before they are freed to ensure vma when reused is
> > > in a consistent state. Add missing vma_mark_detached() before freeing the
> > > vma.
> >
> > Hmm this really makes me worry that we'll see bugs from this detached
> > stuff, do we make this assumption anywhere else I wonder?
>
> This is the only place which does not currently detach the vma before
> freeing it. If someone tries adding a case like that in the future,
> they will be met with vma_assert_detached() inside vm_area_free().
OK good to know!
Again, I wonder if we should make these assertions stronger as commented
elsewhere, because if we see them in production isn't that worth an actual
non-debug WARN_ON_ONCE()?
>
> >
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> >
> > But regardless, prima facie, this looks fine, so:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > > ---
> > > mm/vma.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/vma.c b/mm/vma.c
> > > index b9cf552e120c..93ff42ac2002 100644
> > > --- a/mm/vma.c
> > > +++ b/mm/vma.c
> > > @@ -413,10 +413,12 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
> > > if (vma->vm_file)
> > > fput(vma->vm_file);
> > > mpol_put(vma_policy(vma));
> > > - if (unreachable)
> > > + if (unreachable) {
> > > + vma_mark_detached(vma);
> > > __vm_area_free(vma);
> > > - else
> > > + } else {
> > > vm_area_free(vma);
> > > + }
> > > }
> > >
> > > /*
> > > --
> > > 2.47.1.613.gc27f4b7a9f-goog
> > >
On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote:
> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote:
> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not
> > > > mark them as detached since they can't be reached by other tasks and they
> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to
> > > > be in detached state before they are freed to ensure vma when reused is
> > > > in a consistent state. Add missing vma_mark_detached() before freeing the
> > > > vma.
> > >
> > > Hmm this really makes me worry that we'll see bugs from this detached
> > > stuff, do we make this assumption anywhere else I wonder?
> >
> > This is the only place which does not currently detach the vma before
> > freeing it. If someone tries adding a case like that in the future,
> > they will be met with vma_assert_detached() inside vm_area_free().
>
> OK good to know!
>
> Again, I wonder if we should make these assertions stronger as commented
> elsewhere, because if we see them in production isn't that worth an actual
> non-debug WARN_ON_ONCE()?
Sure. I'll change vma_assert_attached()/vma_assert_detached() to use
WARN_ON_ONCE() and to return a bool (see also my reply in the patch
[0/17]).
>
> >
> > >
> > > >
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> > >
> > > But regardless, prima facie, this looks fine, so:
> > >
> > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > >
> > > > ---
> > > > mm/vma.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/mm/vma.c b/mm/vma.c
> > > > index b9cf552e120c..93ff42ac2002 100644
> > > > --- a/mm/vma.c
> > > > +++ b/mm/vma.c
> > > > @@ -413,10 +413,12 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
> > > > if (vma->vm_file)
> > > > fput(vma->vm_file);
> > > > mpol_put(vma_policy(vma));
> > > > - if (unreachable)
> > > > + if (unreachable) {
> > > > + vma_mark_detached(vma);
> > > > __vm_area_free(vma);
> > > > - else
> > > > + } else {
> > > > vm_area_free(vma);
> > > > + }
> > > > }
> > > >
> > > > /*
> > > > --
> > > > 2.47.1.613.gc27f4b7a9f-goog
> > > >
On 1/13/25 20:11, Suren Baghdasaryan wrote: > On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: >> >> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote: >> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes >> > <lorenzo.stoakes@oracle.com> wrote: >> > > >> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote: >> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not >> > > > mark them as detached since they can't be reached by other tasks and they >> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to >> > > > be in detached state before they are freed to ensure vma when reused is >> > > > in a consistent state. Add missing vma_mark_detached() before freeing the >> > > > vma. >> > > >> > > Hmm this really makes me worry that we'll see bugs from this detached >> > > stuff, do we make this assumption anywhere else I wonder? >> > >> > This is the only place which does not currently detach the vma before >> > freeing it. If someone tries adding a case like that in the future, >> > they will be met with vma_assert_detached() inside vm_area_free(). >> >> OK good to know! >> >> Again, I wonder if we should make these assertions stronger as commented >> elsewhere, because if we see them in production isn't that worth an actual >> non-debug WARN_ON_ONCE()? > > Sure. I'll change vma_assert_attached()/vma_assert_detached() to use > WARN_ON_ONCE() and to return a bool (see also my reply in the patch > [0/17]). So is this a case of "someone might introduce code later that will violate them" as alluded to above? Unconditional WARN_ON_ONCE seems too much then. In general it's not easy to determine how paranoid we should be in non-debug code, but I'm not sure what's the need here specifically.
On Mon, Jan 13, 2025 at 12:32 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 1/13/25 20:11, Suren Baghdasaryan wrote: > > On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > >> > >> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote: > >> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes > >> > <lorenzo.stoakes@oracle.com> wrote: > >> > > > >> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote: > >> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not > >> > > > mark them as detached since they can't be reached by other tasks and they > >> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to > >> > > > be in detached state before they are freed to ensure vma when reused is > >> > > > in a consistent state. Add missing vma_mark_detached() before freeing the > >> > > > vma. > >> > > > >> > > Hmm this really makes me worry that we'll see bugs from this detached > >> > > stuff, do we make this assumption anywhere else I wonder? > >> > > >> > This is the only place which does not currently detach the vma before > >> > freeing it. If someone tries adding a case like that in the future, > >> > they will be met with vma_assert_detached() inside vm_area_free(). > >> > >> OK good to know! > >> > >> Again, I wonder if we should make these assertions stronger as commented > >> elsewhere, because if we see them in production isn't that worth an actual > >> non-debug WARN_ON_ONCE()? > > > > Sure. I'll change vma_assert_attached()/vma_assert_detached() to use > > WARN_ON_ONCE() and to return a bool (see also my reply in the patch > > [0/17]). > > So is this a case of "someone might introduce code later that will violate > them" as alluded to above? Unconditional WARN_ON_ONCE seems too much then. Yes, I wanted to make sure refcounting will not be broken by someone doing re-attach/re-detach. > > In general it's not easy to determine how paranoid we should be in non-debug > code, but I'm not sure what's the need here specifically. I'm not sure how strict we should be but we definitely should try to catch refcounting mistakes and that's my goal here. >
On Mon, Jan 13, 2025 at 12:42:53PM -0800, Suren Baghdasaryan wrote: > On Mon, Jan 13, 2025 at 12:32 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > > > On 1/13/25 20:11, Suren Baghdasaryan wrote: > > > On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes > > > <lorenzo.stoakes@oracle.com> wrote: > > >> > > >> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote: > > >> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes > > >> > <lorenzo.stoakes@oracle.com> wrote: > > >> > > > > >> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote: > > >> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not > > >> > > > mark them as detached since they can't be reached by other tasks and they > > >> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to > > >> > > > be in detached state before they are freed to ensure vma when reused is > > >> > > > in a consistent state. Add missing vma_mark_detached() before freeing the > > >> > > > vma. > > >> > > > > >> > > Hmm this really makes me worry that we'll see bugs from this detached > > >> > > stuff, do we make this assumption anywhere else I wonder? > > >> > > > >> > This is the only place which does not currently detach the vma before > > >> > freeing it. If someone tries adding a case like that in the future, > > >> > they will be met with vma_assert_detached() inside vm_area_free(). > > >> > > >> OK good to know! > > >> > > >> Again, I wonder if we should make these assertions stronger as commented > > >> elsewhere, because if we see them in production isn't that worth an actual > > >> non-debug WARN_ON_ONCE()? > > > > > > Sure. I'll change vma_assert_attached()/vma_assert_detached() to use > > > WARN_ON_ONCE() and to return a bool (see also my reply in the patch > > > [0/17]). > > > > So is this a case of "someone might introduce code later that will violate > > them" as alluded to above? Unconditional WARN_ON_ONCE seems too much then. My concern is that there is a broken case that remains hidden because nothing is actually checked in production, which would then become really difficult to debug should somebody report it. We intend the WARN_ONxxx() functions to be asserting things that -should not be- for precisely this kind of purpose so I think it makes sense here. > > Yes, I wanted to make sure refcounting will not be broken by someone > doing re-attach/re-detach. Yes, and debugging this without it could be really horrible. > > > > > In general it's not easy to determine how paranoid we should be in non-debug > > code, but I'm not sure what's the need here specifically. > > I'm not sure how strict we should be but we definitely should try to > catch refcounting mistakes and that's my goal here. Yes I think it is worth it here (obviously :) > > >
© 2016 - 2026 Red Hat, Inc.