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, 3 Nov 2022 08:42:01 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        James Houghton <jthoughton@google.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        David Hildenbrand <david@redhat.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Rik van Riel <riel@surriel.com>
Subject: Re: [PATCH RFC 02/10] mm/hugetlb: Comment huge_pte_offset() for its
 locking requirements
Message-ID: <Y2PhSUnufjkoqSaH@monkey>
References: <20221030212929.335473-1-peterx@redhat.com>
 <20221030212929.335473-3-peterx@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221030212929.335473-3-peterx@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?w0dGJG4fSr/Z/j7sqKsikfhiwN7+W21no8d0CrLjCWQAgLqjQ+oe1Mlu4RAK?=
 =?us-ascii?Q?scGbhJtZjNeUeceoDjicATtFYkNEH9dpKcNO28zKgTDGuWwWDlTuyGQbGJea?=
 =?us-ascii?Q?Qdnv304dMFnyaUjYTbBRYn8a32c6VArkdolgeW2vPwX9xLCp+5SK+Lehaa3N?=
 =?us-ascii?Q?4GOKRAtXF2zq+bHeaXjTMgtS9EnvUlgNhDk5zba7EAr98oY8S1D0T0z2stN2?=
 =?us-ascii?Q?JZD/qOBtyi7RSuMlz9J4uFheDulMrt9HX13pOdSw+kuGmolfEmUt0FnuIcxJ?=
 =?us-ascii?Q?MDNGm5+aAbtiilwpwN5LKYQegNNkdBCxy310fNiuxbyJUOz9+W/1M2RYD/Pe?=
 =?us-ascii?Q?gHwd0Ek5VTC92w0eZDMGrkBw9eMOavejq4mSSID6DMUlRM+aAXR5ElNHRZG9?=
 =?us-ascii?Q?hqjZFywZRf63RgEWZHSC2vCak6+LFsyB4w8NBYgJketNJ5m+1UBv1Ub0mEJM?=
 =?us-ascii?Q?gX8nINsg159Ny+dqol/MAN+bkO6diazPFdYseLqG1kDASWM8U/i9+NM94bi2?=
 =?us-ascii?Q?9uXrkogaozxpp2gP0tdRX8BxX3yn0fhy2MI0QI0wEEx0VFUU3ZzDlK13tWq1?=
 =?us-ascii?Q?pBA1qyDW68Raof16OFEQqhlEr4gLp3WGOdULC/FnTJeznXbZGZfN11LO6jaU?=
 =?us-ascii?Q?DRdUJ/jJBro/Y86TL0cpAkGEfgrMpKFgf4hpl9U27lk/la0c3tBYKKB0bqLj?=
 =?us-ascii?Q?AJ1Wr78uE3Ngj8ROqbR7bkg7loZEZl4Fk0g69YY6KLjlUAUM3VPv1Pb32Aa8?=
 =?us-ascii?Q?mj+VdcTy3KcWZntO7UhMly1QtF0pL3EplXb/OFMqtUcuJSWYybXp1caDS3L5?=
 =?us-ascii?Q?3DIZ7a03ltJPXna2ScLFZ/zrnL7gInJCJkN3XmZ7VzuCZvUOanqfHQUSQZpe?=
 =?us-ascii?Q?yDcWDmEP7YVyhZo+y8IK31/l5r1cuEOpPIp3UhtIrSt2yFZKWgQnAJknRF7o?=
 =?us-ascii?Q?Oo4rSDqBoJeBeCz4RI5f/J75mABOqJmPO6PC/Ygi4qqcmypq3EPPi0/ueF03?=
 =?us-ascii?Q?Rx3NTUs6P1vJDXSKNumbrMqu3n/iMAowukWH2whC7EdJyOWov5rAHhisgjIj?=
 =?us-ascii?Q?KWFMuU/7LnGcVasMYOImf2dcruCEvYcjhb7P4SQDCz1DKnxggVvXU+ABDVvU?=
 =?us-ascii?Q?2kRHAEojh6C6mn9xnwmmTkJH4C2thsG6D2YXaN03XhOxQYgbA/akNeBzAVOe?=
 =?us-ascii?Q?RcX0Nd0GxbkhpwB1RvM+/90OBcR4wxaBGxakq3qkHUOKGCxMVtpmMOQb0mAP?=
 =?us-ascii?Q?NI+u74KdpEwgIQdoMD+s6tw7QzyX56s6K3DjGq3bVf5o+7FlMzftc6r/OXK+?=
 =?us-ascii?Q?0GP8rx0r/S7TIJH1JNuKSVO+b+y0130RETVQ5Mro5Yj5Icx+vaBOsxvt2E2p?=
 =?us-ascii?Q?MxZbuCNu+RdDrlIMie5LvxT+JX/92ws/Ygqe8yoKu/OyQOzKZGvuzSVYUBVs?=
 =?us-ascii?Q?kza6d2o/e1ExvzNxlHfgj26a7ddRP9KKmpxcLprHXhwkthX8cHqTnOsBqxld?=
 =?us-ascii?Q?8feT42I2YsoX5R5YsM3Jpf3Ej5a5hwsSZk9FMkKGbA1Odb0DD2LLdkdX7ze/?=
 =?us-ascii?Q?JERpmt5qNO5ENtA27E71tJy9k4hHASOnBtUUH1+ckFEqxLeCtchq7i9Ufz+H?=
 =?us-ascii?Q?bw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bbe15d1a-0a03-4c8e-f133-08dabdb1f6d6
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2022 15:42:07.6832
 (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: Z/B7ajbOvmS+K5T3q92cHz3R9x+RMaWJOtT46rmKk8tRhcWiDNswazcIOERZk24GrLuBOir1jI8Qrp9fTSSTxw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5812
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-03_04,2022-11-03_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 phishscore=0
 mlxlogscore=999 suspectscore=0 spamscore=0 malwarescore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211030104
X-Proofpoint-GUID: EFQhvhdFnkC2Q3-BNWZyXg7s3N8vEYts
X-Proofpoint-ORIG-GUID: EFQhvhdFnkC2Q3-BNWZyXg7s3N8vEYts
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 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 10/30/22 17:29, Peter Xu wrote:
> huge_pte_offset() is potentially a pgtable walker, looking up pte_t* for a
> hugetlb address.
> 
> Normally, it's always safe to walk the pgtable as long as we're with the
> mmap lock held for either read or write, because that guarantees the
> pgtable pages will always be valid during the process.
> 
> But it's not true for hugetlbfs: hugetlbfs has the pmd sharing feature, it
> means that even with mmap lock held, the PUD pgtable page can still go away
> from under us if pmd unsharing is possible during the walk.
> 
> It's not always the case, e.g.:
> 
>   (1) If the mapping is private we're not prone to pmd sharing or
>       unsharing, so it's okay.
> 
>   (2) If we're with the hugetlb vma lock held for either read/write, it's
>       okay too because pmd unshare cannot happen at all.
> 
> Document all these explicitly for huge_pte_offset(), because it's really
> not that obvious.  This also tells all the callers on what it needs to
> guarantee huge_pte_offset() thread-safety.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/arm64/mm/hugetlbpage.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 35e9a468d13e..0bf930c75d4b 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -329,6 +329,38 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
>  	return ptep;
>  }
>  
> +/*
> + * huge_pte_offset(): Walk the hugetlb pgtable until the last level PTE.
> + * Returns the pte_t* if found, or NULL if the address is not mapped.
> + *
> + * NOTE: since this function will walk all the pgtable pages (including not
> + * only high-level pgtable page, but also PUD that can be unshared
> + * concurrently for VM_SHARED), the caller of this function should be
> + * responsible of its thread safety.  One can follow this rule:
> + *
> + *  (1) For private mappings: pmd unsharing is not possible, so it'll
> + *      always be safe if we're with the mmap sem for either read or write.
> + *      This is normally always the case, so IOW we don't need to do
> + *      anything special.

Not sure if it is worth calling out that we are safe if the process owning the
page table being walked is single threaded?  Although, a pmd can be 'unshared'
due to an operation in another process, the primary is when the pmd is cleared
which only happens when the unshare is initiated by a thread of the process
owning the page tables being walked.

-- 
Mike Kravetz

> + *
> + *  (2) For shared mappings: pmd unsharing is possible (so the PUD-ranged
> + *      pgtable page can go away from under us!  It can be done by a pmd
> + *      unshare with a follow up munmap() on the other process), then we
> + *      need either:
> + *
> + *     (2.1) hugetlb vma lock read or write held, to make sure pmd unshare
> + *           won't happen upon the range (it also makes sure the pte_t we
> + *           read is the right and stable one), or,
> + *
> + *     (2.2) RCU read lock, to make sure even pmd unsharing happened, the
> + *           old shared PUD page won't get freed from under us, so even of
> + *           the pteval can be obsolete, at least it's still always safe to
> + *           access the pgtable page (e.g., de-referencing pte_t* would not
> + *           cause use-after-free).
> + *
> + * PS: from the regard of (2.2), it's the same logic of fast-gup being safe
> + * for generic mm, as long as RCU is used to free any pgtable page.
> + */
>  pte_t *huge_pte_offset(struct mm_struct *mm,
>  		       unsigned long addr, unsigned long sz)
>  {
> -- 
> 2.37.3
>