Re: [PATCH STABLE 5.10] mm/memory: add non-anonymous page check in the copy_present_page()
Reinstating Cc stable, which I removed just before the discussion settled.
On Thu, 27 Oct 2022, Peter Xu wrote:
...
After a re-read and 2nd thought, I think David has a valid point in that we shouldn't have special handling of !anon pages on CoW during fork(), because that seems to be against the fundamental concept of fork().
So now I think I agree the !Anon original check does look a bit cleaner, and also make fork() behavior matching with the old/new kernels, irrelevant of the pin mess.
Thanks Peter. So Yuanzheng's patch for 5.10 is exactly right.
Sorry for leading everyone astray: my mistake was to suppose that its !PageAnon check was simply to avoid the later BUG_ON(!anon_vma): whereas David and Peter now agree that it actually corrects the semantics for fork() on file pages.
I lift my hold on Yuanzheng's patch: nobody actually said "Acked-by", but I think the discussion and resolution have given better than that. (No 3rd thoughts please!)
Hugh
On 28.10.22 03:32, Hugh Dickins wrote:
Reinstating Cc stable, which I removed just before the discussion settled.
Sorry for not reading the full thread before and considering Peters mail; I had to take short cuts :)
On Thu, 27 Oct 2022, Peter Xu wrote:
...
After a re-read and 2nd thought, I think David has a valid point in that we shouldn't have special handling of !anon pages on CoW during fork(), because that seems to be against the fundamental concept of fork().
So now I think I agree the !Anon original check does look a bit cleaner, and also make fork() behavior matching with the old/new kernels, irrelevant of the pin mess.
Thanks Peter. So Yuanzheng's patch for 5.10 is exactly right.
Sorry for leading everyone astray: my mistake was to suppose that its !PageAnon check was simply to avoid the later BUG_ON(!anon_vma): whereas David and Peter now agree that it actually corrects the semantics for fork() on file pages.
I lift my hold on Yuanzheng's patch: nobody actually said "Acked-by", but I think the discussion and resolution have given better than that. (No 3rd thoughts please!)
Unless someone tells me why I am obviously wrong
Acked-by: David Hildenbrand david@redhat.com
On Thu, Oct 27, 2022 at 06:32:01PM -0700, Hugh Dickins wrote:
Sorry for leading everyone astray: my mistake was to suppose that its !PageAnon check was simply to avoid the later BUG_ON(!anon_vma): whereas David and Peter now agree that it actually corrects the semantics for fork() on file pages.
Thanks for raising this from the start, Hugh. It's definitely worthwhile to discuss this topic which is not obvious at all at least to me, and merge even the same patch would be different before/after such a discussion, since we're clearer on the side effects.
I lift my hold on Yuanzheng's patch: nobody actually said "Acked-by", but I think the discussion and resolution have given better than that. (No 3rd thoughts please!)
I've acked directly on v2, note that after this discussion IMHO the comment of !Anon check can be slightly improved (e.g. add some more information on why we decided to not copy the page even if anon_vma existed), but I don't want to be harsh on any stable backports that helps resolving problems already in correct ways.
Thanks,
linux-stable-mirror@lists.linaro.org
-
David Hildenbrand -
Hugh Dickins -
Peter Xu