Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 25 Sep 2023 15:25:39 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     riel@surriel.com
Cc:     linux-kernel@vger.kernel.org, kernel-team@meta.com,
        linux-mm@kvack.org, akpm@linux-foundation.org,
        muchun.song@linux.dev, leit@meta.com, willy@infradead.org
Subject: Re: [PATCH 2/3] hugetlbfs: close race between MADV_DONTNEED and page
 fault
Message-ID: <20230925222539.GC11309@monkey>
References: <20230925203030.703439-1-riel@surriel.com>
 <20230925203030.703439-3-riel@surriel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230925203030.703439-3-riel@surriel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pJD/Jvua3GJZ3EzfAMtHF9uvss1RrK7kRM2ia9aSScsxzfhnx404I+2aXUYz?=
 =?us-ascii?Q?zKAolpJTNUB9CwmdAMXNYQicBG1dJJAdm2Jgg4XoVREZoMnz5SEcoxf4tsT5?=
 =?us-ascii?Q?VDd3lK25nO6askK9jgcM2/rM2p8oDb7cLdxbPf9ZDhjnbTBQyVOKDMEhRw6p?=
 =?us-ascii?Q?chQ8eUSnRru1Q8VOBL8RBzBM+PZj5fVa/MmEUYylBis9LJWrV6Bm8umd4uZx?=
 =?us-ascii?Q?9aJfsLvft0GpjBrCozauI1MsE2SoWU6Zsz4pKlfEH+ah9Q3sWK68CT9I/v/J?=
 =?us-ascii?Q?yVDniylm/32b37ERryiAtQ5YjZFJp3oifUwHQTI9XPVft+LFS3WbJjvYoO8a?=
 =?us-ascii?Q?ozFMCY0+hr4nvB017ow5yGmA/NOCClKYq/lEkPCe0acOpLSQkVst2oCqx6BE?=
 =?us-ascii?Q?3Oo9ZcFmCbfnoNGJdzQr6z0e03X6by+kZmbemr/4QfVkKiIjgERz1BzsigBo?=
 =?us-ascii?Q?82r/2y2gPKyx+TK7U//eHZ86OlisdSwl+VIA+J0/1v4iokKV/MO/9k9wsTuW?=
 =?us-ascii?Q?Fl5UulbZlk88j18d/Vg+L5ZRc4SOFZRWv2y5/itGctn9DlAbaw7XBKQSqPyS?=
 =?us-ascii?Q?R9B6N/RtvltjIwcWpb7GLX5gjNePFxSDF+i79Of4HxPki4b9C3dUYGPz3afn?=
 =?us-ascii?Q?NZo/X5J6EmAwGOdZnjV5o/O6jund8GeJhX3Yj0bx1v+NiRc1/sGIXDfDRcE6?=
 =?us-ascii?Q?17QxPWH4CLzY0cx2UvZTERnjd9XImGMzQ4vjnepI0JiOQQP8y5RFix8EK8Pd?=
 =?us-ascii?Q?jU6v6HnjAYXQLz0qJmcTey2Asjq0HzpeIPjfklaBg0WjCWJudPR7fTml4lYI?=
 =?us-ascii?Q?MkqU5/hl5G+ekKyhLff+agcq51N0BucKsTEfNxQ+n/1u0MvQSz/FSjzcMSVK?=
 =?us-ascii?Q?6nOHmMynqUBncQtAG24j2+jTMZL/9B5djMxSnJoU9AsKuP3tHeRqLz44aBgQ?=
 =?us-ascii?Q?I2QLCmvdXPHHxUGx4r9BPGyk3SavNQxQ6AyFXHujRPTuWJZ+WPWiuaD0ltuH?=
 =?us-ascii?Q?eBVHdf8x+Lcw0J6niBOz//hwW9GSvwZhv/xH92CCpKaN0fS0RUwKTtiUs45Q?=
 =?us-ascii?Q?XdNDU4ioQv2vcbn2I+C/SRo9ywB50bKPyGOTqfH9Phm1w/Q/iaoB6put+jDY?=
 =?us-ascii?Q?LCCjnc/jN6YlGBGbr+RKcqJjyNP3vErj3RyajJlmxTXbhZXi54boBVIj7N/2?=
 =?us-ascii?Q?3TJnjvc/KEB0auuMe94BcbSx1kLS0F9qMbCoCdrIvQkaxEw0fJXwau4gsWc4?=
 =?us-ascii?Q?JT9uiwb4w9MxljQwWaS4wOPA+WhFZOiLoSlvpY2+EgeJxpeMadpLKmOiAX2z?=
 =?us-ascii?Q?VoICfTzz7xgZaU76rOP/5zWrsiJZLRYS+fVZv7njzWEYnuEGQ0D6frfuah9Y?=
 =?us-ascii?Q?8TyFjAYvrruKT/5CGyG0CiOueplihrT1ULOdpYmk2yWajTyZ9aNbV6dm9VjX?=
 =?us-ascii?Q?zXIpEhVM/Q7Iwel4muuTjImpxxX2pTFywhqhlFX25a4m0TBRb/F12C4Libx5?=
 =?us-ascii?Q?SGTLOQBL6eSVUpbfzwLaiSGhnQJYdnJggWEv/MSXd+3lPzpQ6aAOSc9CaMl0?=
 =?us-ascii?Q?5GjX/qZlEA1fHgy3RNj2o00dbVxQwGUcLw+Zm6UuQx5ZSyns7M4bXxRkgk6Q?=
 =?us-ascii?Q?Bg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?cVEX45iaFcrr4BYRu3cLs6mM2+imq/s3G5bnSYhjmgh0rCsUuGMYNxWnqgHO?=
 =?us-ascii?Q?GftnCBfE9FbYQ/YUsWLi3WKLgzqitfzLtzL94TSh/W1hjWFglefTpB4/2z6x?=
 =?us-ascii?Q?DmzewEBmxXAZvG0BOFVR8xBNIqboktwJNSVNS1Wm8K+WlIN7Eniu6hxSF7KI?=
 =?us-ascii?Q?eRP4NkBTUY4IRdyUKt3wiaFvStO0ZciYoMTXwJ9RMzz/8IJGqlsBJi8S5Z2l?=
 =?us-ascii?Q?QOVqQvXVhFViXbhK3QPal24sHauZ1Ls/zPQ4xRyOpXpS/dwbA5EfMgFYOMlJ?=
 =?us-ascii?Q?0KkBWXM9sLnzNPUvnZ8W96BfJlswfdslUgfcYokhNtQw6eiE315RzNMUFxJ7?=
 =?us-ascii?Q?itff93SC6H5+PK58osnYQvPe+jBca6Tb/zXC9UbJr4NOeWhMvMX9WysJ0pUa?=
 =?us-ascii?Q?2t5daYk5EA3DIBRVlqMafrvlMYiUmDSViHU5aQ5dOKPmiwqWC1zWEqsr+CI2?=
 =?us-ascii?Q?WAMZa7yi+N/lADT1NobCb5mGu4Un4/AhA8eh5bBZ0dgAOXpetvMO6oFNja4O?=
 =?us-ascii?Q?mP0Q5QMCT+SpXzT/QGz6WyWHH5FmbtcRPU+XDdUa9F2wPlACJ0m9mbLG21b2?=
 =?us-ascii?Q?6X9mXBUnLvfI2ToGLrbjehWbD80lbJDlT1VdVMs8YSeY679CM4tyBv7CXv+b?=
 =?us-ascii?Q?4ZtdtlJ0fdINFKHNWWsb8odfsgkwyYtNQK06ScqrCFfDUl2rfd4rLkRQhB/g?=
 =?us-ascii?Q?cjr8DCbaHJjE173KarIdvx+Zb5cbbDo78iImLwdS2W1K9Ka0306quLIXrVDY?=
 =?us-ascii?Q?iuR9mVFkK/q1TnovFY6Bc/mn/c4W6ADIt4hojPowTNhf7LsDB9835vmfwWg/?=
 =?us-ascii?Q?a3zNHw1dPHRurofCGpUjkHpnlIBesl8fLIfRizHOeTiqXjDOI0HYlonn+hyf?=
 =?us-ascii?Q?5ww6SZOVs4LJ5hAhpj8k/Kw9+W6MeiEJY8jYGYP9RrLszpdBWQtsb4iJ5JLk?=
 =?us-ascii?Q?7LpWSPcagc0r6Tehdp+Wjw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b62405aa-eb81-40d0-f47b-08dbbe165af9
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Sep 2023 22:25:42.8948
 (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: v/qHjQVgs40cgabgF7xA8FfTuHLPGQtz7EhOhqnUnGOOZbwcIGKJyxH1+J69angkK7Ny2KoKTLZPhxwO2Ss/jw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR10MB7229
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-09-25_18,2023-09-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 mlxscore=0
 spamscore=0 mlxlogscore=576 bulkscore=0 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309250172
X-Proofpoint-ORIG-GUID: mGt3tDjxmUyJ_pd6jv_mz4lN_ms9AOWU
X-Proofpoint-GUID: mGt3tDjxmUyJ_pd6jv_mz4lN_ms9AOWU
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 25 Sep 2023 15:26:13 -0700 (PDT)

On 09/25/23 16:28, riel@surriel.com wrote:
> From: Rik van Riel <riel@surriel.com>
> 
> Malloc libraries, like jemalloc and tcalloc, take decisions on when
> to call madvise independently from the code in the main application.
> 
> This sometimes results in the application page faulting on an address,
> right after the malloc library has shot down the backing memory with
> MADV_DONTNEED.
> 
> Usually this is harmless, because we always have some 4kB pages
> sitting around to satisfy a page fault. However, with hugetlbfs
> systems often allocate only the exact number of huge pages that
> the application wants.
> 
> Due to TLB batching, hugetlbfs MADV_DONTNEED will free pages outside of
> any lock taken on the page fault path, which can open up the following
> race condition:
> 
>        CPU 1                            CPU 2
> 
>        MADV_DONTNEED
>        unmap page
>        shoot down TLB entry
>                                        page fault
>                                        fail to allocate a huge page
>                                        killed with SIGBUS
>        free page
> 
> Fix that race by pulling the locking from __unmap_hugepage_final_range
> into helper functions called from zap_page_range_single. This ensures
> page faults stay locked out of the MADV_DONTNEED VMA until the
> huge pages have actually been freed.
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> ---
>  include/linux/hugetlb.h | 35 +++++++++++++++++++++++++++++++++--
>  mm/hugetlb.c            | 20 +++++++++++---------
>  mm/memory.c             |  7 +++----
>  3 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 694928fa06a3..d9ec500cfef9 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -139,7 +139,7 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
>  void unmap_hugepage_range(struct vm_area_struct *,
>  			  unsigned long, unsigned long, struct page *,
>  			  zap_flags_t);
> -void __unmap_hugepage_range_final(struct mmu_gather *tlb,
> +void __unmap_hugepage_range(struct mmu_gather *tlb,
>  			  struct vm_area_struct *vma,
>  			  unsigned long start, unsigned long end,
>  			  struct page *ref_page, zap_flags_t zap_flags);
> @@ -246,6 +246,25 @@ int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
>  void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
>  				unsigned long *start, unsigned long *end);
>  
> +extern void __hugetlb_zap_begin(struct vm_area_struct *vma,
> +				unsigned long *begin, unsigned long *end);
> +extern void __hugetlb_zap_end(struct vm_area_struct *vma,
> +			      struct zap_details *details);
> +
> +static inline void hugetlb_zap_begin(struct vm_area_struct *vma,
> +				     unsigned long *start, unsigned long *end)
> +{
> +	if (is_vm_hugetlb_page(vma))
> +		__hugetlb_zap_begin(vma, start, end);
> +}
> +
> +static inline void hugetlb_zap_end(struct vm_area_struct *vma,
> +				   struct zap_details *details)
> +{
> +	if (is_vm_hugetlb_page(vma))
> +		__hugetlb_zap_end(vma, details);
> +}
> +
>  void hugetlb_vma_lock_read(struct vm_area_struct *vma);
>  void hugetlb_vma_unlock_read(struct vm_area_struct *vma);
>  void hugetlb_vma_lock_write(struct vm_area_struct *vma);
> @@ -297,6 +316,18 @@ static inline void adjust_range_if_pmd_sharing_possible(
>  {
>  }
>  
> +static inline void hugetlb_zap_begin(
> +				struct vm_area_struct *vma,
> +				unsigned long *start, unsigned long *end)
> +{
> +}
> +
> +static inline void hugetlb_zap_end(
> +				struct vm_area_struct *vma,
> +				struct zap_details *details)
> +{
> +}
> +
>  static inline struct page *hugetlb_follow_page_mask(
>      struct vm_area_struct *vma, unsigned long address, unsigned int flags,
>      unsigned int *page_mask)
> @@ -442,7 +473,7 @@ static inline long hugetlb_change_protection(
>  	return 0;
>  }
>  
> -static inline void __unmap_hugepage_range_final(struct mmu_gather *tlb,
> +static inline void __unmap_hugepage_range(struct mmu_gather *tlb,
>  			struct vm_area_struct *vma, unsigned long start,
>  			unsigned long end, struct page *ref_page,
>  			zap_flags_t zap_flags)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index e859fba5bc7d..5f8b82e902a8 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5306,9 +5306,9 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>  	return len + old_addr - old_end;
>  }
>  
> -static void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
> -				   unsigned long start, unsigned long end,
> -				   struct page *ref_page, zap_flags_t zap_flags)
> +void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
> +			    unsigned long start, unsigned long end,
> +			    struct page *ref_page, zap_flags_t zap_flags)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
>  	unsigned long address;
> @@ -5435,16 +5435,18 @@ static void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct
>  		tlb_flush_mmu_tlbonly(tlb);
>  }
>  
> -void __unmap_hugepage_range_final(struct mmu_gather *tlb,
> -			  struct vm_area_struct *vma, unsigned long start,
> -			  unsigned long end, struct page *ref_page,
> -			  zap_flags_t zap_flags)
> +void __hugetlb_zap_begin(struct vm_area_struct *vma,
> +			 unsigned long *start, unsigned long *end)
>  {
> +	adjust_range_if_pmd_sharing_possible(vma, start, end);
>  	hugetlb_vma_lock_write(vma);
>  	i_mmap_lock_write(vma->vm_file->f_mapping);
> +}

__unmap_hugepage_range_final() was called from unmap_single_vma.
unmap_single_vma has two callers, zap_page_range_single and unmap_vmas.

When the locking was moved into hugetlb_zap_begin, it was only added to the
zap_page_range_single call path.  Calls from unmap_vmas are missing the
locking.
-- 
Mike Kravetz


>  
> -	/* mmu notification performed in caller */
> -	__unmap_hugepage_range(tlb, vma, start, end, ref_page, zap_flags);
> +void __hugetlb_zap_end(struct vm_area_struct *vma,
> +		       struct zap_details *details)
> +{
> +	zap_flags_t zap_flags = details ? details->zap_flags : 0;
>  
>  	if (zap_flags & ZAP_FLAG_UNMAP) {	/* final unmap */
>  		/*
> diff --git a/mm/memory.c b/mm/memory.c
> index 6c264d2f969c..a07ae3b60530 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1683,7 +1683,7 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>  			if (vma->vm_file) {
>  				zap_flags_t zap_flags = details ?
>  				    details->zap_flags : 0;
> -				__unmap_hugepage_range_final(tlb, vma, start, end,
> +				__unmap_hugepage_range(tlb, vma, start, end,
>  							     NULL, zap_flags);
>  			}
>  		} else
> @@ -1753,9 +1753,7 @@ void zap_page_range_single(struct vm_area_struct *vma, unsigned long address,
>  	lru_add_drain();
>  	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>  				address, end);
> -	if (is_vm_hugetlb_page(vma))
> -		adjust_range_if_pmd_sharing_possible(vma, &range.start,
> -						     &range.end);
> +	hugetlb_zap_begin(vma, &range.start, &range.end);
>  	tlb_gather_mmu(&tlb, vma->vm_mm);
>  	update_hiwater_rss(vma->vm_mm);
>  	mmu_notifier_invalidate_range_start(&range);
> @@ -1766,6 +1764,7 @@ void zap_page_range_single(struct vm_area_struct *vma, unsigned long address,
>  	unmap_single_vma(&tlb, vma, address, end, details, false);
>  	mmu_notifier_invalidate_range_end(&range);
>  	tlb_finish_mmu(&tlb);
> +	hugetlb_zap_end(vma, details);
>  }
>  
>  /**
> -- 
> 2.41.0
>