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:   Mon, 12 Jun 2023 13:44:18 -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>,
        Muchun Song <songmuchun@bytedance.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>
Subject: Re: [PATCH] mm/hugetlb: Fix pgtable lock on pmd sharing
Message-ID: <20230612204418.GD3704@monkey>
References: <20230612160420.809818-1-peterx@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230612160420.809818-1-peterx@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7HDTtapIXs9sNj02UKRpwuxjcl3r0y/breElpK1L7dM3MUzMSDhQqjpEcyet?=
 =?us-ascii?Q?ibtiXpgt0EilZxGkK0egS1IR2Kmj6068Xo0xfNqI+scELVmtfBGLoUYnpQDi?=
 =?us-ascii?Q?8+ytLrlzVO1NyimtrMGqcr+7gmDrvARsZWeEKRqoIGFvtJXpS3zKt5DQeDqO?=
 =?us-ascii?Q?JrWvXGQMmmEzZcixdBslvOWmnpcg0YSw5ecQVqLD07MkpvYRPMi7WdaKA/lq?=
 =?us-ascii?Q?tyvp4Qkfc4oLoBQnsH0ZQZ4eqr+ytlZvUidAAJtpHyyYiQkqfvUAWSkbYo9u?=
 =?us-ascii?Q?n9jTSYN/KukQeOOCPC/nKdPrPUcgnPKJsEWuWFHDt2Yivwfkfjq6A2jt8wl5?=
 =?us-ascii?Q?NuG+yUWfRDRxF1SOLe/WpFdd4BaAnXP5wnGo5zE3sPxDUUTQf6PeaqJxTVK9?=
 =?us-ascii?Q?/ev/7XTHnSWirkmtO7Dxvt4SGpjQFeXyImRGCgRqKauaQKp+JvJ/lBKRPyyx?=
 =?us-ascii?Q?RvxAH8d7oH8yuulOi5ntTBZ5lPlghlFPqkZq9Fvz/Lo3KLMGkVMMny9GQf+a?=
 =?us-ascii?Q?jdYokBpMfbskRtWVL+dmzSFsRusaeKNh/xhs2FnyTn3I6rOZWdj7iVB93rxc?=
 =?us-ascii?Q?gXPObZLZk1Knz8FTAn8eCF46wMCdIl1bTZP3AxmU/WsPNXg/UEyRxe2DjO0r?=
 =?us-ascii?Q?891gi0nDbni4HufV//TflXMBYOGt59CHFiHcc6lK5AwJLpG3GWJ9xFMK4yMS?=
 =?us-ascii?Q?Un+qZ2f3GJgKDp2GgzgGfwVXDCONRHXn6/oO6v8Ch5USCEEs7w0b4eoyLYkC?=
 =?us-ascii?Q?2D+gPtXchaPuerPamhKZSP6Awi8LnBhzq14tBxRh3KHJkgEDogA8vv9ySP4N?=
 =?us-ascii?Q?m7sHznL/XmqJ+04N8uML3L2cT00bRSficidIVimIO0/yQTUlLtb9r00ESVbP?=
 =?us-ascii?Q?LvqKMmP1eax9iLEhp3Ytbmu8MQUhR0O1OlyQX+0wAQh/mkTil/4Vrp9z/TYx?=
 =?us-ascii?Q?APf+neZuLg4Oj4oP5sDmLNIzAWfsyg4jhzo5UuU0eHYhXrlTNQqoFBhWvOdM?=
 =?us-ascii?Q?CixSFybfQiJWFWYG6ZjszS/1P2D6HGsH+dcoJpbvegQ8u6YeAqrPzlR1wXM+?=
 =?us-ascii?Q?Sx7D5qp/w+YDhdqEUAh6p3H7m1TSAJkFajmyOACN8eNwFcVAUICgWBRo+OhJ?=
 =?us-ascii?Q?63XcdIgL5J+RHQDxJrJOtaDuiPFXBUMQAGJxw9RAPcB78loqbt6XLZ5KPpSW?=
 =?us-ascii?Q?d9+QF70EeC2VKfRsO6GSmD4Ko7TwyM2idJTb0sBBZJ2xOI4ByazRE/5W0aLS?=
 =?us-ascii?Q?kytbhcFxH1Uxv4vB8sOO6xhdkGY3LfPmMjhi3aLVEuQU6BAgfPNoEIouDx54?=
 =?us-ascii?Q?v4BlIkC7Q4OgVM5975s8AR8sOAb2spbZF47qJ1ApnIhcUo+M/HNDybUmyA0V?=
 =?us-ascii?Q?Wl7enYDKDZYBXpt7fPX7qHHbMsG5tE4+U6vNq1Otkev1MlTRykimUZo6NM4w?=
 =?us-ascii?Q?xut8tv9aEqKQ3/8srSsSfIQaa5gqGZwevhE+RMN5X+aLqUJIWVTeb2Jqba/s?=
 =?us-ascii?Q?RGc2TsYNnjQQAWsgmR8UuOqfA7LJquSjRYKPWGCbugPpI50N10TB+h+JHGMf?=
 =?us-ascii?Q?+N3VeXk8IghtHufBSVdlJe2x6igq+i4cx5Hn5j/k?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?le1d21L1xMKdu5rdv4sFwm2QyJrhOhetGXA0XMNrwa6s2EopLqY4x5adpgO8?=
 =?us-ascii?Q?nP5BLcBz1nVzOduGc3LMjGDyQ3inuQv9kHHiMyolPt4+UZ5EHSF53tl8WSQa?=
 =?us-ascii?Q?RXBOhWcrYSF3Wep0zFzMrBm5IiFbxga8i9eCRIl3XMqAfbsqW1xFwRY1208f?=
 =?us-ascii?Q?AFCsmQaaXiY+Dg3UuWOUCoZMjkJdgh9opEsJ91nImCx3djt31DKY5Ylrgo8P?=
 =?us-ascii?Q?j9iu7pp0u+CJSdvHRM5NpvKpZjcSG1U7fujCpAs0cQ1jHQj4cUmdaixeoQj/?=
 =?us-ascii?Q?6FsYbyrb+UtdVthmQ034W/jpVj/h+w0FxXcHHZrXvO/H+6EK2/FXm8gJInJx?=
 =?us-ascii?Q?Z312xvSGtybzQcBNofvnSmAPRvVFVH+IU1QHpNveE8Kg70dA9G7cZBEYG/o3?=
 =?us-ascii?Q?c+bTIPXQzfi5uIzQbIKfzJzmxPscJ+B6ZtvydTb94LXJq1ecwdhJ65aCCutg?=
 =?us-ascii?Q?1Jx2k9spH8RJ2ki4YJrer/xuqDX9ML5m1/eIRXt3RHQHpg72AylLrDLssl3p?=
 =?us-ascii?Q?0bCmi8WbWzcmceC4V8J4kqnJcu8p54nwuZx26jVrhlUBLpemKqoUS3prD8Jm?=
 =?us-ascii?Q?IiBHGnTymMQM6xw0ofkYcSMUH8ciEKzDj1+pWwcr6lm+2m2MklbvcWsBnkai?=
 =?us-ascii?Q?oJN9NDr8xPjjK+L1l+ABOYOpkA4b9FB5tvfnFEs7BRxDF3F0Xn1U1F5Y02rz?=
 =?us-ascii?Q?NUcULTO7TXhsrMBhAIcncI57GORz51/iHM5JLmeQROdNNBJ0yhYs9VyJYmBU?=
 =?us-ascii?Q?6MA6Njy8ScQJAoaYOpp1OV9a3gfvVuXTYgZEmqMnlz1JLBxUnh2NElpU7aB/?=
 =?us-ascii?Q?jNErVmxopcWZxrOkyKwx6XeIFzoBuDB+e0KRIRx1YH2LNf+E7bYdpowpqqDb?=
 =?us-ascii?Q?adJ19WeXeyeBr5zRu2Tcg/nhBvKReAYuQFbKWXc74vkZFTEW4F/3+nJcu5Kv?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c34782d8-4d00-4faf-14f7-08db6b85cca8
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2023 20:44:21.7303
 (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: BjKj0Rm3myXKzZ3FM57s839z0mIkZHrG1T0H+NJbJee38XRsN7a8GK5wobz73Ljm6gN4HX61wGsTzLJ/7nw05Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4800
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-06-12_16,2023-06-12_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0 mlxscore=0
 spamscore=0 malwarescore=0 adultscore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000
 definitions=main-2306120177
X-Proofpoint-ORIG-GUID: 9vYXap7uQtlKgg5skpJsMtf1cBXYh_R6
X-Proofpoint-GUID: 9vYXap7uQtlKgg5skpJsMtf1cBXYh_R6
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_H5,RCVD_IN_MSPIKE_WL,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 06/12/23 12:04, Peter Xu wrote:
> Huge pmd sharing operates on PUD not PMD, huge_pte_lock() is not suitable
> in this case because it should only work for last level pte changes, while
> pmd sharing is always one level higher.

Right!  That lock does not prevent someone else from concurrently modifying
the PUD.

> Meanwhile, here we're locking over the spte pgtable lock which is even not
> a lock for current mm but someone else's.
> 
> It seems even racy on operating on the lock, as after put_page() of the
> spte pgtable page logically the page can be released, so at least the
> spin_unlock() needs to be done after the put_page().

Agree.

> No report I am aware, I'm not even sure whether it'll just work on taking
> the spte pmd lock, because while we're holding i_mmap read lock it probably
> means the vma interval tree is frozen, all pte allocators over this pud
> entry could always find the specific svma and spte page, so maybe they'll
> serialize on this spte page lock?

It seems they would serialize IF they were trying to instantiate the same
shared page.  However, I suppose another thread could be trying to
instantiate something totally different in the VA range represented by that
PUD.  In this case, it seems like there would be no synchronization.

>                                    Even so, doesn't seem to be expected.
> It just seems to be an accident of cb900f412154.
> 
> Fix it with the proper pud lock (which is the mm's page_table_lock).
> 
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Naoya Horiguchi <naoya.horiguchi@nec.com>
> Fixes: cb900f412154 ("mm, hugetlb: convert hugetlbfs to use split pmd lock")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  mm/hugetlb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Agree with this change.  But, it does make one wonder if the pud_clear()
in huge_pmd_unshare is safe.  Like here, we really should be holding the
higher level lock but are holding the PMD lock.
-- 
Mike Kravetz

> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index dfa412d8cb30..270ec0ecd5a1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -7133,7 +7133,6 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	unsigned long saddr;
>  	pte_t *spte = NULL;
>  	pte_t *pte;
> -	spinlock_t *ptl;
>  
>  	i_mmap_lock_read(mapping);
>  	vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
> @@ -7154,7 +7153,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	if (!spte)
>  		goto out;
>  
> -	ptl = huge_pte_lock(hstate_vma(vma), mm, spte);
> +	spin_lock(&mm->page_table_lock);
>  	if (pud_none(*pud)) {
>  		pud_populate(mm, pud,
>  				(pmd_t *)((unsigned long)spte & PAGE_MASK));
> @@ -7162,7 +7161,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	} else {
>  		put_page(virt_to_page(spte));
>  	}
> -	spin_unlock(ptl);
> +	spin_unlock(&mm->page_table_lock);
>  out:
>  	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>  	i_mmap_unlock_read(mapping);
> -- 
> 2.40.1
>