diff mbox series

[stable,v6.1] mm/mmap: Fix extra maple tree write

Message ID 20230706185135.2235532-1-Liam.Howlett@oracle.com
State New
Headers
Series [stable,v6.1] mm/mmap: Fix extra maple tree write |

Commit Message

Liam R. Howlett July 6, 2023, 6:51 p.m. UTC 
  commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.

This was inadvertently fixed during the removal of __vma_adjust().

When __vma_adjust() is adjusting next with a negative value (pushing
vma->vm_end lower), there would be two writes to the maple tree.  The
first write is unnecessary and uses all allocated nodes in the maple
state.  The second write is necessary but will need to allocate nodes
since the first write has used the allocated nodes.  This may be a
problem as it may not be safe to allocate at this time, such as a low
memory situation.  Fix the issue by avoiding the first write and only
write the adjusted "next" VMA.

Reported-by: John Hsu <John.Hsu@mediatek.com>
Link: https://lore.kernel.org/lkml/9cb8c599b1d7f9c1c300d1a334d5eb70ec4d7357.camel@mediatek.com/
Cc: stable@vger.kernel.org
Cc: linux-mm@kvack.org
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Greg KH July 7, 2023, 3:55 p.m. UTC | #1 
On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
> commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
> 
> This was inadvertently fixed during the removal of __vma_adjust().
> 
> When __vma_adjust() is adjusting next with a negative value (pushing
> vma->vm_end lower), there would be two writes to the maple tree.  The
> first write is unnecessary and uses all allocated nodes in the maple
> state.  The second write is necessary but will need to allocate nodes
> since the first write has used the allocated nodes.  This may be a
> problem as it may not be safe to allocate at this time, such as a low
> memory situation.  Fix the issue by avoiding the first write and only
> write the adjusted "next" VMA.

Are you sure this is the same git id?  The one you reference above is
_VERY_ different from your 2 line change below.

And the changelog text is not the same.

confused,

greg k-h
Liam R. Howlett July 7, 2023, 4:45 p.m. UTC | #2 
* Greg KH <gregkh@linuxfoundation.org> [230707 11:55]:
> On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
> > commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
> > 
> > This was inadvertently fixed during the removal of __vma_adjust().
> > 
> > When __vma_adjust() is adjusting next with a negative value (pushing
> > vma->vm_end lower), there would be two writes to the maple tree.  The
> > first write is unnecessary and uses all allocated nodes in the maple
> > state.  The second write is necessary but will need to allocate nodes
> > since the first write has used the allocated nodes.  This may be a
> > problem as it may not be safe to allocate at this time, such as a low
> > memory situation.  Fix the issue by avoiding the first write and only
> > write the adjusted "next" VMA.
> 
> Are you sure this is the same git id?  The one you reference above is
> _VERY_ different from your 2 line change below.
> 
> And the changelog text is not the same.

Yes, but I am not sure I've indicated what happened correctly.

The bug exists in the older __vma_adjust() function, but I removed
__vma_adjust() and inadvertently fixed the bug.  So the bug doesn't
exist upstream *because* of that commit:

0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")

My comment after the commit id indicates what happened, but the
documentation wasn't clear to me on how to specify what happened.

Does this answer your question?

Thanks,
Liam
Liam R. Howlett July 12, 2023, 12:54 a.m. UTC | #3 
* Liam R. Howlett <Liam.Howlett@Oracle.com> [230707 12:45]:
> * Greg KH <gregkh@linuxfoundation.org> [230707 11:55]:
> > On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
> > > commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
> > > 
> > > This was inadvertently fixed during the removal of __vma_adjust().
> > > 
> > > When __vma_adjust() is adjusting next with a negative value (pushing
> > > vma->vm_end lower), there would be two writes to the maple tree.  The
> > > first write is unnecessary and uses all allocated nodes in the maple
> > > state.  The second write is necessary but will need to allocate nodes
> > > since the first write has used the allocated nodes.  This may be a
> > > problem as it may not be safe to allocate at this time, such as a low
> > > memory situation.  Fix the issue by avoiding the first write and only
> > > write the adjusted "next" VMA.
> > 
> > Are you sure this is the same git id?  The one you reference above is
> > _VERY_ different from your 2 line change below.
> > 
> > And the changelog text is not the same.
> 
> Yes, but I am not sure I've indicated what happened correctly.
> 
> The bug exists in the older __vma_adjust() function, but I removed
> __vma_adjust() and inadvertently fixed the bug.  So the bug doesn't
> exist upstream *because* of that commit:
> 
> 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
> 
> My comment after the commit id indicates what happened, but the
> documentation wasn't clear to me on how to specify what happened.
> 
> Does this answer your question?

Friendly ping on this one?

Thanks,
Liam
Vlastimil Babka July 14, 2023, 2:44 p.m. UTC | #4 
On 7/12/23 02:54, Liam R. Howlett wrote:
> * Liam R. Howlett <Liam.Howlett@Oracle.com> [230707 12:45]:
>> * Greg KH <gregkh@linuxfoundation.org> [230707 11:55]:
>> > On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
>> > > commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
>> > > 
>> > > This was inadvertently fixed during the removal of __vma_adjust().
>> > > 
>> > > When __vma_adjust() is adjusting next with a negative value (pushing
>> > > vma->vm_end lower), there would be two writes to the maple tree.  The
>> > > first write is unnecessary and uses all allocated nodes in the maple
>> > > state.  The second write is necessary but will need to allocate nodes
>> > > since the first write has used the allocated nodes.  This may be a
>> > > problem as it may not be safe to allocate at this time, such as a low
>> > > memory situation.  Fix the issue by avoiding the first write and only
>> > > write the adjusted "next" VMA.
>> > 
>> > Are you sure this is the same git id?  The one you reference above is
>> > _VERY_ different from your 2 line change below.
>> > 
>> > And the changelog text is not the same.
>> 
>> Yes, but I am not sure I've indicated what happened correctly.

"commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream." is indeed not
the best indication. For stable it would mean you're backporting said
commit, which is not the case.

>> The bug exists in the older __vma_adjust() function, but I removed
>> __vma_adjust() and inadvertently fixed the bug.  So the bug doesn't
>> exist upstream *because* of that commit:
>> 
>> 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
>> 
>> My comment after the commit id indicates what happened, but the
>> documentation wasn't clear to me on how to specify what happened.

I think it's because the process discourages stable-specific fixes. However
this is the case where such approach is much simpler than  backporting
several series with non-trivial vma_merge() cleanups and subsequent
follow-up fixes...

So I agree with the exceptional stable-specific fix. Can you pinpoint a
Fixes: tag? Some of the commits introducing the maple tree?

Vlastimil

>> Does this answer your question?
> 
> Friendly ping on this one?
> 
> Thanks,
> Liam
>
Liam R. Howlett July 14, 2023, 3 p.m. UTC | #5 
* Vlastimil Babka <vbabka@suse.cz> [230714 10:44]:
> On 7/12/23 02:54, Liam R. Howlett wrote:
> > * Liam R. Howlett <Liam.Howlett@Oracle.com> [230707 12:45]:
> >> * Greg KH <gregkh@linuxfoundation.org> [230707 11:55]:
> >> > On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
> >> > > commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
> >> > > 
> >> > > This was inadvertently fixed during the removal of __vma_adjust().
> >> > > 
> >> > > When __vma_adjust() is adjusting next with a negative value (pushing
> >> > > vma->vm_end lower), there would be two writes to the maple tree.  The
> >> > > first write is unnecessary and uses all allocated nodes in the maple
> >> > > state.  The second write is necessary but will need to allocate nodes
> >> > > since the first write has used the allocated nodes.  This may be a
> >> > > problem as it may not be safe to allocate at this time, such as a low
> >> > > memory situation.  Fix the issue by avoiding the first write and only
> >> > > write the adjusted "next" VMA.
> >> > 
> >> > Are you sure this is the same git id?  The one you reference above is
> >> > _VERY_ different from your 2 line change below.
> >> > 
> >> > And the changelog text is not the same.
> >> 
> >> Yes, but I am not sure I've indicated what happened correctly.
> 
> "commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream." is indeed not
> the best indication. For stable it would mean you're backporting said
> commit, which is not the case.

Okay, thanks.  I am certainly not doing that.

> 
> >> The bug exists in the older __vma_adjust() function, but I removed
> >> __vma_adjust() and inadvertently fixed the bug.  So the bug doesn't
> >> exist upstream *because* of that commit:
> >> 
> >> 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
> >> 
> >> My comment after the commit id indicates what happened, but the
> >> documentation wasn't clear to me on how to specify what happened.
> 
> I think it's because the process discourages stable-specific fixes. However
> this is the case where such approach is much simpler than  backporting
> several series with non-trivial vma_merge() cleanups and subsequent
> follow-up fixes...

Thanks, and I agree.  I want to avoid that at all costs.  The chances of
introducing subtle bugs would be unacceptable for stable.

> 
> So I agree with the exceptional stable-specific fix. Can you pinpoint a
> Fixes: tag? Some of the commits introducing the maple tree?

Fixes: 3b0e81a1cdc9 ("mmap: change zeroing of maple tree in __vma_adjust()")

Thanks for the response & explanation,
Liam
Greg KH July 16, 2023, 3:05 p.m. UTC | #6 
On Tue, Jul 11, 2023 at 08:54:42PM -0400, Liam R. Howlett wrote:
> * Liam R. Howlett <Liam.Howlett@Oracle.com> [230707 12:45]:
> > * Greg KH <gregkh@linuxfoundation.org> [230707 11:55]:
> > > On Thu, Jul 06, 2023 at 02:51:35PM -0400, Liam R. Howlett wrote:
> > > > commit 0503ea8f5ba73eb3ab13a81c1eefbaf51405385a upstream.
> > > > 
> > > > This was inadvertently fixed during the removal of __vma_adjust().
> > > > 
> > > > When __vma_adjust() is adjusting next with a negative value (pushing
> > > > vma->vm_end lower), there would be two writes to the maple tree.  The
> > > > first write is unnecessary and uses all allocated nodes in the maple
> > > > state.  The second write is necessary but will need to allocate nodes
> > > > since the first write has used the allocated nodes.  This may be a
> > > > problem as it may not be safe to allocate at this time, such as a low
> > > > memory situation.  Fix the issue by avoiding the first write and only
> > > > write the adjusted "next" VMA.
> > > 
> > > Are you sure this is the same git id?  The one you reference above is
> > > _VERY_ different from your 2 line change below.
> > > 
> > > And the changelog text is not the same.
> > 
> > Yes, but I am not sure I've indicated what happened correctly.
> > 
> > The bug exists in the older __vma_adjust() function, but I removed
> > __vma_adjust() and inadvertently fixed the bug.  So the bug doesn't
> > exist upstream *because* of that commit:
> > 
> > 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
> > 
> > My comment after the commit id indicates what happened, but the
> > documentation wasn't clear to me on how to specify what happened.
> > 
> > Does this answer your question?
> 
> Friendly ping on this one?

Now queued up, thanks.

greg k-h
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index b8af52db3bbe..bb2e0ff0ef61 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -767,7 +767,8 @@  int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 	}
 	if (end != vma->vm_end) {
 		if (vma->vm_end > end) {
-			if (!insert || (insert->vm_start != end)) {
+			if ((vma->vm_end + adjust_next != end) &&
+			    (!insert || (insert->vm_start != end))) {
 				vma_mas_szero(&mas, end, vma->vm_end);
 				mas_reset(&mas);
 				VM_WARN_ON(insert &&