Re: [PATCH] mm/vmalloc: Fix regression caused by needless vmalloc_sync_all()

Re: [PATCH] mm/vmalloc: Fix regression caused by needless vmalloc_sync_all()

[Shile Zhang]

Fix wrong cc list.

On 2019/11/14 05:12, Andrew Morton wrote:
> On Wed, 13 Nov 2019 17:55:30 +0800 Shile Zhang <shile.zhang@linux.alibaba.com> wrote:
>
>> vmalloc_sync_all() was put in the common path in
>> __purge_vmap_area_lazy(), for one sync issue only happened on X86_32
>> with PTI enabled. It is needless for X86_64, which caused a big regression
>> in UnixBench Shell8 testing on X86_64.
>> Similar regression also reported by 0-day kernel test robot in reaim
>> benchmarking:
>> https://lists.01.org/hyperkitty/list/lkp@lists.01.org/thread/4D3JPPHBNOSPFK2KEPC6KGKS6J25AIDB/
> That is indeed a large performance regression.
>
>> Fix it by adding more conditions.
>>
>> Fixes: 3f8fd02b1bf1 ("mm/vmalloc: Sync unmappings in __purge_vmap_area_lazy()")
>>
>> ...
>>
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -1255,11 +1255,17 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>>   	if (unlikely(valist == NULL))
>>   		return false;
>>   
>> +#if defined(CONFIG_X86_32) && defined(CONFIG_X86_PAE)
> Are we sure that x86_32 is the only architecture whcih is (or ever will
> be) affected?
>
>>   	/*
>>   	 * First make sure the mappings are removed from all page-tables
>>   	 * before they are freed.
>> +	 *
>> +	 * This is only needed on x86-32 with !SHARED_KERNEL_PMD, which is
>> +	 * the case on a PAE kernel with PTI enabled.
>>   	 */
>> -	vmalloc_sync_all();
>> +	if (!SHARED_KERNEL_PMD && boot_cpu_has(X86_FEATURE_PTI))
>> +		vmalloc_sync_all();
>> +#endif
>>   
>>   	/*
>>   	 * TODO: to calculate a flush range without looping.
> CONFIG_X86_PAE depends on CONFIG_X86_32 so no need to check
> CONFIG_X86_32?

Yes, Thanks for your review and kindly refactoring!

> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm-vmalloc-fix-regression-caused-by-needless-vmalloc_sync_all-fix
>
> simplify config expression, use IS_ENABLED()
>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Qian Cai <cai@lca.pw>
> Cc: Shile Zhang <shile.zhang@linux.alibaba.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>   mm/vmalloc.c |   21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> --- a/mm/vmalloc.c~mm-vmalloc-fix-regression-caused-by-needless-vmalloc_sync_all-fix
> +++ a/mm/vmalloc.c
> @@ -1255,16 +1255,17 @@ static bool __purge_vmap_area_lazy(unsig
>   	if (unlikely(valist == NULL))
>   		return false;
>   
> -#if defined(CONFIG_X86_32) && defined(CONFIG_X86_PAE)
> -	/*
> -	 * First make sure the mappings are removed from all page-tables
> -	 * before they are freed.
> -	 *
> -	 * This is only needed on x86-32 with !SHARED_KERNEL_PMD, which is
> -	 * the case on a PAE kernel with PTI enabled.
> -	 */
> -	if (!SHARED_KERNEL_PMD && boot_cpu_has(X86_FEATURE_PTI))
> -		vmalloc_sync_all();
> +	if (IS_ENABLED(CONFIG_X86_PAE)) {
> +		/*
> +		 * First make sure the mappings are removed from all page-tables
> +		 * before they are freed.
> +		 *
> +		 * This is only needed on x86-32 with !SHARED_KERNEL_PMD, which
> +		 * is the case on a PAE kernel with PTI enabled.
> +		 */
> +		if (!SHARED_KERNEL_PMD && boot_cpu_has(X86_FEATURE_PTI))
> +			vmalloc_sync_all();
> +	}
>   #endif
>   
>   	/*
> _

Re: [PATCH] mm/vmalloc: Fix regression caused by needless vmalloc_sync_all()

[Shile Zhang]

On 2019/11/14 21:56, Michal Hocko wrote:
> On Wed 13-11-19 17:55:30, Shile Zhang wrote:
>> vmalloc_sync_all() was put in the common path in
>> __purge_vmap_area_lazy(), for one sync issue only happened on X86_32
>> with PTI enabled. It is needless for X86_64, which caused a big regression
>> in UnixBench Shell8 testing on X86_64.
>> Similar regression also reported by 0-day kernel test robot in reaim
>> benchmarking:
>> https://lists.01.org/hyperkitty/list/lkp@lists.01.org/thread/4D3JPPHBNOSPFK2KEPC6KGKS6J25AIDB/
>>
>> Fix it by adding more conditions.
> This doesn't really explain a lot. Could you explain why the syncing
> should be limited only to PAE and !SHARED_KERNEL_PMD?

Thanks for your review!
Sorry for that, I'm not clear about the original issue the patch
3f8fd02b1bf1 ("mm/vmalloc: Sync unmappings in __purge_vmap_area_lazy()")
fixed.

I just get that limitation info from the commit log as following:
"
This is only needed x86-32 with !SHARED_KERNEL_PMD, which is the case on 
a PAE kernel with PTI enabled. On affected systems the missing sync 
causes old mappings to persist in some page-tables, causing data 
corruption and other undefined behavior.
"
https://patchwork.kernel.org/cover/11050511/

Hi, Joerg
Could you please help to recall the original issue you encountered before?
Thanks!

>
>> Fixes: 3f8fd02b1bf1 ("mm/vmalloc: Sync unmappings in __purge_vmap_area_lazy()")
>>
>> Signed-off-by: Shile Zhang <shile.zhang@linux.alibaba.com>
>> ---
>>   mm/vmalloc.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index a3c70e275f4e..7b9fc7966da6 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -1255,11 +1255,17 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>>   	if (unlikely(valist == NULL))
>>   		return false;
>>   
>> +#if defined(CONFIG_X86_32) && defined(CONFIG_X86_PAE)
>>   	/*
>>   	 * First make sure the mappings are removed from all page-tables
>>   	 * before they are freed.
>> +	 *
>> +	 * This is only needed on x86-32 with !SHARED_KERNEL_PMD, which is
>> +	 * the case on a PAE kernel with PTI enabled.
>>   	 */
>> -	vmalloc_sync_all();
>> +	if (!SHARED_KERNEL_PMD && boot_cpu_has(X86_FEATURE_PTI))
>> +		vmalloc_sync_all();
>> +#endif
>>   
>>   	/*
>>   	 * TODO: to calculate a flush range without looping.
>> -- 
>> 2.24.0.rc2
>>

Re: [PATCH] mm/vmalloc: Fix regression caused by needless vmalloc_sync_all()

[Joerg Roedel]

On Thu, Nov 14, 2019 at 10:40:22PM +0800, Shile Zhang wrote:
> Could you please help to recall the original issue you encountered before?

The original issue was data corruption because old mappings did not get
removed from the vmalloc area, and thus also new mappings did not get
faulted in. So depending on the page-table currently loaded one
vmalloc/ioremap area pointed to different (often already freed or
re-used) memory and caused the corruption.

Regards,

	Joerg

Re: [PATCH] mm/vmalloc: Fix regression caused by needless vmalloc_sync_all()

[Shile Zhang]

On 2019/11/15 01:12, Joerg Roedel wrote:
> On Wed, Nov 13, 2019 at 05:55:30PM +0800, Shile Zhang wrote:
>> +#if defined(CONFIG_X86_32) && defined(CONFIG_X86_PAE)
>>   	/*
>>   	 * First make sure the mappings are removed from all page-tables
>>   	 * before they are freed.
>> +	 *
>> +	 * This is only needed on x86-32 with !SHARED_KERNEL_PMD, which is
>> +	 * the case on a PAE kernel with PTI enabled.
>>   	 */
>> -	vmalloc_sync_all();
>> +	if (!SHARED_KERNEL_PMD && boot_cpu_has(X86_FEATURE_PTI))
>> +		vmalloc_sync_all();
>> +#endif
> I already submitted another fix for this quite some time ago:
>
> 	https://lore.kernel.org/lkml/20191009124418.8286-1-joro@8bytes.org/
>
> Regards,
>
> 	Joerg
Oh, sorry for I missed that, good job and thanks for your work!