Re: [PATCH v1 1/2] mm: Clear uffd-wp PTE/PMD state on mremap()

[Ryan Roberts]

On 15/01/2025 17:30, Lorenzo Stoakes wrote:
> On Wed, Jan 15, 2025 at 12:21:15PM -0500, Peter Xu wrote:
>> On Wed, Jan 15, 2025 at 04:58:06PM +0000, Ryan Roberts wrote:
>>> Hi Peter, David,
>>
>> Hey, Ryan,
>>
>>>
>>> On 07/01/2025 14:47, Ryan Roberts wrote:
>>>> When mremap()ing a memory region previously registered with userfaultfd
>>>> as write-protected but without UFFD_FEATURE_EVENT_REMAP, an
>>>> inconsistency in flag clearing leads to a mismatch between the vma flags
>>>> (which have uffd-wp cleared) and the pte/pmd flags (which do not have
>>>> uffd-wp cleared). This mismatch causes a subsequent mprotect(PROT_WRITE)
>>>> to trigger a warning in page_table_check_pte_flags() due to setting the
>>>> pte to writable while uffd-wp is still set.
>>>>
>>>> Fix this by always explicitly clearing the uffd-wp pte/pmd flags on any
>>>> such mremap() so that the values are consistent with the existing
>>>> clearing of VM_UFFD_WP. Be careful to clear the logical flag regardless
>>>> of its physical form; a PTE bit, a swap PTE bit, or a PTE marker. Cover
>>>> PTE, huge PMD and hugetlb paths.
>>>
>>> I just noticed that Andrew sent this to Linus and it's now in his tree; I'm
>>> suddenly very nervous that it doesn't have any acks. I don't suppose you would
>>> be able to do a quick review to calm the nerves??
>>
>> Heh, I fully trusted you, and I appreciated your help too. I'll need to run
>> for 1-2 hours, but I'll read it this afternoon.

Thanks - appreciate it! I was just conscious that in the original thread there
was some disagreement between you and David about whether we should clear the
PTE state or set the VMA state. I think we converged on the former (and that's
what's implemented) but would be good to get an explicit ack.

>>
>> Side note: no review is as good as tests on reliability POV if that was the
>> concern, but I'll try my best.
> 
> Things go all inception though when part of the review _are_ the tests ;)
> Though of course there are also all existing uffd tests and the bots that
> add a bit of weight.
> 
> This isn't really my area so will defer to Peter on the review side.
> 
> I sort of favour putting hotfixes in quick, but this one has gone in
> quicker than some reviewed hotfixes which we left in unstable... however
> towards the end of a cycle I think Andrew is stuck between a rock and a
> hard place in deciding how to handle these.
> 
> So I'm guessing the heuristic is 'allow to simmer in unstable if time
> permits in cycle', if known 'good egg' + no objection + towards end of
> cycle + hotfix - send.
> 
> I do wonder whether we should require review on hotfixes generally. But
> then of course that creates rock + hard place decision for Andrew as to
> whether it gets deferred to the next cycle + stable backports...

I have no issue with the process in general. I agree it's better to go quickly -
that's the best way to find the bugs. I'm really just highlighting that in this
case, I don't feel sufficiently expert with the subject matter and would
appreciate another set of eyes.

Thanks,
Ryan

> 
> Maybe one to discuss at LSF?
> 
>>
>> Thanks,
>>
>> --
>> Peter Xu
>>