On Tue, Jul 15, 2025 at 10:21 AM Lorenzo Stoakes
<[email protected]> 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.
> 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!
