mm/maps: move kmalloc() call location in do_procmap_query() out of RCU critical section

[PATCH next] mm/maps: move kmalloc() call location in do_procmap_query() out of RCU critical section

Posted by Jeongjun Park 3 months, 1 week ago

In do_procmap_query(), we are allocating name_buf as much as name_buf_sz
with kmalloc().

However, due to the previous commit eff061546ca5 
("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks"),
the location of kmalloc() is located inside the RCU critical section.

This causes might_sleep_if() to be called inside the RCU critical section,
so we need to move the call location of kmalloc() outside the RCU critical
section to prevent this.

Reported-by: syzbot+6246a83e7bd9f8a3e239@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6246a83e7bd9f8a3e239
Fixes: eff061546ca5 ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 fs/proc/task_mmu.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f3659046efb7..42b0224c6ac9 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -595,6 +595,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
 	char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
 	__u64 usize;
 	int err;
+	size_t name_buf_sz;
 
 	if (copy_from_user(&usize, (void __user *)uarg, sizeof(usize)))
 		return -EFAULT;
@@ -621,12 +622,18 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
 	if (!mm || !mmget_not_zero(mm))
 		return -ESRCH;
 
-	err = query_vma_setup(priv);
-	if (err) {
+	name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
+
+	name_buf = kmalloc(name_buf_sz, GFP_KERNEL);
+	if (!name_buf) {
 		mmput(mm);
-		return err;
+		return -ENOMEM;
 	}
 
+	err = query_vma_setup(priv);
+	if (err)
+		goto fail_vma_setup;
+
 	vma = query_matching_vma(priv, karg.query_addr, karg.query_flags);
 	if (IS_ERR(vma)) {
 		err = PTR_ERR(vma);
@@ -679,20 +686,12 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
 	}
 
 	if (karg.vma_name_size) {
-		size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
 		const struct path *path;
 		const char *name_fmt;
 		size_t name_sz = 0;
 
 		get_vma_name(vma, &path, &name, &name_fmt);
 
-		if (path || name_fmt || name) {
-			name_buf = kmalloc(name_buf_sz, GFP_KERNEL);
-			if (!name_buf) {
-				err = -ENOMEM;
-				goto out;
-			}
-		}
 		if (path) {
 			name = d_path(path, name_buf, name_buf_sz);
 			if (IS_ERR(name)) {
@@ -733,6 +732,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
 
 out:
 	query_vma_teardown(priv);
+fail_vma_setup:
 	mmput(mm);
 	kfree(name_buf);
 	return err;
--

Re: [PATCH next] mm/maps: move kmalloc() call location in do_procmap_query() out of RCU critical section

Posted by David Hildenbrand 3 months, 1 week ago

On 02.07.25 15:53, Jeongjun Park wrote:
> In do_procmap_query(), we are allocating name_buf as much as name_buf_sz
> with kmalloc().
> 
> However, due to the previous commit eff061546ca5
> ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks"),
> the location of kmalloc() is located inside the RCU critical section.
> 
> This causes might_sleep_if() to be called inside the RCU critical section,
> so we need to move the call location of kmalloc() outside the RCU critical
> section to prevent this.
> 
> Reported-by: syzbot+6246a83e7bd9f8a3e239@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6246a83e7bd9f8a3e239
> Fixes: eff061546ca5 ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks")

That commit is not upstream yet (and the commit id is not stable), so it 
should be squashed into the problematic commit.

As a side note: the patch subject of this and the original patch should 
start with "fs/proc/task_mmu", not "mm/maps".

-- 
Cheers,

David / dhildenb

Re: [PATCH next] mm/maps: move kmalloc() call location in do_procmap_query() out of RCU critical section

Posted by Suren Baghdasaryan 3 months, 1 week ago

On Wed, Jul 2, 2025 at 7:44 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 02.07.25 15:53, Jeongjun Park wrote:
> > In do_procmap_query(), we are allocating name_buf as much as name_buf_sz
> > with kmalloc().
> >
> > However, due to the previous commit eff061546ca5
> > ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks"),
> > the location of kmalloc() is located inside the RCU critical section.
> >
> > This causes might_sleep_if() to be called inside the RCU critical section,
> > so we need to move the call location of kmalloc() outside the RCU critical
> > section to prevent this.
> >
> > Reported-by: syzbot+6246a83e7bd9f8a3e239@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=6246a83e7bd9f8a3e239
> > Fixes: eff061546ca5 ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks")
>
> That commit is not upstream yet (and the commit id is not stable), so it
> should be squashed into the problematic commit.
>
> As a side note: the patch subject of this and the original patch should
> start with "fs/proc/task_mmu", not "mm/maps".

Thanks for the fix Jeongjun and thanks for the note David.
I'm preparing the next version of my patchset and there is a much
simpler fix for this issue which I'll implement there. Planning to
post it tomorrow.
Thanks,
Suren.

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH next] mm/maps: move kmalloc() call location in do_procmap_query() out of RCU critical section

Posted by Suren Baghdasaryan 3 months ago

On Wed, Jul 2, 2025 at 4:34 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 2, 2025 at 7:44 AM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 02.07.25 15:53, Jeongjun Park wrote:
> > > In do_procmap_query(), we are allocating name_buf as much as name_buf_sz
> > > with kmalloc().
> > >
> > > However, due to the previous commit eff061546ca5
> > > ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks"),
> > > the location of kmalloc() is located inside the RCU critical section.
> > >
> > > This causes might_sleep_if() to be called inside the RCU critical section,
> > > so we need to move the call location of kmalloc() outside the RCU critical
> > > section to prevent this.
> > >
> > > Reported-by: syzbot+6246a83e7bd9f8a3e239@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=6246a83e7bd9f8a3e239
> > > Fixes: eff061546ca5 ("mm/maps: execute PROCMAP_QUERY ioctl under per-vma locks")
> >
> > That commit is not upstream yet (and the commit id is not stable), so it
> > should be squashed into the problematic commit.
> >
> > As a side note: the patch subject of this and the original patch should
> > start with "fs/proc/task_mmu", not "mm/maps".
>
> Thanks for the fix Jeongjun and thanks for the note David.
> I'm preparing the next version of my patchset and there is a much
> simpler fix for this issue which I'll implement there. Planning to
> post it tomorrow.

v6 of my patchset is posted at
https://lore.kernel.org/all/20250704060727.724817-1-surenb@google.com/
I reworked the last patch to address this issue by narrowing down the
rcu read section to query_vma_find_by_addr() only. That should fix the
original issue.

> Thanks,
> Suren.
>
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >