Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3ca19dec-5ded-02ab-184c-89aba4052aca@oracle.com>
Date:   Wed, 26 Jan 2022 15:26:55 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 2/2] mm: fix missing cache flush for all tail pages of
 THP
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     Zi Yan <ziy@nvidia.com>, David Rientjes <rientjes@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Xiongchun duan <duanxiongchun@bytedance.com>,
        Lars Persson <lars.persson@axis.com>
References: <20220124051752.83281-1-songmuchun@bytedance.com>
 <20220124051752.83281-2-songmuchun@bytedance.com>
 <4d5044e7-cac9-b6e6-1467-59ea6010b0f5@google.com>
 <5D9B52E1-A74B-4964-AACF-ADB91536C4C0@nvidia.com>
 <CAMZfGtXsLyagpo8rM6RmayAFR_hgk0987X1usxYWRZLeA5H45Q@mail.gmail.com>
 <7D7EB27F-DEA7-41AA-B24E-B61A2A1A5F07@nvidia.com>
 <CAMZfGtVCUbTa6xv2R9x+_a5_dK4ZLUX8ECNkF8mkJgEy4kdnfQ@mail.gmail.com>
 <039a9107-756e-bc0a-6e72-fbe08408de38@oracle.com>
 <CAMZfGtXigcVQincXZ6kdNRY7GRwR6E=RD3-pGaiymv87ynoOqQ@mail.gmail.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <CAMZfGtXigcVQincXZ6kdNRY7GRwR6E=RD3-pGaiymv87ynoOqQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VndnZG1FQTJ2UmtYN1BJYUhXajJuK1BYMzJ4VTNQL2N2blZIbkZBalViSDIz?=
 =?utf-8?B?bGNmSDU2VmlJcDVGWXBRZ0pNQU1ZQ054TjlaeTdBVm40KzlpbHZzdlR1RWo3?=
 =?utf-8?B?NjE2MVRVOTcwUmVGVzhPT2FBeThQcFV2TU5PTCtCbWoySG5TblZ1bEpMZ1hV?=
 =?utf-8?B?dUErS2krVVhUSjc4eG9qbzRuYjlWRGRGUytUeDRUaWdjRk5PekJmSGNjV3FG?=
 =?utf-8?B?TkxaR0daWVJMSXdCTkpXTnVVclB1dWVYa3I5WC80aGhSYzB5QmhZTXBmMXQz?=
 =?utf-8?B?Zzl3dXpwS2k1cnpHN0R0U1ZNQjFSYmJFU2FpMlliZGVPekZqeFR2MGhoY29U?=
 =?utf-8?B?N3R0Z1hlTjBBZmd6K01QYnFpalVLalpZOHc5K01jNFcwQ2FmOFJTZDd3bWIr?=
 =?utf-8?B?RkNzVHZ2R2ZLTWxWTmsrVUZZa2k3YjYvYWMweGxoRFNzK0cvK0JGZjVldXBn?=
 =?utf-8?B?UDJpV1lTbW5vbVNvSW1MNCtLc05WaDZsK3VCMkFPZ0dYQTgxS2hGc0V5TWpo?=
 =?utf-8?B?NXpFYlp6R3RkN3NRSm9TK0pNcGpIOGhXRHJpMkpwUzlXajNzTC9JVk5KUitY?=
 =?utf-8?B?QUFBR1g4WWxHN3FLQ0tnQlVPakNxeVdKTzltcXJkZW5GWE5EVUd1bGFZdEFN?=
 =?utf-8?B?c0lGSXlDWnlCYVUwMWpMbUdRYkFjU0dRRFM2eWFIUWZKTnl1ZXFjYlR1VUkr?=
 =?utf-8?B?TFhFYVVkMkZJUVQwQTU4K21JVnp4cmovVmllNm11ZFAybkNEbm4yUFRKakdY?=
 =?utf-8?B?cFhSV3p1STR1dmtaV3g1aXVSQ2ZlSzJDVnc5dVdzcEs5M1UzajNjV2FXdWpl?=
 =?utf-8?B?Zi9QVlhYREJ1bmdQdkljeDRra2pkelJUSUh0TEV2NjZoN2RZU3NoSW1jblU5?=
 =?utf-8?B?OUREbkg4cEt6UzdRVTJib0N5bjd3dWd3MjhJTEphOEEwVjlndURJQVhXNW5W?=
 =?utf-8?B?TEZFdldGZlo0YWpXR05iRFIwMkdYb3N2QWV0L09jMGhjVlFLSzVQSEVKaURz?=
 =?utf-8?B?cnNxRnVUcEhCcHk5d0w1T0d5NXJDNjEzT1BKTkhMWWU1ZTBQWmZsMVFHNjJz?=
 =?utf-8?B?Ump0cUhYUWpwSU82WlE4eDlwOWRSSmRFMU9xOU5rUmtuVmxDNlJKRkVYdUY4?=
 =?utf-8?B?VnZpd0haR3NvRjhVWVM4SmRUT1NzQWZmRXQ2aEMvaDFjeUdYekJ6OXdKc1Fx?=
 =?utf-8?B?RjdDWmNsZXJtV1FDZlNwOTAwelB1cjBRUVh6cFI0bkxiZDU0SFUrV1I0WTRP?=
 =?utf-8?B?S2lIa3AyRjhwNlkrbVY0N0VzZldPWDJyL1ZvY2ZPT1JZc3VsQm5VTlpaT0dq?=
 =?utf-8?B?MXR2bGRMRTMvYVl5bFQrYVoxdXhUWk16ZnVTZnd0cm83UUVwV09QNEVFMUpv?=
 =?utf-8?B?SzBoRzFMQVlvdlViVitqY2ZGc0dCV0dqS3ZCbTR6WTFzVmJHM1hYc2t6WXBm?=
 =?utf-8?B?bWo2bG53c2E0dlJvYVFsMTNINURkTzZKOEY5eW4zOFROQ2VCK3hVQUN1anpQ?=
 =?utf-8?B?L256Z3EzZ1RUQ0FBMi9vVmluaDdiYVdjVU5sWDZrUy9nTDY5aUR3WDJzYmFq?=
 =?utf-8?B?UTBaeWkxN2JvYmdzV3dNZ244MG1QTE55ZytPdXVUaEV4Q2pyZ3ZrZXlCZ1pS?=
 =?utf-8?B?clZ1SERKc00xMTZ1WjVCc0tITHdZNDVFZU52QXUzRmxOS3VTNVJidXN4ZDJW?=
 =?utf-8?B?NlkzRVN5eDNmWnJxSHJUMTVrUnpSRlhyM1FRVGVUUkdDRnUyNVppeHNncTB2?=
 =?utf-8?B?SHpLUXNBTjFrVlpLdTZmY0phZzhZSFJXMXJoU0tvMndXMVJWQSthUVl2Vm1C?=
 =?utf-8?B?NWVyVzRIK0JoYXpSNkE3RHFEcW1MM2JGdThCWUpWNXlOK0hRYVJhN0NJSTJO?=
 =?utf-8?B?dUVXZEs4Rlh4NGxKaUVVODRaanV6TEJjeUVZRUdRUkh4bE1LSWJNcFlPeGNu?=
 =?utf-8?B?aUhxVnk3NngzWFFIa2Y0dUV3Um5sdVU1Vk90bExOMDNrcGVPcHhYKzZSVmYx?=
 =?utf-8?B?UTdDMTR2Q2c5aEhmUVlISTNmODBEbWRkdFdvYm1UQmxGVjd4WDlsYTE3WUxo?=
 =?utf-8?B?Y1h3MkNNdy9HS2t1bENiakl4dXdwaFByWWpuTnhDclBybnBUQm1MUTBydWF5?=
 =?utf-8?B?SEgrZkhxTzUvMUVOSW42Qm53OEhaU3hpTXJGYTcxWHZaa1BGaFZvMU9tSjZ3?=
 =?utf-8?Q?pBlorMfvAzCAgmioekdmT5w=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e777fbad-ad87-4179-4354-08d9e1235963
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2022 23:26:59.2386
 (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: CmMQvriz6XUXTMdNxDD8KkcYI/5AFc3te9h1XjlOWe3t2s5S9NFExK+m7HAz85aqieoycQOE9C56Pt2KVskcsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1296
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10239 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 bulkscore=0
 suspectscore=0 malwarescore=0 adultscore=0 mlxlogscore=999 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2201260132
X-Proofpoint-GUID: UArRA98dg2lQFuvpRbZkj1K0nWzjxvWn
X-Proofpoint-ORIG-GUID: UArRA98dg2lQFuvpRbZkj1K0nWzjxvWn
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/25/22 19:29, Muchun Song wrote:
> On Wed, Jan 26, 2022 at 5:24 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>
>> On 1/24/22 22:01, Muchun Song wrote:
>>> On Tue, Jan 25, 2022 at 10:42 AM Zi Yan <ziy@nvidia.com> wrote:
>>>>
>>>> On 24 Jan 2022, at 20:55, Muchun Song wrote:
>>>>
>>>>> On Tue, Jan 25, 2022 at 3:22 AM Zi Yan <ziy@nvidia.com> wrote:
>>>>>>
>>>>>> On 24 Jan 2022, at 13:11, David Rientjes wrote:
>>>>>>
>>>>>>> On Mon, 24 Jan 2022, Muchun Song wrote:
>>>>>>>
>>>>>>>> The D-cache maintenance inside move_to_new_page() only consider one page,
>>>>>>>> there is still D-cache maintenance issue for tail pages of THP. Fix this
>>>>>>>> by not using flush_dcache_folio() since it is not backportable.
>>>>>>>>
>>>>>>>
>>>>>>> The mention of being backportable suggests that we should backport this,
>>>>>>> likely to 4.14+.  So should it be marked as stable?
>>>>>>
>>>>>> Hmm, after more digging, I am not sure if the bug exists. For THP migration,
>>>>>> flush_cache_range() is used in remove_migration_pmd(). The flush_dcache_page()
>>>>>> was added by Lars Persson (cc’d) to solve the data corruption on MIPS[1],
>>>>>> but THP migration is only enabled on x86_64, PPC_BOOK3S_64, and ARM64.
>>>>>
>>>>> I only mention the THP case. After some more thinking, I think the HugeTLB
>>>>> should also be considered, Right? The HugeTLB is enabled on arm, arm64,
>>>>> mips, parisc, powerpc, riscv, s390 and sh.
>>>>>
>>>>
>>>> +Mike for HugeTLB
>>>>
>>>> If HugeTLB page migration also misses flush_dcache_page() on its tail pages,
>>>> you will need a different patch for the commit introducing hugetlb page migration.
>>>
>>> Agree. I think arm (see the following commit) has handled this issue, while most
>>> others do not.
>>>
>>>   commit 0b19f93351dd ("ARM: mm: Add support for flushing HugeTLB pages.")
>>>
>>> But I do not have any real devices to test if this issue exists on other archs.
>>> In theory, it exists.
>>>
>>
>> Thanks for adding me to the discussion.
>>
>> I agree that this issue exists at least in theory for hugetlb pages as well.
>> This made me look at other places with similar code for hugetlb.  i.e.
>> Allocating a new page, copying data to new page and then establishing a
>> mapping (pte) to the new page.
> 
> Hi Mike,
> 
> Thanks for looking at this.
> 
>>
>> - hugetlb_cow calls copy_user_huge_page() which ends up calling
>>   copy_user_highpage that includes dcache flushing of the target for some
>>   architectures, but not all.
> 
> copy_user_page() inside copy_user_highpage() is already considering
> the cache maintenance on different architectures, which is documented
> in Documentation/core-api/cachetlb.rst. So there are no problems in this
> case.
> 

Thanks!  That cleared up some of my confusion.


>> - userfaultfd calls copy_huge_page_from_user which does not appear to do
>>   any dcache flushing for the target page.
> 
> Right. The new page should be flushed before setting up the mapping
> to the user space.
> 
>> Do you think these code paths have the same potential issue?
> 
> The latter does have the issue, the former does not. The fixes may
> look like the following:
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index a1baa198519a..828240aee3f9 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5819,6 +5819,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>                         goto out;
>                 }
>                 folio_copy(page_folio(page), page_folio(*pagep));
> +               flush_dcache_folio(page_folio(page));
>                 put_page(*pagep);
>                 *pagep = NULL;
>         }
> diff --git a/mm/memory.c b/mm/memory.c
> index e8ce066be5f2..ff6f48cdcc48 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5400,6 +5400,7 @@ long copy_huge_page_from_user(struct page *dst_page,
>                         kunmap(subpage);
>                 else
>                         kunmap_atomic(page_kaddr);
> +               flush_dcache_page(subpage);
> 
>                 ret_val -= (PAGE_SIZE - rc);
>                 if (rc)
> 

That looks good to me.  Do you plan to include this in the next version
of this series?

-- 
Mike Kravetz