[PATCH] mm/vma: fix imbalanced file reference count, properly abort mmap_prepare

[Liav Albani <liavalb@gmail.com>]

Under the new `vm_area_desc` struct, a driver may change the vm_file
to provide a different backing VM file.

In such case, it became apparent during testing of this capability, that
a remove_vma() may drop a refcount on a VM file that we didn't call
get_file() upon.

In addition to that, calling vms_abort_munmap_vmas when we didn't do
actual work is wrong, because mmap_prepare hook is about preparing the
mmap state and not performing any actual mapping (yet).

A kernel stack (before of this patch) could look like this:

imbalanced put on file reference count
WARNING: fs/file.c:36 at __file_ref_put_badval.isra.0+0x3f/0x60, CPU#1: test_proxy_mmap/116
Modules linked in: ufedm(OE) virtio_net net_failover failover nandsim nand nandcore bch mtd
CPU: 1 UID: 0 PID: 116 Comm: test_proxy_mmap Tainted: G           OE       7.2.0-rc3-gaf5e34a41cd6 #1 PREEMPT(full)  bdadc55b51524b311c430c1536a97e7071150fd3
Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.17.0-2-2 04/01/2014
RIP: 0010:__file_ref_put_badval.isra.0+0x3f/0x60
Code: 85 f6 78 05 c3 cc cc cc cc 48 b8 00 00 00 00 00 00 00 a0 48 89 07 c3 cc cc cc cc 48 83 ec 08 48 89 3c 24 48 8d 3d 61 bc 30 02 <67> 48 0f b9 3a 48 ba 00 00 00 00 00 00 00 e0 48 8b 04 24 48 89 10
RSP: 0018:ffffc900006bbae8 EFLAGS: 00010296
RAX: bfffffffffffffff RBX: ffff88810409a180 RCX: 0000000038748000
RDX: ffff888100a44480 RSI: dfffffffffffffff RDI: ffffffff83b12730
RBP: ffffc900006bbbf8 R08: 0000000000000000 R09: 00007f8b49f8f000
R10: ffffc900006bbba0 R11: ffff88810409a208 R12: ffffc900006bbba0
R13: 00007f8b4a179000 R14: ffff888102222440 R15: 0000000000015040
FS:  0000000038726400(0000) GS:ffff8881b77d1000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000038728618 CR3: 0000000104a1c006 CR4: 0000000000370ef0
Call Trace:
 <TASK>
 __file_ref_put+0x30/0x40
 fput+0x4f/0x80
 remove_vma+0x42/0x60
 vms_complete_munmap_vmas+0x179/0x230
 do_vmi_align_munmap+0x225/0x2a0
 do_vmi_munmap+0xe6/0x1c0
 __vm_munmap+0x113/0x200
 __x64_sys_munmap+0x1b/0x30
 do_syscall_64+0xaa/0x660
 ? ksys_mmap_pgoff+0x168/0x200
 ? do_syscall_64+0xaa/0x660
 ? vfs_write+0x281/0x560
 ? ksys_write+0x7b/0x110
 ? do_syscall_64+0xaa/0x660
 ? do_syscall_64+0x5f/0x660
 ? clear_bhb_loop+0x30/0x80
 ? clear_bhb_loop+0x30/0x80
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x440bcb

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 include/linux/mm_types.h |  1 +
 mm/vma.c                 | 24 +++++++++++++-----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index b18c2b2e7d2c..5db0894e6064 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -981,6 +981,7 @@ struct vm_area_struct {
 	unsigned long vm_pgoff;		/* Offset (within vm_file) in PAGE_SIZE
 					   units */
 	struct file * vm_file;		/* File we map to (can be NULL). */
+	bool vm_file_doesnt_need_put; /* Do fput only if we did get_file... */
 	void * vm_private_data;		/* was vm_pte (shared mem) */
 
 #ifdef CONFIG_SWAP
diff --git a/mm/vma.c b/mm/vma.c
index 9eea2850818a..0c2e71fc09be 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -467,7 +467,7 @@ void remove_vma(struct vm_area_struct *vma)
 {
 	might_sleep();
 	vma_close(vma);
-	if (vma->vm_file)
+	if (vma->vm_file && !vma->vm_file_doesnt_need_put)
 		fput(vma->vm_file);
 	mpol_put(vma_policy(vma));
 	vm_area_free(vma);
@@ -2487,6 +2487,7 @@ static int __mmap_new_file_vma(struct mmap_state *map,
 	int error;
 
 	vma->vm_file = map->file;
+	vma->vm_file_doesnt_need_put = map->file_doesnt_need_get;
 	if (!map->file_doesnt_need_get)
 		get_file(map->file);
 
@@ -2497,7 +2498,14 @@ static int __mmap_new_file_vma(struct mmap_state *map,
 	if (error) {
 		UNMAP_STATE(unmap, vmi, vma, vma->vm_start, vma->vm_end,
 			    map->prev, map->next);
-		fput(vma->vm_file);
+
+		/*
+		 * We set vma->vm_file to map->file, so if we didn't increase
+		 * the refcount, don't decrease it in this path.
+		 */
+		if (!map->file_doesnt_need_get)
+			fput(vma->vm_file);
+
 		vma->vm_file = NULL;
 
 		vma_iter_set(vmi, vma->vm_end);
@@ -2757,7 +2765,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 	if (!error && have_mmap_prepare)
 		error = call_mmap_prepare(&map, &desc);
 	if (error)
-		goto abort_munmap;
+		goto abort_mmap_prepare;
 
 	if (map.check_ksm_early)
 		update_ksm_flags(&map);
@@ -2795,15 +2803,9 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
 unacct_error:
 	if (map.charged)
 		vm_unacct_memory(map.charged);
-abort_munmap:
-	/*
-	 * This indicates that .mmap_prepare has set a new file, differing from
-	 * desc->vm_file. But since we're aborting the operation, only the
-	 * original file will be cleaned up. Ensure we clean up both.
-	 */
-	if (map.file_doesnt_need_get)
-		fput(map.file);
+
 	vms_abort_munmap_vmas(&map.vms, &map.mas_detach);
+abort_mmap_prepare:
 	return error;
 }
 
-- 
2.55.0