Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9cb84b60-6b51-3117-27cb-a29b3bd9e741@mbosol.com>
Date:   Fri, 14 Apr 2023 12:45:29 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 1/6] mm/hugetlb: Fix uffd-wp during fork()
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Cc:     Axel Rasmussen <axelrasmussen@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        linux-stable <stable@vger.kernel.org>
References: <20230413231120.544685-1-peterx@redhat.com>
 <20230413231120.544685-2-peterx@redhat.com>
From:   =?UTF-8?Q?Mika_Penttil=c3=a4?= <mika.penttila@mbosol.com>
In-Reply-To: <20230413231120.544685-2-peterx@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d3JHVXhEWFNSN1lqS0pmZjNSY0loSDVaZ3VNUVBxT3ZYSFFWbXhway92TnJu?=
 =?utf-8?B?eEMrV2Jid2RNdUFwMzNqdkFMREJiTDR4VGtjSGdBVHZQeEE1OTBvcldMdW9h?=
 =?utf-8?B?WWtndm1hUXk0ck14eXNqNkZVZ2NTL3R3QklyRlVQMFZDbWZrdndOZk0vQmlz?=
 =?utf-8?B?MTIvWTh3TFdVeUQ3cGZYUUVKL29zQmp5bUgwa2ZjVHVtYnlXMnJFWVcxRnV6?=
 =?utf-8?B?L25SRzdBZFlOT3JKSTM3c2tnTGpNS0VpWDlpTHlnNEZZR0luR2xheXJ5SnlF?=
 =?utf-8?B?TFBXdlpWTEQrYzlIR3ZUb3JCcWFtcGEvclBQTEpycjlJZkJQVVJQK3NHejFM?=
 =?utf-8?B?ZHRSSGNJUmpXSnR3QWdPN1VCWjFVdkdXeDVHRHJHalhUWWh1ZHlydmR4Um5N?=
 =?utf-8?B?WDN1ZlRMNStreHl1TklLNDlOSjdVTFR5NUFZZlZubWRHeFUzM1N5SEVNVGlk?=
 =?utf-8?B?WEVkb2Z0cnFJQnp3TXNCMDRXY3NkRW12M2NtM2gzNjNWNDdNYVhCTzJ5ZDlU?=
 =?utf-8?B?RUNRVVg2ck9ncjlEa1dkK1lOb3FvdFVUaEllQU5zelVwTFVmQ1hZaUc4MHU1?=
 =?utf-8?B?MHRKWVBpclR0Y1hTYWRzc1QvNG9SanZjdGhTWmtwQTk5WEJoVFo0SXI5ekti?=
 =?utf-8?B?YTQzUUo5V3BmUGlDWXlRSit3ZnBYN0FyVCtoMTE4eGYzdG1pNzRBQkFCTkVs?=
 =?utf-8?B?aVc0M0t3K3BmdUsvT24wM3FVMnhjM29LczB0ZUZJemZpa2NRUjVwOTNqdzNT?=
 =?utf-8?B?cHhSZzlKMW1TcDRTVWhERUlPcjVraHFHc2hMb3huV2pWaWkvZEx5L0RYbWF1?=
 =?utf-8?B?Z3hSODVScUtYMUJWWlhJeW5Pb2FqQ1RCeVkrNnhtQlE4U05HdWR1UDVxcHNl?=
 =?utf-8?B?eGlEeTJQMkdHcVpzZExkZ0I5ZWJWSUVTV2tZL2xSV1pzYjd3WUJTY2g0R0Rl?=
 =?utf-8?B?WVlOZWpESTExbXVySEc4a0JKN2ZjNkV1bVBsUzhvakJCRHQ2YkI0cTBic2lM?=
 =?utf-8?B?SnVTMjdMODMvTFZmNEMvbGxoMklOTDZtR0I5RnU1dElWM2o4TTBsMktSUG9H?=
 =?utf-8?B?MVlkS045Zm1xSUwxMUx4NHY2U2RsY0tXQXlLYlJ4RHVnTFB3cXVrTCtKYk9G?=
 =?utf-8?B?dEUvMy9oZG0yTU83bnR2d1o0QUc1aXI5Q1hLMENvVjRUUStKN1dJcTVsaEZR?=
 =?utf-8?B?b1JFSGNGT1BGcVcyMjM3cHNsVnlSVXUweUVaZUNwcjllbFpzNG9pYXI2Qko0?=
 =?utf-8?B?QXhzcllnUFk5dmJHTysyWUZqVC95OWJBOHRDbTdrSzZSVWt0VmVUQjZkdWNv?=
 =?utf-8?B?NkpHNHZkbVpQeWp2UXBLTTBFUDZXay9KR0FCZi9Ha0wxa0Y3VFZmd0JEQ0Vz?=
 =?utf-8?B?bEZxYTJ0NU1VSnMzb0RwR1o2N085Ty9YeUlsb0hOd3RDTDg4SmtMeWR3bWtq?=
 =?utf-8?B?WEtkejhmODlkSi9OS01qTy9takJIbFp6dVFRa2ZzTTJ2QTI5eGtnM0d2T0pU?=
 =?utf-8?B?S05YZmFIWmREb0Y1V0JHbW5MNE5HaExNcGR6ejAxZldjMzk0Tm1MQkZrd2lC?=
 =?utf-8?B?aG1zMGRLKzFEQThRTzlJSEVCYXQ1RTRYVnJEWFArL2lhSy9wS1NFN1NqNkdu?=
 =?utf-8?B?TDZXUDY0U3VadUJLRU1UY0g4NXJLWjlNTGJEbUp3Q2FSZ0tQSkJiWlpQeXBm?=
 =?utf-8?B?U0huSURRUDNpOW5NQndXM3h1d0dwUW55RDEwR3ZXaG1SYVUvdDdKQlR0aGNw?=
 =?utf-8?B?bm1UaTlnUExvalFFQzdWclVSVmtOVkk5NW9YeTFtOGV0WXZZRm1nS2lLV21w?=
 =?utf-8?B?UWtURUFlNDR1YmFJWW9mN0k5L0svLytCU2VwUVExTzNHWEpSWFRjaGI3c05r?=
 =?utf-8?B?NElnRzBibDlSaWszdkVTZHd1enMvdTczWUJUR0U5S3QrcTFja0xrR1E2OVRZ?=
 =?utf-8?B?VjQ1T3I5SEwvQlZkb3kzMSt6U3BzQ1dZVEJwSnNYRjlnUXZrWGsyeGE1TkUr?=
 =?utf-8?B?QUhPbzVqREpZWWdtTEhKbU5rN21NN1hjcUQzcFB6dHRLMXJiWnhrZ281TkNk?=
 =?utf-8?B?b3hEV3V4bmswd2pHMzE2TCtDUGM4U3pDcjVVSk5UTmIrelhzMmNicG12QnFh?=
 =?utf-8?B?MWh5QkgyS2VsYVhBY0tWaDczdGdiTmt5TWpPZUZaNDk3L2pqRm5xWHFhYlNa?=
 =?utf-8?B?dmc9PQ==?=
X-OriginatorOrg: mbosol.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ee5a96f-cf62-4258-c182-08db3ccd040b
X-MS-Exchange-CrossTenant-AuthSource: DB8P192MB0533.EURP192.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2023 09:45:43.9295
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 571b6760-44e0-4aae-b783-84984ac780c0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ll8IhfkxMqjbz1Z3NfCYF98F36+7MaD/qjkrCDFLuikaWEDPRSLdoqM7+UJPEUVRP9V3iFCmQCVE03qPzdj9nrWlgmtU5GaHj33y7+HDYZA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3P192MB2108
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 14.4.2023 2.11, Peter Xu wrote:
> There're a bunch of things that were wrong:
> 
>    - Reading uffd-wp bit from a swap entry should use pte_swp_uffd_wp()
>      rather than huge_pte_uffd_wp().
> 
>    - When copying over a pte, we should drop uffd-wp bit when
>      !EVENT_FORK (aka, when !userfaultfd_wp(dst_vma)).
> 
>    - When doing early CoW for private hugetlb (e.g. when the parent page was
>      pinned), uffd-wp bit should be properly carried over if necessary.
> 
> No bug reported probably because most people do not even care about these
> corner cases, but they are still bugs and can be exposed by the recent unit
> tests introduced, so fix all of them in one shot.
> 
> Cc: linux-stable <stable@vger.kernel.org>
> Fixes: bc70fbf269fd ("mm/hugetlb: handle uffd-wp during fork()")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   mm/hugetlb.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f16b25b1a6b9..7320e64aacc6 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4953,11 +4953,15 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
>   
>   static void
>   hugetlb_install_folio(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
> -		     struct folio *new_folio)
> +		      struct folio *new_folio, pte_t old)
>   {
> +	pte_t newpte = make_huge_pte(vma, &new_folio->page, 1);
> +
>   	__folio_mark_uptodate(new_folio);
>   	hugepage_add_new_anon_rmap(new_folio, vma, addr);
> -	set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, &new_folio->page, 1));
> +	if (userfaultfd_wp(vma) && huge_pte_uffd_wp(old))
> +		newpte = huge_pte_mkuffd_wp(newpte);
> +	set_huge_pte_at(vma->vm_mm, addr, ptep, newpte);
>   	hugetlb_count_add(pages_per_huge_page(hstate_vma(vma)), vma->vm_mm);
>   	folio_set_hugetlb_migratable(new_folio);
>   }
> @@ -5032,14 +5036,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   			 */
>   			;
>   		} else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) {
> -			bool uffd_wp = huge_pte_uffd_wp(entry);
> -
> -			if (!userfaultfd_wp(dst_vma) && uffd_wp)
> +			if (!userfaultfd_wp(dst_vma))
>   				entry = huge_pte_clear_uffd_wp(entry);
>   			set_huge_pte_at(dst, addr, dst_pte, entry);
>   		} else if (unlikely(is_hugetlb_entry_migration(entry))) {
>   			swp_entry_t swp_entry = pte_to_swp_entry(entry);
> -			bool uffd_wp = huge_pte_uffd_wp(entry);
>   
>   			if (!is_readable_migration_entry(swp_entry) && cow) {
>   				/*
> @@ -5049,11 +5050,12 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   				swp_entry = make_readable_migration_entry(
>   							swp_offset(swp_entry));
>   				entry = swp_entry_to_pte(swp_entry);
> -				if (userfaultfd_wp(src_vma) && uffd_wp)
> -					entry = huge_pte_mkuffd_wp(entry);
> +				if (userfaultfd_wp(src_vma) &&
> +				    pte_swp_uffd_wp(entry))
> +					entry = pte_swp_mkuffd_wp(entry);


This looks interesting with pte_swp_uffd_wp and pte_swp_mkuffd_wp ?


>   				set_huge_pte_at(src, addr, src_pte, entry);
>   			}
> -			if (!userfaultfd_wp(dst_vma) && uffd_wp)
> +			if (!userfaultfd_wp(dst_vma))
>   				entry = huge_pte_clear_uffd_wp(entry);
>   			set_huge_pte_at(dst, addr, dst_pte, entry);
>   		} else if (unlikely(is_pte_marker(entry))) {
> @@ -5114,7 +5116,8 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   					/* huge_ptep of dst_pte won't change as in child */
>   					goto again;
>   				}
> -				hugetlb_install_folio(dst_vma, dst_pte, addr, new_folio);
> +				hugetlb_install_folio(dst_vma, dst_pte, addr,
> +						      new_folio, src_pte_old);
>   				spin_unlock(src_ptl);
>   				spin_unlock(dst_ptl);
>   				continue;
> @@ -5132,6 +5135,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   				entry = huge_pte_wrprotect(entry);
>   			}
>   
> +			if (!userfaultfd_wp(dst_vma))
> +				entry = huge_pte_clear_uffd_wp(entry);
> +
>   			set_huge_pte_at(dst, addr, dst_pte, entry);
>   			hugetlb_count_add(npages, dst);
>   		}


--Mika