| Message ID | 20221228194249.170354-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] mm: fix vma->anon_name memory leak for anonymous shmem VMAs | expand |
On 28.12.22 20:42, Suren Baghdasaryan wrote: > free_anon_vma_name() is missing a check for anonymous shmem VMA which > leads to a memory leak due to refcount not being dropped. Fix this by > adding the missing check. > > Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") > Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/mm_inline.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h > index e8ed225d8f7c..d650ca2c5d29 100644 > --- a/include/linux/mm_inline.h > +++ b/include/linux/mm_inline.h > @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) > * Not using anon_vma_name because it generates a warning if mmap_lock > * is not held, which might be the case here. > */ > - if (!vma->vm_file) > + if (!vma->vm_file || vma_is_anon_shmem(vma)) > anon_vma_name_put(vma->anon_name); Wouldn't it be me more consistent to check for "vma->anon_name"? That's what dup_anon_vma_name() checks. And it's safe now because anon_name is no longer overloaded in vm_area_struct.
On Mon, Jan 2, 2023 at 4:00 AM David Hildenbrand <david@redhat.com> wrote: > > On 28.12.22 20:42, Suren Baghdasaryan wrote: > > free_anon_vma_name() is missing a check for anonymous shmem VMA which > > leads to a memory leak due to refcount not being dropped. Fix this by > > adding the missing check. > > > > Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") > > Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/mm_inline.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h > > index e8ed225d8f7c..d650ca2c5d29 100644 > > --- a/include/linux/mm_inline.h > > +++ b/include/linux/mm_inline.h > > @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) > > * Not using anon_vma_name because it generates a warning if mmap_lock > > * is not held, which might be the case here. > > */ > > - if (!vma->vm_file) > > + if (!vma->vm_file || vma_is_anon_shmem(vma)) > > anon_vma_name_put(vma->anon_name); > > Wouldn't it be me more consistent to check for "vma->anon_name"? > > That's what dup_anon_vma_name() checks. And it's safe now because > anon_name is no longer overloaded in vm_area_struct. Thanks for the suggestion, David. Yes, with the recent change that does not overload anon_name, checking for "vma->anon_name" would be simpler. I think we can also drop anon_vma_name() function now (https://elixir.bootlin.com/linux/v6.2-rc2/source/mm/madvise.c#L94) since vma->anon_name does not depend on vma->vm_file anymore, remove the last part of this comment: https://elixir.bootlin.com/linux/v6.2-rc2/source/include/linux/mm_types.h#L584 and use vma->anon_name directly going forward. If all that sounds good, I'll post a separate patch implementing all these changes. So, for this patch I would suggest keeping it as is because functionally it is correct and will change this check along with other corrections I mentioned above in a separate patch. Does that sound good? - if (!vma->vm_file) + if (!vma->vm_file || vma_is_anon_shmem(vma)) > > -- > Thanks, > > David / dhildenb >
On 03.01.23 20:53, Suren Baghdasaryan wrote: > On Mon, Jan 2, 2023 at 4:00 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 28.12.22 20:42, Suren Baghdasaryan wrote: >>> free_anon_vma_name() is missing a check for anonymous shmem VMA which >>> leads to a memory leak due to refcount not being dropped. Fix this by >>> adding the missing check. >>> >>> Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") >>> Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> >>> --- >>> include/linux/mm_inline.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h >>> index e8ed225d8f7c..d650ca2c5d29 100644 >>> --- a/include/linux/mm_inline.h >>> +++ b/include/linux/mm_inline.h >>> @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) >>> * Not using anon_vma_name because it generates a warning if mmap_lock >>> * is not held, which might be the case here. >>> */ >>> - if (!vma->vm_file) >>> + if (!vma->vm_file || vma_is_anon_shmem(vma)) >>> anon_vma_name_put(vma->anon_name); >> >> Wouldn't it be me more consistent to check for "vma->anon_name"? >> >> That's what dup_anon_vma_name() checks. And it's safe now because >> anon_name is no longer overloaded in vm_area_struct. > > Thanks for the suggestion, David. Yes, with the recent change that > does not overload anon_name, checking for "vma->anon_name" would be > simpler. I think we can also drop anon_vma_name() function now > (https://elixir.bootlin.com/linux/v6.2-rc2/source/mm/madvise.c#L94) > since vma->anon_name does not depend on vma->vm_file anymore, remove > the last part of this comment: > https://elixir.bootlin.com/linux/v6.2-rc2/source/include/linux/mm_types.h#L584 > and use vma->anon_name directly going forward. If all that sounds > good, I'll post a separate patch implementing all these changes. > So, for this patch I would suggest keeping it as is because > functionally it is correct and will change this check along with other > corrections I mentioned above in a separate patch. Does that sound > good? Works for me. Acked-by: David Hildenbrand <david@redhat.com> for this one, as it fixes the issue.
On Wed, Jan 4, 2023 at 1:04 AM David Hildenbrand <david@redhat.com> wrote: > > On 03.01.23 20:53, Suren Baghdasaryan wrote: > > On Mon, Jan 2, 2023 at 4:00 AM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 28.12.22 20:42, Suren Baghdasaryan wrote: > >>> free_anon_vma_name() is missing a check for anonymous shmem VMA which > >>> leads to a memory leak due to refcount not being dropped. Fix this by > >>> adding the missing check. > >>> > >>> Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") > >>> Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com > >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >>> --- > >>> include/linux/mm_inline.h | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h > >>> index e8ed225d8f7c..d650ca2c5d29 100644 > >>> --- a/include/linux/mm_inline.h > >>> +++ b/include/linux/mm_inline.h > >>> @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) > >>> * Not using anon_vma_name because it generates a warning if mmap_lock > >>> * is not held, which might be the case here. > >>> */ > >>> - if (!vma->vm_file) > >>> + if (!vma->vm_file || vma_is_anon_shmem(vma)) > >>> anon_vma_name_put(vma->anon_name); > >> > >> Wouldn't it be me more consistent to check for "vma->anon_name"? > >> > >> That's what dup_anon_vma_name() checks. And it's safe now because > >> anon_name is no longer overloaded in vm_area_struct. > > > > Thanks for the suggestion, David. Yes, with the recent change that > > does not overload anon_name, checking for "vma->anon_name" would be > > simpler. I think we can also drop anon_vma_name() function now > > (https://elixir.bootlin.com/linux/v6.2-rc2/source/mm/madvise.c#L94) > > since vma->anon_name does not depend on vma->vm_file anymore, remove > > the last part of this comment: > > https://elixir.bootlin.com/linux/v6.2-rc2/source/include/linux/mm_types.h#L584 > > and use vma->anon_name directly going forward. If all that sounds > > good, I'll post a separate patch implementing all these changes. > > So, for this patch I would suggest keeping it as is because > > functionally it is correct and will change this check along with other > > corrections I mentioned above in a separate patch. Does that sound > > good? > > Works for me. > > Acked-by: David Hildenbrand <david@redhat.com> Thank you! Will post the followup cleanup patch shorly. > > for this one, as it fixes the issue. > > -- > Thanks, > > David / dhildenb >
On Wed, Jan 4, 2023 at 10:24 AM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Jan 4, 2023 at 1:04 AM David Hildenbrand <david@redhat.com> wrote: > > > > On 03.01.23 20:53, Suren Baghdasaryan wrote: > > > On Mon, Jan 2, 2023 at 4:00 AM David Hildenbrand <david@redhat.com> wrote: > > >> > > >> On 28.12.22 20:42, Suren Baghdasaryan wrote: > > >>> free_anon_vma_name() is missing a check for anonymous shmem VMA which > > >>> leads to a memory leak due to refcount not being dropped. Fix this by > > >>> adding the missing check. > > >>> > > >>> Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") > > >>> Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com > > >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > >>> --- > > >>> include/linux/mm_inline.h | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h > > >>> index e8ed225d8f7c..d650ca2c5d29 100644 > > >>> --- a/include/linux/mm_inline.h > > >>> +++ b/include/linux/mm_inline.h > > >>> @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) > > >>> * Not using anon_vma_name because it generates a warning if mmap_lock > > >>> * is not held, which might be the case here. > > >>> */ > > >>> - if (!vma->vm_file) > > >>> + if (!vma->vm_file || vma_is_anon_shmem(vma)) > > >>> anon_vma_name_put(vma->anon_name); > > >> > > >> Wouldn't it be me more consistent to check for "vma->anon_name"? > > >> > > >> That's what dup_anon_vma_name() checks. And it's safe now because > > >> anon_name is no longer overloaded in vm_area_struct. > > > > > > Thanks for the suggestion, David. Yes, with the recent change that > > > does not overload anon_name, checking for "vma->anon_name" would be > > > simpler. I think we can also drop anon_vma_name() function now > > > (https://elixir.bootlin.com/linux/v6.2-rc2/source/mm/madvise.c#L94) > > > since vma->anon_name does not depend on vma->vm_file anymore, remove > > > the last part of this comment: > > > https://elixir.bootlin.com/linux/v6.2-rc2/source/include/linux/mm_types.h#L584 > > > and use vma->anon_name directly going forward. If all that sounds > > > good, I'll post a separate patch implementing all these changes. > > > So, for this patch I would suggest keeping it as is because > > > functionally it is correct and will change this check along with other > > > corrections I mentioned above in a separate patch. Does that sound > > > good? > > > > Works for me. > > > > Acked-by: David Hildenbrand <david@redhat.com> > > Thank you! Will post the followup cleanup patch shorly. Andrew, I posted v2 of this patch at https://lore.kernel.org/all/20230105000241.1450843-1-surenb@google.com/. Please replace the original one in your tree. I had to follow David's original suggestion instead of my planned cleanup because removing anon_vma_name() would require us to add #ifdef CONFIG_ANON_VMA_NAME everywhere we use vma->anon_name, which is quite ugly. Thanks, Suren. > > > > > for this one, as it fixes the issue. > > > > -- > > Thanks, > > > > David / dhildenb > >
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h index e8ed225d8f7c..d650ca2c5d29 100644 --- a/include/linux/mm_inline.h +++ b/include/linux/mm_inline.h @@ -413,7 +413,7 @@ static inline void free_anon_vma_name(struct vm_area_struct *vma) * Not using anon_vma_name because it generates a warning if mmap_lock * is not held, which might be the case here. */ - if (!vma->vm_file) + if (!vma->vm_file || vma_is_anon_shmem(vma)) anon_vma_name_put(vma->anon_name); }
free_anon_vma_name() is missing a check for anonymous shmem VMA which leads to a memory leak due to refcount not being dropped. Fix this by adding the missing check. Fixes: d09e8ca6cb93 ("mm: anonymous shared memory naming") Reported-by: syzbot+91edf9178386a07d06a7@syzkaller.appspotmail.com Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/mm_inline.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)