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;
Date:   Thu, 11 Aug 2022 11:59:09 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        stable@vger.kernel.org
Subject: Re: [PATCH v2 2/2] mm/hugetlb: support write-faults in shared
 mappings
Message-ID: <YvVRfSYsPOraTo6o@monkey>
References: <20220811103435.188481-1-david@redhat.com>
 <20220811103435.188481-3-david@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220811103435.188481-3-david@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AfwujUO4OqBgpnyB+8NO9FWO4Qp7HMv9vf76JYfboWDZwlOsxuAyjlQb6PKa?=
 =?us-ascii?Q?tyO/HyOJ75unsS0c5anb2HfJn/Gsvwc288nVBn8xou3Q+d6+ux9bnPMUrP/V?=
 =?us-ascii?Q?h37dW+hwnZqp2hP4UQyhOilH/7ryi+iJwe13xERdjFACofR7YDtmmIFeKLh9?=
 =?us-ascii?Q?+EZw06KHPWXz78MUh6lu3EwLVcOm1Vd5r4rS5Nx3/N4GNYBKKLRJXmAZnmhs?=
 =?us-ascii?Q?ahhHkHq2EAI1pE/r4OcKApAu2XWjIdccpr/Mxd8T9DihgnUn0IRt6gst3elI?=
 =?us-ascii?Q?qJEf31fTVLPQb8A/YaPoSrAjMout3bJ42abBfNCnnfrZrfW4Fv54izV1wqSb?=
 =?us-ascii?Q?q3T1aKl++orHd5fGJWxuftMOENStGtoO2Qqvgfvo0+hY3IQqdQxR3YgXSbaB?=
 =?us-ascii?Q?95FybXiymxuIFhKZdfBeQ4/WnDlZD4bEmN8HO26cBbuVReOEpTaDJlaVasvW?=
 =?us-ascii?Q?tWKMrKRK/Sn63daUz59f5SoZ6JTqn4WYl8IRJ3ienbH9EFVPlSsPqZEaW8oX?=
 =?us-ascii?Q?dSrSVp7t0LiIXonxurqONdWk7tHbG0NoyDYwNe3ze9r4RxlTkaZ4geiOY2aB?=
 =?us-ascii?Q?nkVfnl7Oyv8RbLpzqwe48Dt9jzGUaqoC8/lFuddDEF+HC20Df8UDAA+pzLHo?=
 =?us-ascii?Q?Y7YdRZ5ihnV097O42iN64QJrymItsk7b5SBIZDqKwUSkNFCkZF8NSmGbcs7v?=
 =?us-ascii?Q?SEXKTblIvPUXzV1JrtNHv2ee+mCGE4xGYEJoQEJ0Qpo0hts6YRy7KzMOD/is?=
 =?us-ascii?Q?5R+bed5Mwa+cUFMKHrKHsp6DPfFIyfoSm5Jj0UTbVshbAVOjv7l10N1QiPTt?=
 =?us-ascii?Q?cjDDmwe1tdcGAGH8eCGeZdEwEfh+Vs1q4bvPGrLCBJwJrD6VMoTFbnk81nDt?=
 =?us-ascii?Q?sBpOrCLXxGjcMOW7DDznx61PFApDDnsl7wnBzqgy9mUFOgcU6pq4n9hyF4wh?=
 =?us-ascii?Q?EHj1+t2e+bdej8UjBeSg+6VxpLWczV1mS/WJ/rzZvRfD/A3LLQYQxaffDPls?=
 =?us-ascii?Q?jEgJCqNS/zLwwfTSkjdvYHpsI/jrtjB5PSrolUfLHAp4P1LvMo9Cz7tOGeup?=
 =?us-ascii?Q?YLQ16W2aTutAfRPivEOQYP4bI9QH1D+n/e0D0oj+1qIuB8QaMCXfUdTgzXJE?=
 =?us-ascii?Q?oXsxaMiVKj6nESFj/znwaUZtM6926uXKZkSi/Z/ev8sCfXltcS++gJPjToqO?=
 =?us-ascii?Q?0khNtd3HWHeWpu4m3QR3pcM7hy4NyWV6ycBKIJdgzzFyIkzjV4AiZvwI6J6j?=
 =?us-ascii?Q?s06rDjbfctKC/FTRhMeqS8l3svvlrZlj1NvZVi392KXE+in79+H3ohQHlliw?=
 =?us-ascii?Q?E+d8cw7tX/m6OC3GYfa1zDmH4Q597jBXFc5i3kkcXitHCE8iyyjeWIlQ9bD7?=
 =?us-ascii?Q?Cz2Vgk7Rjzce7SfpblMy5qYuo0Pf14PD4kv27NvTrVwVwSKcoOfMRYu+88dk?=
 =?us-ascii?Q?tXOQpZsoJmgJcDgUbDy5v08+d/0IGFojx+9Bv5JYUNskNc/ztzwRCCu0Icm+?=
 =?us-ascii?Q?jF+bJAOvk/wBBqbxpL/kQH0+PT9Nnd7zlWRW6PhK3t/hbWnloNz2qUmMRp3g?=
 =?us-ascii?Q?4e4zT7QhcfrOsAn9QRd5eUlxid4vhoE/sj22HebhJpYGS+KoidlO3m0Bu24k?=
 =?us-ascii?Q?Eg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c789364-019f-4ef6-7b95-08da7bcb94a8
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Aug 2022 18:59:12.9930
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: czmLSoAZ3kiPk6ir0Q4+WM5rrJpG96zvif4m8IujMuLglOYdkr4nk9ZuGmdp9uC3KBSBK3Tawj+8MICEPay+TQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5629
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-11_13,2022-08-11_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 suspectscore=0
 spamscore=0 phishscore=0 adultscore=0 mlxscore=0 malwarescore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2207270000 definitions=main-2208110060
X-Proofpoint-GUID: qPUS7PJd6RfKFqrgU4EIc519dg9VryhG
X-Proofpoint-ORIG-GUID: qPUS7PJd6RfKFqrgU4EIc519dg9VryhG
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 08/11/22 12:34, David Hildenbrand wrote:
> If we ever get a write-fault on a write-protected page in a shared mapping,
> we'd be in trouble (again). Instead, we can simply map the page writable.
> 
<snip>
> 
> Reason is that uffd-wp doesn't clear the uffd-wp PTE bit when
> unregistering and consequently keeps the PTE writeprotected. Reason for
> this is to avoid the additional overhead when unregistering. Note
> that this is the case also for !hugetlb and that we will end up with
> writable PTEs that still have the uffd-wp PTE bit set once we return
> from hugetlb_wp(). I'm not touching the uffd-wp PTE bit for now, because it
> seems to be a generic thing -- wp_page_reuse() also doesn't clear it.
> 
> VM_MAYSHARE handling in hugetlb_fault() for FAULT_FLAG_WRITE
> indicates that MAP_SHARED handling was at least envisioned, but could never
> have worked as expected.
> 
> While at it, make sure that we never end up in hugetlb_wp() on write
> faults without VM_WRITE, because we don't support maybe_mkwrite()
> semantics as commonly used in the !hugetlb case -- for example, in
> wp_page_reuse().

Nit,
to me 'make sure that we never end up in hugetlb_wp()' implies that
we would check for condition in callers as opposed to first thing in
hugetlb_wp().  However, I am OK with description as it.

> Note that there is no need to do any kind of reservation in hugetlb_fault()
> in this case ... because we already have a hugetlb page mapped R/O
> that we will simply map writable and we are not dealing with COW/unsharing.

Note that we are not really doing any reservation adjustment in
hugetlb_fault().  That code does pre-allocation of reservation data in
case we might need it in hugetlb_wp.  Since hugetlb_wp will certainly
not do an allocation in this case, we do not even need to do the
preallocation here.  This change is more of an optimization.  I am still
happy with it.

> 
> Fixes: b1f9e876862d ("mm/uffd: enable write protection for shmem & hugetlbfs")
> Cc: <stable@vger.kernel.org> # v5.19
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/hugetlb.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Thanks,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 0aee2f3ae15c..2480ba627aa5 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5241,6 +5241,21 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>  	VM_BUG_ON(unshare && (flags & FOLL_WRITE));
>  	VM_BUG_ON(!unshare && !(flags & FOLL_WRITE));
>  
> +	/*
> +	 * hugetlb does not support FOLL_FORCE-style write faults that keep the
> +	 * PTE mapped R/O such as maybe_mkwrite() would do.
> +	 */
> +	if (WARN_ON_ONCE(!unshare && !(vma->vm_flags & VM_WRITE)))
> +		return VM_FAULT_SIGSEGV;
> +
> +	/* Let's take out MAP_SHARED mappings first. */
> +	if (vma->vm_flags & VM_MAYSHARE) {
> +		if (unlikely(unshare))
> +			return 0;
> +		set_huge_ptep_writable(vma, haddr, ptep);
> +		return 0;
> +	}
> +
>  	pte = huge_ptep_get(ptep);
>  	old_page = pte_page(pte);
>  
> @@ -5781,12 +5796,11 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	 * If we are going to COW/unshare the mapping later, we examine the
>  	 * pending reservations for this page now. This will ensure that any
>  	 * allocations necessary to record that reservation occur outside the
> -	 * spinlock. For private mappings, we also lookup the pagecache
> -	 * page now as it is used to determine if a reservation has been
> -	 * consumed.
> +	 * spinlock. Also lookup the pagecache page now as it is used to
> +	 * determine if a reservation has been consumed.
>  	 */
>  	if ((flags & (FAULT_FLAG_WRITE|FAULT_FLAG_UNSHARE)) &&
> -	    !huge_pte_write(entry)) {
> +	    !(vma->vm_flags & VM_MAYSHARE) && !huge_pte_write(entry)) {
>  		if (vma_needs_reservation(h, vma, haddr) < 0) {
>  			ret = VM_FAULT_OOM;
>  			goto out_mutex;
> @@ -5794,9 +5808,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  		/* Just decrements count, does not deallocate */
>  		vma_end_reservation(h, vma, haddr);
>  
> -		if (!(vma->vm_flags & VM_MAYSHARE))
> -			pagecache_page = hugetlbfs_pagecache_page(h,
> -								vma, haddr);
> +		pagecache_page = hugetlbfs_pagecache_page(h, vma, haddr);
>  	}
>  
>  	ptl = huge_pte_lock(h, mm, ptep);
> -- 
> 2.35.3
> 
>