linux-kernel - Re: [PATCH v6 7/8] fs/proc/task_mmu: read proc/pid/maps under per-vma lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d878566-f445-4fc2-9d04-eb8b38024c9b@lucifer.local>
Date: Tue, 15 Jul 2025 18:20:47 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Vlastimil Babka <vbabka@...e.cz>, David Hildenbrand <david@...hat.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>, akpm@...ux-foundation.org,
        peterx@...hat.com, jannh@...gle.com, hannes@...xchg.org,
        mhocko@...nel.org, paulmck@...nel.org, shuah@...nel.org,
        adobriyan@...il.com, brauner@...nel.org, josef@...icpanda.com,
        yebin10@...wei.com, linux@...ssschuh.net, willy@...radead.org,
        osalvador@...e.de, andrii@...nel.org, ryan.roberts@....com,
        christophe.leroy@...roup.eu, tjmercier@...gle.com,
        kaleshsingh@...gle.com, aha310510@...il.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v6 7/8] fs/proc/task_mmu: read proc/pid/maps under
 per-vma lock

On Tue, Jul 15, 2025 at 06:10:16PM +0100, Lorenzo Stoakes wrote:
> > For PROCMAP_QUERY, we need priv->mm, but the newly added locked_vma
> > and locked_vma don't need to be persisted between ioctl calls. So we
> > can just add those two fields into a small struct, and for seq_file
> > case have it in priv, but for PROCMAP_QUERY just have it on the stack.
> > The code can be written to accept this struct to maintain the state,
> > which for PROCMAP_QUERY ioctl will be very short-lived on the stack
> > one.
> >
> > Would that work?
>
> Yeah that's a great idea actually, the stack would obviously give us the
> per-query invocation thing. Nice!
>
> I am kicking myself because I jokingly suggested (off-list) that a helper
> struct would be the answer to everything (I do love them) and of
> course... here we are :P

Hm but actually we'd have to invert things I think, what I mean is - since
these fields can be updated at any time by racing threads, we can't have
_anything_ in the priv struct that is mutable.

So instead we should do something like:

struct proc_maps_state {
	const struct proc_maps_private *priv;
	bool mmap_locked;
	struct vm_area_struct *locked_vma;
	struct vma_iterator iter;
	loff_t last_pos;
};

static long procfs_procmap_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	struct seq_file *seq = file->private_data;
	struct proc_maps_private *priv = seq->private;
	struct proc_maps_state state = {
		.priv = priv,
	};

	switch (cmd) {
	case PROCMAP_QUERY:
		return do_procmap_query(state, (void __user *)arg);
	default:
		return -ENOIOCTLCMD;
	}
}

And then we have a stack-based thing with the bits that change and a
read-only pointer to the bits that must remain static. And we can enforce
that with const...

We'd have to move the VMI and last_pos out too to make it const.

Anyway the general idea should work!