Fix an imbalanced file ref count in the mmap syscall
Subsystems (3)
cover Fix an imbalanced file ref count in the mmap syscall
Hi!
This is my first patch that I contribute myself to the Linux kernel. My apologize if there are issues, I tried to solve this issue in one evening and understand the call graph and what could go wrong.
I work recently on a project of mine called nfiop, and specifically on a kernel module called ufedm - https://github.com/nfiop/ufedm .
The project has a driver that allows a userspace implementation to intervene in the write and read callbacks of an MTD device by spawning an upper MTD device that sends the data back and forth to userspace, so userspace can implement its custom ECC engine and send back the results.
So far so good. I started working with kernel 6.18.8 on an Olimex A20 board and then moved to my x86 linux machine with a nandsim simulation. When I started my project, I implemented a memory backing based on a shmem file for the shared memory buffer, that is expected to be mmap'ed from my custom character device.
When I updated my host machine, the new kernel version was 7.x so I decided to update my module as well.
Many changes were comsetic, but the major change was to implement a mmap_prepare hook.
I wrote something simple, taking notes on the mmap_prepare rules and the guide from kernel docs.
Then I started to test that my character device is at least working fine, so I have my unit tests for ioctls and mmap, and the mmap unit-test failed after the second-run.
A kernel stack looked 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 ```
I started investigating this, and it became apparent that there's an issue with ref-counting of the vm_file - I changed it in my mmap_prepare hook, so it must be the cause, right?
I also tried to artificially increment the ref-count of my shmem file when I create it (taking another ref), so the second attempt didn't fail but the third did...
I looked at the stack trace, and the culprit is the fact that we do fput on a vm_file that we didn't pin - to fix this, I added a flag if we actually did get_file() on the vm_file or not, on the vma struct.
When testing this patch, I've observed that the problem was completely gone.
The mmap_prepare hook is still quite new, so presumably fixing corner cases like this one is a good candidate for upstream, for future drivers that might do the same as I do in my driver.
Liav Albani (1):
mm/vma: fix imbalanced file reference count, properly abort
mmap_prepare
include/linux/mm_types.h | 1 + mm/vma.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)
Signature · 1 line
2.55.0
PATCH 1/1 mm/vma: fix imbalanced file reference count, properly abort mmap_prepare
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
--- include/linux/mm_types.h | 1 + mm/vma.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)
include/linux/mm_types.h +1 -0
@@ -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
mm/vma.c +13 -11
@@ -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; }
Signature · 1 line
2.55.0
Lorenzo Stoakes (ARM)
maintainerRe: [PATCH] mm/vma: fix imbalanced file reference count, properly abort mmap_prepare
Thanks for the patch but as per the 0/1 this is not correct - the issue is that your mmap_prepare hook doesn't get the file.
(Also in general, we don't add cover letters for single patches, only for series with >1 patch :)
Quoted 6 lines · Liav Albani
Under the new
vm_area_descstruct, a driver may change thevm_fileto 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 callget_file()upon.
As above.
Quoted 4 lines
In addition to that, calling
vms_abort_munmap_vmaswhen we didn't do actual work is wrong, becausemmap_preparehook is about preparing the mmap state and not performing any actual mapping (yet).
Not the case - __mmap_setup() unmaps any existing mappings in the area being mapped into (they are taken out of the maple tree and put into a detached state then added to the temporary map->mt_detach tree), and vms_abort_munmap_vmas() will put them back.
The vms data structure is set up in int_vma_munmap(), then vms_gather_munmap_vmas() populates both map->vms and map->mas_detach.
If nothing was unmapped map->vms->nr_pages == 0, and vms_abort_munmap_vmas() is a noop.
Quoted 53 lines
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:0x440bcbSigned-off-by Liav Albani <[email protected]>--- include/linux/mm_types.h | 1 + mm/vma.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)include/linux/mm_types.h +0
-0
include/linux/mm_types.h struct vm_area_struct { · line 984
Quoted 4 lines
include/linux/mm_types.h +1
-0@@ -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... */
We definitely wouldn't add a field like this in the VMA type :) VMA fields are in very short supply and the data structure is very sensitive on size (it scales to a lot on many systems with many threads).
In general it feels like this is to pass things down to remove_vma(), but that's an indicator really that you need to put it somehwere else.
Quoted 27 lines
include/linux/mm_types.h +0
-0void * vm_private_data;/* was vm_pte (shared mem) */ #ifdef CONFIG_SWAP mm/vma.c +2
-1@@ -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,
mm/vma.c static int __mmap_new_file_vma(struct mmap_state *map, · line 2508
Quoted 10 lines
mm/vma.c +8
-1map->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); +
But this just means the file is unpinned in general and you really want its lifetime to be at least that of the mapping :>)
mm/vma.c static unsigned long __mmap_region(struct file *file, unsigned long addr, · line 2768
Quoted 9 lines
mm/vma.c +1
-1vma->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;
Yeah, as above, this is incorrect.
Quoted 22 lines
mm/vma.c +2
-9if (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; } --
Quoted 2 lines
2.55.0
In general I can see you're a genuine kernel enthusiast so I don't want to dissuade you :)
This code is fiddly in general so it's _entirely_ understandable you had some misapprehensions about it.
Hopefully the feedback helps and you are able to continue implementing your feature, do let me know if you're able to do what you need with your driver (mmap_prepare is in live development and I'm gradually adding more actions to suit actual driver usage).
It's actually nice to see something human rather than LLM for some early contributions :)
Cheers, Lorenzo
Re: [PATCH] mm/vma: fix imbalanced file reference count, properly abort mmap_prepare
Hi and thanks for your reply (both of you)!
I see that mmap_prepare essentially creates a shmem_file on each mmap call for /dev/zero, so it's like donating a ref, right?
I will try that because it looks good to me :)
There is the second part to this patch and I was about to ask if we actually want to do the abort_munmap path when aborting because of failed mmap_prepare
and it seems like Lorenzo already gave the answer to this :)
I will let you all know about the results and see if it works fine.
Again thanks for your help and time!
Liav
בתאריך שבת, 18 ביולי 2026 ב-17:18 מאת Lorenzo Stoakes (ARM) < [email protected]>:
Quoted 209 lines
Thanks for the patch but as per the 0/1 this is not correct - the issue is that
yourmmap_preparehook doesn't get the file.(Also in general, we don't add cover letters for single patches, only for series
with >1 patch :)Quoted 6 lines · Liav Albani
Under the new
vm_area_descstruct, a driver may change thevm_fileto 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 callget_file()upon.As above.
Quoted 4 lines
In addition to that, calling
vms_abort_munmap_vmaswhen we didn't do actual work is wrong, becausemmap_preparehook is about preparing the mmap state and not performing any actual mapping (yet).Not the case -
__mmap_setup()unmaps any existing mappings in the area being mapped into (they are taken out of the maple tree and put into a detached state then added to the temporarymap->mt_detachtree), andvms_abort_munmap_vmas()will put them back.The vms data structure is set up in
int_vma_munmap(), thenvms_gather_munmap_vmas()populates bothmap->vmsandmap->mas_detach.If nothing was unmapped
map->vms->nr_pages== 0, andvms_abort_munmap_vmas()is a noop.Quoted 5 lines
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
Quoted 1 line
Modules linked in: ufedm(OE) virtio_net net_failover failover nandsimnand nandcore bch mtd
Quoted 1 line
CPU: 1 UID: 0 PID: 116 Comm: test_proxy_mmap Tainted: G OE7.2.0-rc3-gaf5e34a41cd6 #1 PREEMPT(full)
bdadc55b51524b311c430c1536a97e7071150fd3Quoted 2 lines
Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux1.17.0-2-2 04/01/2014
Quoted 2 lines
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 07c3 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
Quoted 7 lines
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
Quoted 35 lines
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:0x440bcbSigned-off-by Liav Albani <[email protected]>--- include/linux/mm_types.h | 1 + mm/vma.c | 24 +++++++++++++----------- 2 files changed, 14 insertions(+), 11 deletions(-)include/linux/mm_types.h +0
-0include/linux/mm_types.h struct vm_area_struct { · line 981
Quoted 1 line
include/linux/mm_types.h +0
-0@@ -981,6 +981,7 @@ struct vm_area_struct {unsigned long vm_pgoff;/* Offset (within vm_file) in
PAGE_SIZEinclude/linux/mm_types.h struct vm_area_struct {
Quoted 3 lines
include/linux/mm_types.h +1
-0units */ 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... */We definitely wouldn't add a field like this in the VMA type :) VMA fields are in very short supply and the data structure is very sensitive on size (it scales to a lot on many systems with many threads).
In general it feels like this is to pass things down to
remove_vma(), but that's an indicator really that you need to put it somehwere else.mm/vma.c static int __mmap_new_file_vma(struct mmap_state
Quoted 17 lines
include/linux/mm_types.h +0
-0void * vm_private_data;/* was vm_pte (shared mem) */ #ifdef CONFIG_SWAP mm/vma.c +1
-1@@ -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,
mm/vma.c static int __mmap_new_file_vma(struct mmap_state
Quoted 8 lines
mm/vma.c +1
-0int 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,
mm/vma.c static int __mmap_new_file_vma(struct mmap_state *map,
Quoted 7 lines
mm/vma.c +3
-1if (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
mm/vma.c static int __mmap_new_file_vma(struct mmap_state *map, · line 2508
Quoted 5 lines
mm/vma.c +5
-0+* the refcount, don't decrease it in this path. +*/ +if (!map->file_doesnt_need_get) +fput (vma->vm_file); +But this just means the file is unpinned in general and you really want its lifetime to be at least that of the mapping :>)
mm/vma.c static unsigned long __mmap_region(struct file
Quoted 4 lines
mm/vma.c +0
-0vma->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,
mm/vma.c static unsigned long __mmap_region(struct file *file, unsigned long addr, · line 2768
Quoted 5 lines
mm/vma.c +1
-1if (!error && have_mmap_prepare) error =call_mmap_prepare (&map, &desc);if (error) - goto abort_munmap; + goto abort_mmap_prepare;Yeah, as above, this is incorrect.
mm/vma.c static unsigned long __mmap_region(struct file
Quoted 4 lines
mm/vma.c +0
-0if (map.check_ksm_early)update_ksm_flags (&map);@@ -2795,15 +2803,9 @@ static unsigned long __mmap_region(struct file*file, unsigned long addr,
mm/vma.c static unsigned long __mmap_region(struct file *file, unsigned long addr,
Quoted 6 lines
mm/vma.c +0
-3unacct_error:if (map.charged)vm_unacct_memory (map.charged); -abort_munmap: -/* -* This indicates that .mmap_prepare has set a new file, differing from
Quoted 12 lines
mm/vma.c +2
-6-* 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; } --Quoted 2 lines
2.55.0
In general I can see you're a genuine kernel enthusiast so I don't want to dissuade you :)
This code is fiddly in general so it's _entirely_ understandable you had some misapprehensions about it.
Hopefully the feedback helps and you are able to continue implementing your feature, do let me know if you're able to do what you need with your driver (
mmap_prepareis in live development and I'm gradually adding more actions to suit actual driver usage).It's actually nice to see something human rather than LLM for some early contributions :)
Cheers, Lorenzo
Pedro Falcato
reviewerQuoted 74 lines · Liav Albani
Hi!
This is my first patch that I contribute myself to the Linux kernel. My apologize if there are issues, I tried to solve this issue in one evening and understand the call graph and what could go wrong.
I work recently on a project of mine called nfiop, and specifically on a kernel module called ufedm - https://github.com/nfiop/ufedm .
The project has a driver that allows a userspace implementation to intervene in the write and read callbacks of an MTD device by spawning an upper MTD device that sends the data back and forth to userspace, so userspace can implement its custom ECC engine and send back the results.
So far so good. I started working with kernel 6.18.8 on an Olimex A20 board and then moved to my x86 linux machine with a nandsim simulation. When I started my project, I implemented a memory backing based on a shmem file for the shared memory buffer, that is expected to be mmap'ed from my custom character device.
When I updated my host machine, the new kernel version was 7.x so I decided to update my module as well.
Many changes were comsetic, but the major change was to implement ammap_preparehook.I wrote something simple, taking notes on the
mmap_preparerules and the guide from kernel docs.Then I started to test that my character device is at least working fine, so I have my unit tests for ioctls and mmap, and the mmap unit-test failed after the second-run.
A kernel stack looked 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 ```
Quoted 5 lines
I started investigating this, and it became apparent that there's an issue with ref-counting of the
vm_file- I changed it in mymmap_preparehook, so it must be the cause, right?I also tried to artificially increment the ref-count of my shmem file
You need to take a ref when doing the mmap_prepare file setting. mmap (and VMAs) consumes one reference, thus need to pin the file. Thus why the ref count manipulation around this whole code...
So, in your case, something like:
get_file(dev->shm_mapping.filp);
in your proxy_chrdev_mmap_prepare looks correct. Otherwise, something else could sweep the file under the VMA. Note that this is true for every mmapped file, even something like /dev/zero (which does what you're doing, creates a shmem file and changes the VMA desc) implicitly donates the reference to mmap, which will later be unref'd (and the shmem file torn down) on munmap.
Let me know if I hugely misunderstood your problem :)
Signature · 1 line
Pedro
Lorenzo Stoakes (ARM)
maintainerQuoted 95 lines · Pedro Falcato
Quoted 74 lines · Liav Albani
Hi!
This is my first patch that I contribute myself to the Linux kernel. My apologize if there are issues, I tried to solve this issue in one evening and understand the call graph and what could go wrong.
I work recently on a project of mine called nfiop, and specifically on a kernel module called ufedm - https://github.com/nfiop/ufedm .
The project has a driver that allows a userspace implementation to intervene in the write and read callbacks of an MTD device by spawning an upper MTD device that sends the data back and forth to userspace, so userspace can implement its custom ECC engine and send back the results.
So far so good. I started working with kernel 6.18.8 on an Olimex A20 board and then moved to my x86 linux machine with a nandsim simulation. When I started my project, I implemented a memory backing based on a shmem file for the shared memory buffer, that is expected to be mmap'ed from my custom character device.
When I updated my host machine, the new kernel version was 7.x so I decided to update my module as well.
Many changes were comsetic, but the major change was to implement ammap_preparehook.I wrote something simple, taking notes on the
mmap_preparerules and the guide from kernel docs.Then I started to test that my character device is at least working fine, so I have my unit tests for ioctls and mmap, and the mmap unit-test failed after the second-run.
A kernel stack looked 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 ```Quoted 5 lines
I started investigating this, and it became apparent that there's an issue with ref-counting of the
vm_file- I changed it in mymmap_preparehook, so it must be the cause, right?I also tried to artificially increment the ref-count of my shmem file
You need to take a ref when doing the
mmap_preparefile setting. mmap (and VMAs) consumes one reference, thus need to pin the file. Thus why the ref count manipulation around this whole code...So, in your case, something like:
get_file(dev->shm_mapping.filp);in your
proxy_chrdev_mmap_preparelooks correct. Otherwise, something else could sweep the file under the VMA. Note that this is true for every mmapped file, even something like/dev/zero(which does what you're doing, creates a shmem file and changes the VMA desc) implicitly donates the reference to mmap, which will later be unref'd (and the shmem file torn down) on munmap.
Quoted 4 lines
Let me know if I hugely misunderstood your problem :)
--
Pedro
Yeah I agree. Thanks for the contribution and your interest in this part of the kernel :) but any file replacing an existing one must be pinned as it is then used by the mmap and the pin signifies that.
Perhaps a patch to the documentation could be useful here? In Documentation/filesystems/mmap_prepare.rst
If you found it confusing that that implies it's not clear, so something there saying 'if you replace the file, you MUST pin it via get_file() before assigning desc->vm_file'.
Thanks, Lorenzo
syzbot ci has tested the following series
syzbot ci has tested the following series
[v1] Fix an imbalanced file ref count in the mmap syscall
https://lore.kernel.org/all/[email protected]
* [PATCH] mm/vma: fix imbalanced file reference count, properly abort mmap_prepare
and found the following issue:
WARNING in tear_down_vmas
Full report is available here:
https://ci.syzbot.org/series/63d42f9f-f249-4678-8916-5464a15accd6
***
WARNING in tear_down_vmas
tree: mm-new URL: https://kernel.googlesource.com/pub/scm/linux/kernel/git/akpm/mm.git base: 3827fe994db98623a5a619a72f15f57bf8274deb arch: amd64 compiler: Debian clang version 22.1.6 (++20260514074242+fc4aad7b5db3-1~exp1~20260514074407.73), Debian LLD 22.1.6 config: https://ci.syzbot.org/builds/f57e989a-6273-4413-b35d-20dab5a41e92/config syz repro: https://ci.syzbot.org/findings/8f36751f-0976-460a-b5ab-de97c9074c0a/syz_repro
------------[ cut here ]------------ !vma_is_attached(vma) WARNING: ./include/linux/mmap_lock.h:435 at vma_assert_attached include/linux/mmap_lock.h:435 [inline], CPU#0: syz.2.19/5813 WARNING: ./include/linux/mmap_lock.h:435 at vma_mark_detached include/linux/mmap_lock.h:455 [inline], CPU#0: syz.2.19/5813 WARNING: ./include/linux/mmap_lock.h:435 at tear_down_vmas+0x381/0x520 mm/mmap.c:1261, CPU#0: syz.2.19/5813 Modules linked in: CPU: 0 UID: 0 PID: 5813 Comm: syz.2.19 Not tainted syzkaller #0 PREEMPT(full) Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 RIP: 0010:vma_assert_attached include/linux/mmap_lock.h:435 [inline] RIP: 0010:vma_mark_detached include/linux/mmap_lock.h:455 [inline] RIP: 0010:tear_down_vmas+0x381/0x520 mm/mmap.c:1261 Code: 48 89 df e8 01 46 16 00 48 8b 44 24 18 48 39 03 0f 87 e0 00 00 00 e8 ce a8 a8 ff ff 44 24 04 e9 9a fd ff ff e8 c0 a8 a8 ff 90 <0f> 0b 90 e9 2c ff ff ff e8 b2 a8 a8 ff 90 0f 0b 90 e9 94 fe ff ff RSP: 0018:ffffc90003cdf7e0 EFLAGS: 00010293 RAX: ffffffff821dbb90 RBX: ffff888115e14b40 RCX: ffff888171f9bb80 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffffc90003cdf920 R08: ffff888115e14b43 R09: 1ffff11022bc2968 R10: dffffc0000000000 R11: ffffed1022bc2969 R12: ffff888115e14a80 R13: 0000000000000001 R14: 0000000000000000 R15: dffffc0000000000 FS: 00007f15b88e36c0(0000) GS:ffff88818dc1c000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f9033eab095 CR3: 0000000173ace000 CR4: 00000000000006f0 Call Trace: <TASK> exit_mmap+0x4ca/0x9f0 mm/mmap.c:1320 __mmput+0x118/0x420 kernel/fork.c:1187 exit_mm+0x221/0x2d0 kernel/exit.c:610 do_exit+0x6cd/0x2360 kernel/exit.c:992 do_group_exit+0x22d/0x2f0 kernel/exit.c:1147 get_signal+0x121b/0x12c0 kernel/signal.c:3038 arch_do_signal_or_restart+0xbb/0x860 arch/x86/kernel/signal.c:337 __exit_to_user_mode_loop kernel/entry/common.c:66 [inline] exit_to_user_mode_loop+0x104/0x730 kernel/entry/common.c:101 __exit_to_user_mode_prepare include/linux/irq-entry-common.h:207 [inline] syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:230 [inline] syscall_exit_to_user_mode include/linux/entry-common.h:318 [inline] do_syscall_64+0x353/0x580 arch/x86/entry/syscall_64.c:100 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7f15b799ce59 Code: Unable to access opcode bytes at 0x7f15b799ce2f. RSP: 002b:00007f15b88e30e8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca RAX: fffffffffffffe00 RBX: 00007f15b7c15fa8 RCX: 00007f15b799ce59 RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f15b7c15fa8 RBP: 00007f15b7c15fa0 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 R13: 00007f15b7c16038 R14: 00007ffe1b61d9f0 R15: 00007ffe1b61dad8 </TASK>
***
If these findings have caused you to resend the series or submit a separate fix, please add the following tag to your commit message:
---
This report is generated by a bot. It may contain errors. syzbot ci engineers can be reached at [email protected].
To test a patch for this bug, please reply with #syz test (should be on a separate line).
The patch should be attached to the email.
Note: arguments like custom git repos and branches are not supported.