On Tue, 6 Dec 2022, David Hildenbrand wrote: ... > > We never had to remove write permissions so far from the vma->vm_page_prot > default. We always only added permissions. > > > Now, uffd-wp on shmem as of now violates these semantics. vma->vm_page_prot > cannot always be used as a safe default, because we might have to wrprotect > individual PTEs. Note that for uffd-wp on anonymous memory this was not an > issue, because we default to !write in vma->vm_page_prot. > > > The two possible ways to approach this for uffd-wp on shmem are: > > (1) Obey existing vma->vm_page_prot semantics. Default to !write and > optimize the relevant cases to *add* the write bit. This is > essentially what this patch does, minus > can_change_pte_writable() optimizations on relevant code paths where > we'd want to maintain the write bit. For example, when removing > uffd-wp protection we might want to restore the write bit directly. > > (2) Default to write permissions and check each and every code location > where we remap for uffd-wp ptes, to manuall wrprotect -- *remove* > the write bit. Alternatively, as you said, always wrprotect when > setting the PTE bit, which might work as well. > > > My claim is that (1) is less error prone, because in the worst case we forget > to optimize one code location -- instead to resulting in a BUG if we forget to > wrprotect (what we have now). But I am not going to fight for it, because I > can see that (2) can be made to work as well, as you outline in your patch. > > You seem to have decided on (2). Fair enough, you know uffd-wp best. We just > have to fix it properly and make the logic consistent whenever we remap a > page. > ... > > But I'm not going to argue about whats valid and whats not as long as it's > documented ;). I primarily wanted to showcase that the same logic based on > vma->vm_page_prot is used elsewhere, and that migration PTE restoration is not > particularly special. I have not been following the uffd-wp work, but I believe that David's painstaking and excellent account of vm_page_prot is correct. Peter, please I beg you to follow his advice and go for (1) for uffd-wp. I do not share David's faith in "documented": documented or not, depart from safe convention and you will be adding (at least the opportunity for) serious bugs. Hugh
