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

[David Hildenbrand <david@redhat.com>]

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