On Tue, Jul 15, 2025 at 10:05:49AM -0700, Andrii Nakryiko wrote:
On Tue, Jul 15, 2025 at 3:31 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Tue, Jul 15, 2025 at 12:23:31PM +0200, Vlastimil Babka wrote:
On 7/15/25 11:52, David Hildenbrand wrote:
On 15.07.25 11:40, Lorenzo Stoakes wrote:
On Tue, Jul 15, 2025 at 10:16:41AM +0200, Vlastimil Babka wrote:
> Andrew, could you please remove this patchset from mm-unstable for now > until I fix the issue and re-post the new version?
Andrew can you do that please? We keep getting new syzbot reports.
I also pinged up top :P just to be extra specially clear...
> The error I got after these fixes is:
I suspect the root cause is the ioctls are not serialized against each other (probably not even against read()) and yet we treat m->private as safe to work on. Now we have various fields that are dangerous to race on - for example locked_vma and iter races would explain a lot of this.
I suspect as long as we used purely seq_file workflow, it did the right thing for us wrt serialization, but the ioctl addition violates that. We should rather recheck even the code before this series, if dangerous ioctl vs read() races are possible. And the ioctl implementation should be refactored to use an own per-ioctl-call private context, not the seq_file's per-file-open context.
Entirely agree with this analysis. I had a look at most recent report, see:
https://lore.kernel.org/linux-mm/f13cda37-06a0-4281-87d1-042678a38a6b@lucife...
AFAICT we either have to lock around the ioctl or find a new way of storing per-ioctl state.
We'd probably need to separate out the procmap query stuff to do that though. Probably.
When I skimmed that series the first time, I was wondering "why are we even caring about PROCMAP_QUERY that in the context of this patch series".
Maybe that helps :)
Yeah seems like before patch 8/8 the ioctl handling, specifically do_procmap_query() only looks at priv->mm and nothing else so it should be safe as that's a stable value.
So it should be also enough to drop the last patch from mm for now, not whole series.
Yeah to save the mothership we can ditch the landing craft :P
Maybe worth doing that, and figure out in a follow up how to fix this.
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