Re: [PATCH 1/3] ima: Remove inode lock
From: Kirill A. Shutemov <hidden>
Date: 2024-10-18 11:05:41
Also in:
bpf, linux-fsdevel, linux-integrity, linux-mm, lkml
Subsystem:
memory management, memory mapping, the rest · Maintainers:
Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Linus Torvalds
On Fri, Oct 18, 2024 at 12:00:22PM +0100, Lorenzo Stoakes wrote:
+ Liam, Jann On Fri, Oct 18, 2024 at 01:49:06PM +0300, Kirill A. Shutemov wrote:quoted
On Fri, Oct 18, 2024 at 11:24:06AM +0200, Roberto Sassu wrote:quoted
Probably it is hard, @Kirill would there be any way to safely move security_mmap_file() out of the mmap_lock lock?What about something like this (untested):diff --git a/mm/mmap.c b/mm/mmap.c index dd4b35a25aeb..03473e77d356 100644 --- a/mm/mmap.c +++ b/mm/mmap.c@@ -1646,6 +1646,26 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (pgoff + (size >> PAGE_SHIFT) < pgoff) return ret; + if (mmap_read_lock_killable(mm)) + return -EINTR; + + vma = vma_lookup(mm, start); + + if (!vma || !(vma->vm_flags & VM_SHARED)) { + mmap_read_unlock(mm); + return -EINVAL; + } + + file = get_file(vma->vm_file); + + mmap_read_unlock(mm); + + ret = security_mmap_file(vma->vm_file, prot, flags);Accessing VMA fields without any kind of lock is... very much not advised. I'm guessing you meant to say: ret = security_mmap_file(file, prot, flags); Here? :)
Sure. My bad. Patch with all fixups:
diff --git a/mm/mmap.c b/mm/mmap.c
index dd4b35a25aeb..541787d526b6 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c@@ -1646,14 +1646,41 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (pgoff + (size >> PAGE_SHIFT) < pgoff) return ret; - if (mmap_write_lock_killable(mm)) + if (mmap_read_lock_killable(mm)) return -EINTR; vma = vma_lookup(mm, start); + if (!vma || !(vma->vm_flags & VM_SHARED)) { + mmap_read_unlock(mm); + return -EINVAL; + } + + file = get_file(vma->vm_file); + + mmap_read_unlock(mm); + + ret = security_mmap_file(file, prot, flags); + if (ret) { + fput(file); + return ret; + } + + ret = -EINVAL; + + if (mmap_write_lock_killable(mm)) { + fput(file); + return -EINTR; + } + + vma = vma_lookup(mm, start); + if (!vma || !(vma->vm_flags & VM_SHARED)) goto out; + if (vma->vm_file != file) + goto out; + if (start + size > vma->vm_end) { VMA_ITERATOR(vmi, mm, vma->vm_end); struct vm_area_struct *next, *prev = vma;
@@ -1688,16 +1715,11 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (vma->vm_flags & VM_LOCKED) flags |= MAP_LOCKED; - file = get_file(vma->vm_file); - ret = security_mmap_file(vma->vm_file, prot, flags); - if (ret) - goto out_fput; ret = do_mmap(vma->vm_file, start, size, prot, flags, 0, pgoff, &populate, NULL); -out_fput: - fput(file); out: mmap_write_unlock(mm); + fput(file); if (populate) mm_populate(ret, populate); if (!IS_ERR_VALUE(ret))
--
Kiryl Shutsemau / Kirill A. Shutemov