On Tue, Jul 15, 2025 at 1:18 PM Suren Baghdasaryan surenb@google.com wrote:
On Tue, Jul 15, 2025 at 10:29 AM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Tue, Jul 15, 2025 at 10:21 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
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.
Exactly, and I guess I was just being incomplete with just listing two of the fields that Suren make use of in PROCMAP_QUERY. See below.
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);I guess it's a matter of preference, but I'd actually just pass seq->priv->mm and arg into do_procmap_query(), which will make it super obvious that priv is not used or mutated, and all the new stuff that Suren needs for lockless VMA iteration, including iter (not sure PROCMAP_QUERY needs last_pos, tbh), I'd just put into this new struct, which do_procmap_query() can keep private to itself.
Ultimately, I think we are on the same page, it's just a matter of structuring code and types.
That sounds cleaner to me too. I'll post a new version of my patchset today without the last patch to keep PROCMAP_QUERY changes separate, and then a follow-up patch that does this refactoring and changes PROCMAP_QUERY to use per-vma locks.
Thanks folks! It's good to be back from vacation with the problem already figured out for you :)
Yes, Vlastimil was correct. Once I refactored the last patch so that do_procmap_query() does not use seq->private at all, the reproducer stopped failing. I'll post the patchset without the last patch shortly and once Andrew takes it into mm-unstable, will post the last patch as a follow-up. Thanks, Suren.
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!