Re: [PATCH hotfix 6.12 v4 4/5] mm: refactor arch_calc_vm_flag_bits() and arm64 MTE handling

[Yang Shi <yang@os.amperecomputing.com>]

On 10/30/24 8:08 AM, Lorenzo Stoakes wrote:
> On Wed, Oct 30, 2024 at 07:58:33AM -0700, Yang Shi wrote:
>>
>> On 10/30/24 4:53 AM, Lorenzo Stoakes wrote:
>>> On Wed, Oct 30, 2024 at 12:09:43PM +0100, Vlastimil Babka wrote:
>>>> On 10/30/24 11:58, Catalin Marinas wrote:
>>>>> On Wed, Oct 30, 2024 at 10:18:27AM +0100, Vlastimil Babka wrote:
>>>>>> On 10/29/24 19:11, Lorenzo Stoakes wrote:
>>>>>>> --- a/arch/arm64/include/asm/mman.h
>>>>>>> +++ b/arch/arm64/include/asm/mman.h
>>>>>>> @@ -6,6 +6,8 @@
>>>>>>>
>>>>>>>    #ifndef BUILD_VDSO
>>>>>>>    #include <linux/compiler.h>
>>>>>>> +#include <linux/fs.h>
>>>>>>> +#include <linux/shmem_fs.h>
>>>>>>>    #include <linux/types.h>
>>>>>>>
>>>>>>>    static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
>>>>>>> @@ -31,19 +33,21 @@ static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
>>>>>>>    }
>>>>>>>    #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
>>>>>>>
>>>>>>> -static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags)
>>>>>>> +static inline unsigned long arch_calc_vm_flag_bits(struct file *file,
>>>>>>> +						   unsigned long flags)
>>>>>>>    {
>>>>>>>    	/*
>>>>>>>    	 * Only allow MTE on anonymous mappings as these are guaranteed to be
>>>>>>>    	 * backed by tags-capable memory. The vm_flags may be overridden by a
>>>>>>>    	 * filesystem supporting MTE (RAM-based).
>>>>>> We should also eventually remove the last sentence or even replace it with
>>>>>> its negation, or somebody might try reintroducing the pattern that won't
>>>>>> work anymore (wasn't there such a hugetlbfs thing in -next?).
>>>>> I agree, we should update this comment as well though as a fix this
>>>>> patch is fine for now.
>>>>>
>>>>> There is indeed a hugetlbfs change in -next adding VM_MTE_ALLOWED. It
>>>>> should still work after the above change but we'd need to move it over
>>>> I guess it will work after the above change, but not after 5/5?
>>>>
>>>>> here (and fix the comment at the same time). We'll probably do it around
>>>>> -rc1 or maybe earlier once this fix hits mainline.
>>>> I assume this will hopefully go to rc7.
>>> To be clear - this is a CRITICAL fix that MUST land for 6.12. I'd be inclined to
>>> try to get it to an earlier rc-.
>>>
>>>>> I don't think we have
>>>>> an equivalent of shmem_file() for hugetlbfs, we'll need to figure
>>>>> something out.
>>>> I've found is_file_hugepages(), could work? And while adding the hugetlbfs
>>>> change here, the comment could be adjusted too, right?
>>> Right but the MAP_HUGETLB should work to? Can we save such changes that
>>> alter any kind of existing behaviour to later series?
>> We should need both because mmap hugetlbfs file may not use MAP_HUGETLB.
> Right yeah, we could create a memfd with MFD_HUGETLB for instance and mount
> that...
>
> Perhaps somebody could propose the 6.13 change (as this series is just
> focused on the hotfix)?

Once this series go in rc7, we (me and Catalin) need to rebase hugetlb 
MTE patches anyway due to the conflict. But it should be trivial.

>
> Note that we absolutely plan to try to merge this in 6.12 (it is a critical
> fix for a few separate issues).
>
> I guess since we already have something in the arm64 tree adding
> MAP_HUGETLB we could rebase that and add a is_file_hugepages() there to
> cover off that case too?

Yes

>
> (Though I note that shm_file_operations_huge also sets FOP_HUGE_PAGES which
> this predicate picks up, not sure if we're ok wtih that? But discussion
> better had I think in whichever thread this hugetlb change came from
> perhaps?)

It is ok. SHM_HUGETLB uses hugetlbfs actually.

>
> Catalin, perhaps?
>
>>> As this is going to be backported (by me...!) and I don't want to risk
>>> inadvertant changes.
>>>
>>>>>>>    	 */
>>>>>>> -	if (system_supports_mte() && (flags & MAP_ANONYMOUS))
>>>>>>> +	if (system_supports_mte() &&
>>>>>>> +	    ((flags & MAP_ANONYMOUS) || shmem_file(file)))
>>>>>>>    		return VM_MTE_ALLOWED;
>>>>>>>
>>>>>>>    	return 0;
>>>>>>>    }
>>>>> This will conflict with the arm64 for-next/core tree as it's adding
>>>>> a MAP_HUGETLB check. Trivial resolution though, Stephen will handle it.
>>> Thanks!
>>>
> Thanks all!