Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[David Hildenbrand]

On 02.04.25 14:31, Lorenzo Stoakes wrote:
> TL;DR is I agree with you :P I'm not sure where to put R-b tag given you sent a
> fix-patch, as this is obviously smatch/clang-broken as-is so feels wrong to put
> on main bit.

I'll respin! :)

> I guess I'll put on fix-patch and Andrew? Are you taking this? If so maybe from
> there you can propagate?

[...]

> If that handles it then fine, let's just init to 0.
>
> > So this should be working as expected? No need to add something on top that
> > makes it even more ugly in the caller.
>
> Yes, agreed, if this is already being handled in the one hideous place let's
> make it hideous there only.
>
> But maybe a comment...?

I can add that that function handles the need for actual untracking 
internally.

[...]

> > > > +
> > > > +	/*
> > > > +	 * Duplicate the PAT information for the dst VMA based on the src
> > > > +	 * VMA.
> > > > +	 */
> > > > +	if (get_pat_info(src_vma, &paddr, &pgprot))
> > > > +		return -EINVAL;
> > > > +	rc = reserve_pfn_range(paddr, vma_size, &pgprot, 1);
> > > > +	if (rc)
> > > > +		return rc;
> > >
> > > I mean it's a crazy nit, but we use ret elsewhere but rc here, maybe better
> > > to use ret in both places.
> > >
> > > But also feel free to ignore this.
> >
> > "int retval;" ? ;)
>
> Lol, 'rv'?
>
> Maybe let's leave it as is :P

I think "ret" is used in the file, so I'll use that.

> > > > +	/* Reservation for the destination VMA succeeded. */
> > > > +	vm_flags_set(dst_vma, VM_PAT);
> > > > +	*pfn = PHYS_PFN(paddr);
> > > >    	return 0;
> > > >    }
> > > >
> > > > +void untrack_pfn_copy(struct vm_area_struct *dst_vma, unsigned long pfn)
> > > > +{
> > > > +	untrack_pfn(dst_vma, pfn, dst_vma->vm_end - dst_vma->vm_start, true);
> > > > +	/*
> > > > +	 * Reservation was freed, any copied page tables will get cleaned
> > > > +	 * up later, but without getting PAT involved again.
> > > > +	 */
> > > > +}
> > > > +
> > > >    /*
> > > >     * prot is passed in as a parameter for the new mapping. If the vma has
> > > >     * a linear pfn mapping for the entire range, or no vma is provided,
> > > > @@ -1095,15 +1108,6 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
> > > >    	}
> > > >    }
> > > >
> > > > -/*
> > > > - * untrack_pfn_clear is called if the following situation fits:
> > > > - *
> > > > - * 1) while mremapping a pfnmap for a new region,  with the old vma after
> > > > - * its pfnmap page table has been removed.  The new vma has a new pfnmap
> > > > - * to the same pfn & cache type with VM_PAT set.
> > > > - * 2) while duplicating vm area, the new vma fails to copy the pgtable from
> > > > - * old vma.
> > > > - */
> > >
> > > This just wrong now?
> >
> > Note that I'm keeping the doc to a single place -- the stub in the header.
> > (below)
> >
> > Or can you elaborate what exactly is "wrong"?
>
> Ah ok maybe I just missed this. I was asking whether it was wrong, and this is
> why maybe you are removing (perhaps, not very clearly :)

Ah, sorry. Yes, it's just deduplicated to be adjusted in the other copy :)

[...]

> > > Can I say as an aside that I hate this kind of hook? Like quite a lot?
> > >
> > > I mean I've been looking at mremap() of anon mappings as you know obv. but
> > > the thought of PFN mapping mremap()ing is kind of also a bit ugh.
> >
> > I absolutely hate all of that, but I'll have to leave any cleanups to people
> > with more spare time ;)
>
> Lol well... maybe at some point I will find some for this... when things get
> ugly enough I find that I make the time in the end ;)

We're kind-of attaching metadata to a VMA, that is not directly linked 
to the VMA. And the duplication of a VMA cannot handle that, so we defer 
copying of that metadata. Hm ...

--
Cheers,

David / dhildenb