Fabian has reported another regression in 6.1 due to ca3d76b0aa80 ("mm:
add merging after mremap resize"). The problem is that vma_merge() can
fail when vma has a vm_ops->close() method, causing is_mergeable_vma()
test to be negative. This was happening for vma mapping a file from
fuse-overlayfs, which does have the method. But when we are simply
expanding the vma, we never remove it due to the "merge" with the added
area, so the test should not prevent the expansion.
As a quick fix, check for such vmas and expand them using vma_adjust()
directly as was done before commit ca3d76b0aa80. For a more robust long
term solution we should try to limit the check for vma_ops->close only
to cases that actually result in vma removal, so that no merge would be
prevented unnecessarily.
Reported-by: Fabian Vogt <[email protected]>
Link: https://bugzilla.suse.com/show_bug.cgi?id=1206359#c35
Fixes: ca3d76b0aa80 ("mm: add merging after mremap resize")
Signed-off-by: Vlastimil Babka <[email protected]>
Cc: Jakub Matěna <[email protected]>
Cc: <[email protected]>
Tested-by: Fabian Vogt <[email protected]>
---
Thorsten: this should be added to the previous regression which wasn't
fully fixed by the previous patch:
https://linux-regtracking.leemhuis.info/regzbot/regression/[email protected]/
mm/mremap.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/mm/mremap.c b/mm/mremap.c
index fe587c5d6591..1e234fd95547 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1032,11 +1032,22 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
* the already existing vma (expand operation itself) and possibly also
* with the next vma if it becomes adjacent to the expanded vma and
* otherwise compatible.
+ *
+ * However, vma_merge() can currently fail due to is_mergeable_vma()
+ * check for vm_ops->close (see the comment there). Yet this should not
+ * prevent vma expanding, so perform a simple expand for such vma.
+ * Ideally the check for close op should be only done when a vma would
+ * be actually removed due to a merge.
*/
- vma = vma_merge(mm, vma, extension_start, extension_end,
+ if (!vma->vm_ops || !vma->vm_ops->close) {
+ vma = vma_merge(mm, vma, extension_start, extension_end,
vma->vm_flags, vma->anon_vma, vma->vm_file,
extension_pgoff, vma_policy(vma),
vma->vm_userfaultfd_ctx, anon_vma_name(vma));
+ } else if (vma_adjust(vma, vma->vm_start, addr + new_len,
+ vma->vm_pgoff, NULL)) {
+ vma = NULL;
+ }
if (!vma) {
vm_unacct_memory(pages);
ret = -ENOMEM;
--
2.38.1
On 17.01.23 11:19, Vlastimil Babka wrote:
> Fabian has reported another regression in 6.1 due to ca3d76b0aa80 ("mm:
> add merging after mremap resize"). The problem is that vma_merge() can
> fail when vma has a vm_ops->close() method, causing is_mergeable_vma()
> test to be negative. This was happening for vma mapping a file from
> fuse-overlayfs, which does have the method. But when we are simply
> expanding the vma, we never remove it due to the "merge" with the added
> area, so the test should not prevent the expansion.
>
> As a quick fix, check for such vmas and expand them using vma_adjust()
> directly as was done before commit ca3d76b0aa80. For a more robust long
> term solution we should try to limit the check for vma_ops->close only
> to cases that actually result in vma removal, so that no merge would be
> prevented unnecessarily.
>
> Reported-by: Fabian Vogt <[email protected]>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1206359#c35
> Fixes: ca3d76b0aa80 ("mm: add merging after mremap resize")
> Signed-off-by: Vlastimil Babka <[email protected]>
> Cc: Jakub Matěna <[email protected]>
> Cc: <[email protected]>
> Tested-by: Fabian Vogt <[email protected]>
> ---
Thx for highlighting it and CCing me.
Quick question: how fast do you think this should head towards mainline?
The patch landed in next today, so that step in the process is already
covered. But is the issue serious enough to say "send this to Linus
after it was a day or two in next, so it can be quickly backported to
stable"?
> Thorsten: this should be added to the previous regression which wasn't
> fully fixed by the previous patch:
> https://linux-regtracking.leemhuis.info/regzbot/regression/[email protected]/
> mm/mremap.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> [...]
In that case let me just briefly drop a link to the regression, as
regzbot will notice that and file is as an activity.
https://lore.kernel.org/lkml/[email protected]/
And simply consider your patch submission as a new report I track
separately:
#regzbot introduced ca3d76b0aa80 ^
https://bugzilla.suse.com/show_bug.cgi?id=1206359#c35
#regzbot title mm, mremap: another issue with mremap not fully fixed
with the previous fix for the regression
#regzbot fix: mm, mremap: fix mremap() expanding for vma's with
vm_ops->close()
#regzbot ignore-activity
Not ideal, but that will make sure it's on regzbot radar (where way too
many dots appear currently, as I'm a bit behind with things... :-/ )
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
On 1/19/23 14:37, Linux kernel regression tracking (Thorsten Leemhuis) wrote:
> On 17.01.23 11:19, Vlastimil Babka wrote:
>> Fabian has reported another regression in 6.1 due to ca3d76b0aa80 ("mm:
>> add merging after mremap resize"). The problem is that vma_merge() can
>> fail when vma has a vm_ops->close() method, causing is_mergeable_vma()
>> test to be negative. This was happening for vma mapping a file from
>> fuse-overlayfs, which does have the method. But when we are simply
>> expanding the vma, we never remove it due to the "merge" with the added
>> area, so the test should not prevent the expansion.
>>
>> As a quick fix, check for such vmas and expand them using vma_adjust()
>> directly as was done before commit ca3d76b0aa80. For a more robust long
>> term solution we should try to limit the check for vma_ops->close only
>> to cases that actually result in vma removal, so that no merge would be
>> prevented unnecessarily.
>>
>> Reported-by: Fabian Vogt <[email protected]>
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1206359#c35
>> Fixes: ca3d76b0aa80 ("mm: add merging after mremap resize")
>> Signed-off-by: Vlastimil Babka <[email protected]>
>> Cc: Jakub Matěna <[email protected]>
>> Cc: <[email protected]>
>> Tested-by: Fabian Vogt <[email protected]>
>> ---
>
> Thx for highlighting it and CCing me.
>
> Quick question: how fast do you think this should head towards mainline?
>
> The patch landed in next today, so that step in the process is already
> covered. But is the issue serious enough to say "send this to Linus
> after it was a day or two in next, so it can be quickly backported to
> stable"?
I think it's not as serious as the previous one, the conditions should be
more rare. But you made me realize I should probably reply to the "stalls in
qemu" one in that sense. Thanks!
>> Thorsten: this should be added to the previous regression which wasn't
>> fully fixed by the previous patch:
>> https://linux-regtracking.leemhuis.info/regzbot/regression/[email protected]/
>> mm/mremap.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>> [...]
>
> In that case let me just briefly drop a link to the regression, as
> regzbot will notice that and file is as an activity.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> And simply consider your patch submission as a new report I track
> separately:
>
> #regzbot introduced ca3d76b0aa80 ^
> https://bugzilla.suse.com/show_bug.cgi?id=1206359#c35
> #regzbot title mm, mremap: another issue with mremap not fully fixed
> with the previous fix for the regression
> #regzbot fix: mm, mremap: fix mremap() expanding for vma's with
> vm_ops->close()
> #regzbot ignore-activity
>
> Not ideal, but that will make sure it's on regzbot radar (where way too
> many dots appear currently, as I'm a bit behind with things... :-/ )
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.