Received-SPF: pass (google.com: domain of linux-kernel+bounces-146010-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <28976a8e-678e-4cfa-8748-e566c9c29053@oracle.com>
Date: Mon, 15 Apr 2024 16:32:28 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] fork: defer linking file vma until vma is fully
 initialized
To: Miaohe Lin <linmiaohe@huawei.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Cc: akpm@linux-foundation.org, brauner@kernel.org, oleg@redhat.com,
        tandersen@netflix.com, mjguzik@gmail.com, willy@infradead.org,
        kent.overstreet@linux.dev, zhangpeng.00@bytedance.com,
        hca@linux.ibm.com, mike.kravetz@oracle.com, muchun.song@linux.dev,
        thorvald@google.com, Liam.Howlett@Oracle.com
References: <20240410091441.3539905-1-linmiaohe@huawei.com>
Content-Language: en-US
From: Jane Chu <jane.chu@oracle.com>
In-Reply-To: <20240410091441.3539905-1-linmiaohe@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?empRVW1SNFlobDJNcFRlWmhPVkFyU3JHQkl6ajNFVWpHcE1WL2I4S041Znd6?=
 =?utf-8?B?UXlWZlFibjU2ckgvRGVpcGhCTE0zL1F0dHFpYllaVU1uTGpxWlc0aStaMDFT?=
 =?utf-8?B?emJCbXpEdkkwN1FabXN0YWdtYi9KRHFNaWtNUGNPaUFwbFYrQXlNRlVlTVhI?=
 =?utf-8?B?bkRFNjVpM0FEanp6UTJCVTlaT05qT2xnNDhER0V5TVloRW1NczlwNlJLcXJM?=
 =?utf-8?B?WHFRZFRXY0M5MjNaRmxHM041VTlvZ3M2ejBVM3loMWpJNDdSMUNaTksrUTFC?=
 =?utf-8?B?MUxNeVdvVi9GWW1IU0Y1QjFTTG9tQUU2T1FORURYenBDd0hNMVlTdmYwY1l4?=
 =?utf-8?B?N2F4ODdsYjJic0dlREw1aExXN2tEVys4L0Z6R3FWS2xoQW8vVHFER292TlVJ?=
 =?utf-8?B?R0crMmpQYXlWOTJCdnNoTlVIVzMrS1JGOXdoalZrN1I5UXVQOUNOcXYrOVE3?=
 =?utf-8?B?L0V5eUhseHlHV3ZZR0NIK3h3dlgvQkJXMWU5dHNCd1daNW04NGpmMGVnajI0?=
 =?utf-8?B?aGpxUjJ6WlZiMDZBc0QzbndLMUtqMGRZRnY2U3EvUFNiSmtJSXh6aldHU3pM?=
 =?utf-8?B?Ykp1aXNmbGozZWFKVnlUVTNkS2wrcWhzeEVzZzQwc24vWVZaVi9EakJJR01Y?=
 =?utf-8?B?NnFQMC8rSDFKNDNrNUh3OXBFUlFZK3NxRVllc2tmNUNOaEJIT1BOZEF6ZWpr?=
 =?utf-8?B?Yy94OHdhRDhwL00vaTA1azcyT2h2OXM5OGpVZnhTeVhrWG40NEtmV0Jsdmpz?=
 =?utf-8?B?bGxtVmpwNjFOdzFURTEybng1QjZPelEyZXhPTno2WG41TXhHeHJWMVlHRitM?=
 =?utf-8?B?Z1ZtVzhoSGJhSFhCSHdMV24vQ2ZlVVlQcEY0VTFoeG1HQkcwNzE2RTdpV1BC?=
 =?utf-8?B?M2MwOTgzSTNscHB3Zmk0UXZqWFpPY2hYdithNmRYbTlEaDNXLzRNTWxFQTRl?=
 =?utf-8?B?aFhwZmdGYzZuREp1S2dBd3E1SlY0MWZ6azNCYml6clFZWXZMMGZ4NXRYQ1Ro?=
 =?utf-8?B?UjVMY0lqWGFZSWlDVXE5SkZrUWhHRjNCSjhXMWdleU1iYnFEckF6MXJ4Vzkx?=
 =?utf-8?B?UXk3UG16ZVdna2F4SEltN3Bvd0o0bTNoRzduK0tCaEFCdTh0QWdTeHYvQVdt?=
 =?utf-8?B?dy96aFJpYkRhZFVoSHpqTFFrM2tnTE1nS29MNFo5WExQT1ZTWGQ0dUhLelBZ?=
 =?utf-8?B?TzJIOFlJa2UxbzhHdDNpVEphU0ltR1R2WXV1R01HQnZuZW82ODFqZ2Y5TlAy?=
 =?utf-8?B?anR1bkFObWpicklLelJOVGw3OXUrb2sxUE9KZVc5dkRVQlZ1ZmhkQTd6N1F3?=
 =?utf-8?B?LzNzdzVDcnJuTUlRcERJWWtWdHYzZENMbXQrcFdaVmpsUiszOG1vbVgvSkpw?=
 =?utf-8?B?OXU2ZExVTXNCTlFZZmMzRlZxM1dmTHZFSk80aUFYRlFKZS9Qa2xXZUV4eWhL?=
 =?utf-8?B?dHdhR1BVN0lXUkdNNUl6aEtjWHNJUm9PU2tjc0d1TzVIUCsyc1VHWVpndnIv?=
 =?utf-8?B?SElkaWtZWkVQOGNVNWh1dW5WWjBDdEI4QzJtZ2xzVXNpOWZSZlQyZStHQVJp?=
 =?utf-8?B?T3E0WTVjdDFmRm90ZmQ3Tit4ZFQwVCtzZ2phdXVuREhnWGExQm01UHF4Y1dL?=
 =?utf-8?B?RUxMdGxwNE9TUjQ1czNnZXBHNHh6OWxWMFlZUDBOUTZub2loeW84WXRmYWdY?=
 =?utf-8?B?MDN4eTF4MEVwRXBEQTFNeGhjZXJaV09QbURSdk51NXYyNk9TeVVINERUL0Rr?=
 =?utf-8?B?ekpYYnp3ZFNrV1ROc1dKbGRIRmR0ZEl1UzVCTHoyampiRmttaWx6a1huaGpj?=
 =?utf-8?B?VnV0MXhXV2tmcHd1OWlETVZqN3R1bWNUM2JHclMvQmNLVnN5Wm01TkhLSGFl?=
 =?utf-8?B?U2l2UTRRM0R5aWplSnhtRW5oU05YVi9OR21JUERJUzkzUSs1YVNqWTJQT3dr?=
 =?utf-8?B?Rmt1TXRFV3p3M3ZBZ1JiUlBsN1VZQTU4b1o5bU53YXhhSmppWkJmKy9BeEtq?=
 =?utf-8?B?aGZWV3NXYThMWjV1RVdrMkpLWFhtaXRrZVFmRG1JWjlvY0pya3FHQUsvVHNJ?=
 =?utf-8?B?cnFJdE1LQXhpT2JHcHlvTDkzMFBDdUlzSCtQM3FZU3J6S1lwYUFWNStkWVRh?=
 =?utf-8?Q?GyJCWywdBN2IN2ghiUNUAtQYB?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	L3pSmRvKv9WLI7Q8lA89ezTdpgYeTTh2bZrgj8+jgWtCSvGbWOwDeIAyHj9sVAUPzovVuAZeOEahws/C+Jtgc5dps6CD//aX3zbBmnBBs8mRu1W4EshKW6tJHxz128RWV8x+QfBHiLo5LVhXOKD9t6cRbWegKtNdtDpZcR9d29CEM9uXKz77gBPO1nPpuZouOHWMYQUi9DE/Y+2ixM6CDgkjA8rVzI/jG4EC3KWq/+bqQLldvlfWBOt/WvV5skCw/kelf25acpXD0/PgDbaRFFhEd67IVXIx+4TCtQ8yWkcZKRBrDWJGILOsQQaGoELJU0AJJlzxTpnIodvmgsDArV6hcpTF+do7yIznXxkldcA8YzBHBMSWvFUt78+y4PemXKjNlmz00TIol69wZdJgKbHe0IDod4PJVLuJLz9AzJzO7qJdzbngnEYPf3IM/E7vUlKlpJRqzhrkW7RzNRURGO3XLGamzvqYWBiPn0hHfsZ9wLf2XiQaR53Mspq0bIT0opLrs9BEDTC8wHtDpZFpqy9vBJkdCRjZtUnedABvTKyvfeGkBLygvHIn0SM1AvZXfXH0oWcaDLj6m69N/KJKNK2qui0eq0tHaBc7iAeTGKE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5ef5ef9a-9f22-4950-9a2d-08dc5da45208
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4429.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2024 23:32:31.3825
 (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: Asf0atLDKG/TF00W+KpvB7ZuHwFLlhPSkyyXcURmQxZrkhLy5wd8TZ3w2DP1MG9yzfXqMHrPk6DwsI/YSe23ow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN6PR10MB7541
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-15_18,2024-04-15_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 adultscore=0
 mlxlogscore=999 suspectscore=0 bulkscore=0 spamscore=0 mlxscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404150157
X-Proofpoint-GUID: 4vIaaX_z7Yu68mF2XN7stqSVD06S4y2m
X-Proofpoint-ORIG-GUID: 4vIaaX_z7Yu68mF2XN7stqSVD06S4y2m

On 4/10/2024 2:14 AM, Miaohe Lin wrote:

> Thorvald reported a WARNING [1]. And the root cause is below race:
>
>   CPU 1					CPU 2
>   fork					hugetlbfs_fallocate
>    dup_mmap				 hugetlbfs_punch_hole
>     i_mmap_lock_write(mapping);
>     vma_interval_tree_insert_after -- Child vma is visible through i_mmap tree.
>     i_mmap_unlock_write(mapping);
>     hugetlb_dup_vma_private -- Clear vma_lock outside i_mmap_rwsem!
> 					 i_mmap_lock_write(mapping);
>     					 hugetlb_vmdelete_list
> 					  vma_interval_tree_foreach
> 					   hugetlb_vma_trylock_write -- Vma_lock is cleared.
>     tmp->vm_ops->open -- Alloc new vma_lock outside i_mmap_rwsem!
> 					   hugetlb_vma_unlock_write -- Vma_lock is assigned!!!
> 					 i_mmap_unlock_write(mapping);
>
> hugetlb_dup_vma_private() and hugetlb_vm_op_open() are called outside
> i_mmap_rwsem lock while vma lock can be used in the same time. Fix this
> by deferring linking file vma until vma is fully initialized. Those vmas
> should be initialized first before they can be used.
>
> Reported-by: Thorvald Natvig <thorvald@google.com>
> Closes: https://lore.kernel.org/linux-mm/20240129161735.6gmjsswx62o4pbja@revolver/T/ [1]
> Fixes: 8d9bfb260814 ("hugetlb: add vma based lock for pmd sharing")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   kernel/fork.c | 33 +++++++++++++++++----------------
>   1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 84de5faa8c9a..99076dbe27d8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -714,6 +714,23 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>   		} else if (anon_vma_fork(tmp, mpnt))
>   			goto fail_nomem_anon_vma_fork;
>   		vm_flags_clear(tmp, VM_LOCKED_MASK);
> +		/*
> +		 * Copy/update hugetlb private vma information.
> +		 */
> +		if (is_vm_hugetlb_page(tmp))
> +			hugetlb_dup_vma_private(tmp);
> +
> +		/*
> +		 * Link the vma into the MT. After using __mt_dup(), memory
> +		 * allocation is not necessary here, so it cannot fail.
> +		 */
> +		vma_iter_bulk_store(&vmi, tmp);
> +
> +		mm->map_count++;
> +
> +		if (tmp->vm_ops && tmp->vm_ops->open)
> +			tmp->vm_ops->open(tmp);
> +
>   		file = tmp->vm_file;
>   		if (file) {
>   			struct address_space *mapping = file->f_mapping;
> @@ -730,25 +747,9 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>   			i_mmap_unlock_write(mapping);
>   		}
>   
> -		/*
> -		 * Copy/update hugetlb private vma information.
> -		 */
> -		if (is_vm_hugetlb_page(tmp))
> -			hugetlb_dup_vma_private(tmp);
> -
> -		/*
> -		 * Link the vma into the MT. After using __mt_dup(), memory
> -		 * allocation is not necessary here, so it cannot fail.
> -		 */
> -		vma_iter_bulk_store(&vmi, tmp);
> -
> -		mm->map_count++;
>   		if (!(tmp->vm_flags & VM_WIPEONFORK))
>   			retval = copy_page_range(tmp, mpnt);
>   
> -		if (tmp->vm_ops && tmp->vm_ops->open)
> -			tmp->vm_ops->open(tmp);
> -
>   		if (retval) {
>   			mpnt = vma_next(&vmi);
>   			goto loop_out;

Looks good.

Reviewed-by: Jane Chu <jane.chu@oracle.com>

-jane