Re: [PATCH v2 17/39] mm: Fixup places that call pte_mkwrite() directly

[Edgecombe, Rick P]

On Mon, 2022-10-03 at 18:56 -0700, Nadav Amit wrote:
> Hopefully I will not waste your time again… If it has been discussed
> in the
> last 26 iterations, just tell me and ignore.
> 
> On Sep 29, 2022, at 3:29 PM, Rick Edgecombe <
> rick.p.edgecombe@xxxxxxxxx> wrote:
> 
> > --- a/mm/migrate_device.c
> > +++ b/mm/migrate_device.c
> > @@ -606,8 +606,7 @@ static void migrate_vma_insert_page(struct
> > migrate_vma *migrate,
> > 			goto abort;
> > 		}
> > 		entry = mk_pte(page, vma->vm_page_prot);
> > -		if (vma->vm_flags & VM_WRITE)
> > -			entry = pte_mkwrite(pte_mkdirty(entry));
> > +		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> > 	}
> 
> This is not exactly the same logic. You might dirty read-only pages
> since
> you call pte_mkdirty() unconditionally. It has been known not to be
> very
> robust (e.g., dirty-COW and friends). Perhaps it is not dangerous
> following
> some recent enhancements, but why do you want to take the risk?

Yea those changes let me drop a patch. But, it's a good point.

> 
> Instead, although it might seem redundant, the compiler will
> hopefully would
> make it efficient:
> 
> 		if (vma->vm_flags & VM_WRITE) {
> 			entry = pte_mkdirty(entry);
> 			entry = maybe_mkwrite(entry, vma);
> 		}
> 

Thanks Nadav. I think you're right, it should have the open coded logic
here and in the do_anonymous_page() chunk that got moved to the
previous patch on accident.