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: <768394b0-41ad-66e6-feda-75bc3243dcf6@nvidia.com>
Date:   Wed, 13 Oct 2021 14:58:16 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH 1/1] gup: document and work around "COW can break either
 way" issue
Content-Language: en-US
To:     Suren Baghdasaryan <surenb@google.com>, stable@vger.kernel.org
Cc:     gregkh@linuxfoundation.org, jannh@google.com,
        torvalds@linux-foundation.org, vbabka@suse.cz, peterx@redhat.com,
        aarcange@redhat.com, david@redhat.com, jgg@ziepe.ca,
        ktkhai@virtuozzo.com, shli@fb.com, namit@vmware.com, hch@lst.de,
        oleg@redhat.com, kirill@shutemov.name, jack@suse.cz,
        willy@infradead.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, kernel-team@android.com
References: <20211012015244.693594-1-surenb@google.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20211012015244.693594-1-surenb@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OW10WUdCNEhMbnRmaTYrdWhEaU5DR04rWHB6VTlDSUhwS2VQY3pYUVRJRDNH?=
 =?utf-8?B?QldhK3l1Y0xOY2tZWERvVHdWYWFzSExJTEpET25tUzY5ZmtVQ0hzMkNBcC9S?=
 =?utf-8?B?c3J4cnMrL1JMQm5DNmI5Q0xDUkc2SnU3ZWNZWVNTeHBqZjQzUU1BTmdhRjhW?=
 =?utf-8?B?a2REcEpHMVJqclpTRXRSb3N2RnRQY2lqaU9vMjNRcEJyMWxjY0l1MXdXLytP?=
 =?utf-8?B?QkpMS01GMGw4dEFvelBCN0RPNHlqV0pucHduUjE0MlZPc0hTYi9ZV0VleWh5?=
 =?utf-8?B?QURHN21aUDV1VVZ5K0VTSnJtYUJMUTZYQXRLUGlTOTVUOGI5SjZxUklVTHlm?=
 =?utf-8?B?UGdhUjRBZEVEaU9XbDU3b3V6L1V4djZHcVJkL3gzbktUTC9kRU5FRmppelQ3?=
 =?utf-8?B?UnBQL3ZoTEJXeFFTTW90c2xmcmZ3NWw4SzFHMVhncUw4LzJCSFVVV0ZRdHNR?=
 =?utf-8?B?aFdta1RhR1R2aTk1RytXSkI2SkpzSjhleEZlS1ZUNlZYMFZuMTcxejN1OUZI?=
 =?utf-8?B?c0hyUVd4K3VIUThIVGxBcTEzMzVPeThWSTN0S05teHNoaDY2UUk0VU1JUHhY?=
 =?utf-8?B?QjV4RG9hMGxtbElsRHhpVG5QN3FocCtOK2hSbThrNWV6VWdYdVpKaEo0VFpD?=
 =?utf-8?B?aFpiRWtUNlJjWmdYaUxmRHh5RUliZVV6RnBTaFcxMVI1eTBkL3lGV2t2YkV6?=
 =?utf-8?B?d1E1U3JsU1BVL3pJMVgvQjVTOStlcURoM05vczhwS296U3I1UDFRY1VKSU5R?=
 =?utf-8?B?dXN1QjF1S3RYTGwxamFwZzBBTXpFbEN4aTB6UWpZeFJqZjBNTHRtYkY3VjVK?=
 =?utf-8?B?T0pRYkVIR055K1NQbE9oMFYvV3crT3ltQ3lya0tIRXNMYzhEcWJjeEtsdHRV?=
 =?utf-8?B?REQwNnBvTS84b0tDSkh3cWx4bERsZmJTc3huMGJWejd3bWFYSzM5WmRSOVJS?=
 =?utf-8?B?NVhpV3g0OFF6QUVKZ3p3Y0Z5MnVqbXFraFdIUndyOGVuSFBKTGlJT20yRUZF?=
 =?utf-8?B?RXQzeWp4TFNWSWQ1cG5GaEFIZmJZNVl1MStPZU5pZXNSSDlZdDlWU21uUXIz?=
 =?utf-8?B?bVZIWElZQk1kb09SeFpzVHZRYUlDeEt0dklOK3lJYlJvV3ZDRDJxbG93aEV5?=
 =?utf-8?B?NThYOTFTbE5HUDZQa3M1WXZlSGZHSW51eTJxR0dFQjh4cVNqdGlvQUJvUVpN?=
 =?utf-8?B?aHAxM1lxSkZ1ZGRGME0za3lWOUE5eHhiTkpUcWRqbGRheWh2TTZiK29jYjNh?=
 =?utf-8?B?QnJ3b2Q5Zkc0SmgwbFVSR0xTTE84OTNKVTBRdEdXVlRPdE9COFVOZTl1N3Bn?=
 =?utf-8?B?d3hCK3FtNzE5NGFEMWJ6VnoranBhZUcxUS9wSVdKdzJ0TC9yb3dYOVM3cHFx?=
 =?utf-8?B?SWNRMytSMDhvTHRjYzk2ZDRNbHEvaXNqRzlSYSt2RHEwcVFyY0xiOUNZZHhQ?=
 =?utf-8?B?MURJQVdiRlprZ09LaG5rNjUrZHlZbDVYckFRNk1UbVJKMDBCZ1VzTTZpZTQy?=
 =?utf-8?B?NFMyOWpRTTdwWDM0QnllaFV3YTdpU3ZzVkhySUJKZ2RwOHR1NWFjNGNob3FQ?=
 =?utf-8?B?aXlTcW9jTm5ONkNMMHRxTG5vTEIyZW04Tk1PL2xEc3FOVi84NnFHWSsyQW9h?=
 =?utf-8?B?MmMvVFVxR0dyeWlVaS9oY0hVUk8vU1gyTmNwSjlJQmYvNG5DMlcxTkg5V1di?=
 =?utf-8?B?NGZ3YUZ5SlUxYmdQOExVa0t1NlcxMTdsNWlLQlAxTkJacVdzdjBnQWd5L1FO?=
 =?utf-8?Q?64b1/02+RbZVAB36FWj0vMmcfLCXGtcdRWyxuNh?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c6d09040-1030-4f43-04f9-08d98e949082
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2021 21:58:18.3366
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: H3ki+H/WLZPHE2WLoIIRD0EEJd6VbYoPSsl7vWz2fsPTbPakgszZGUXg0BVC4LxF+CCtqhIfiJIXEcT/5KSdAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2982
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/11/21 18:52, Suren Baghdasaryan wrote:
...
> [surenb: backport notes
>          Since gup_pgd_range does not exist, made appropriate changes on
>          the the gup_huge_pgd, gup_huge_pd and gup_pud_range calls instead.
> 	Replaced (gup_flags | FOLL_WRITE) with write=1 in gup_huge_pgd,
>          gup_huge_pd and gup_pud_range.
> 	Removed FOLL_PIN usage in should_force_cow_break since it's missing in
> 	the earlier kernels.]
> 

This backport looks accurate. At first I thought you missed the comment-only
change to i915_gem_userptr.c, and then I noticed that the older branch still
uses non-fast gup, so not applicable there after all. :)

Agree with others that this whole area is still shaky, but it does sound as if
this will help.


Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

> Reported-by: Jann Horn <jannh@google.com>
> Tested-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Kirill Shutemov <kirill@shutemov.name>
> Acked-by: Jan Kara <jack@suse.cz>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [surenb: backport to 4.4 kernel]
> Cc: stable@vger.kernel.org # 4.4.x
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>   mm/gup.c         | 48 ++++++++++++++++++++++++++++++++++++++++--------
>   mm/huge_memory.c |  7 +++----
>   2 files changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 4c5857889e9d..c80cdc408228 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -59,13 +59,22 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address,
>   }
>   
>   /*
> - * FOLL_FORCE can write to even unwritable pte's, but only
> - * after we've gone through a COW cycle and they are dirty.
> + * FOLL_FORCE or a forced COW break can write even to unwritable pte's,
> + * but only after we've gone through a COW cycle and they are dirty.
>    */
>   static inline bool can_follow_write_pte(pte_t pte, unsigned int flags)
>   {
> -	return pte_write(pte) ||
> -		((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_dirty(pte));
> +	return pte_write(pte) || ((flags & FOLL_COW) && pte_dirty(pte));
> +}
> +
> +/*
> + * A (separate) COW fault might break the page the other way and
> + * get_user_pages() would return the page from what is now the wrong
> + * VM. So we need to force a COW break at GUP time even for reads.
> + */
> +static inline bool should_force_cow_break(struct vm_area_struct *vma, unsigned int flags)
> +{
> +	return is_cow_mapping(vma->vm_flags) && (flags & FOLL_GET);
>   }
>   
>   static struct page *follow_page_pte(struct vm_area_struct *vma,
> @@ -509,12 +518,18 @@ long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
>   			if (!vma || check_vma_flags(vma, gup_flags))
>   				return i ? : -EFAULT;
>   			if (is_vm_hugetlb_page(vma)) {
> +				if (should_force_cow_break(vma, foll_flags))
> +					foll_flags |= FOLL_WRITE;
>   				i = follow_hugetlb_page(mm, vma, pages, vmas,
>   						&start, &nr_pages, i,
> -						gup_flags);
> +						foll_flags);
>   				continue;
>   			}
>   		}
> +
> +		if (should_force_cow_break(vma, foll_flags))
> +			foll_flags |= FOLL_WRITE;
> +
>   retry:
>   		/*
>   		 * If we have a pending SIGKILL, don't keep faulting pages and
> @@ -1346,6 +1361,10 @@ static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end,
>   /*
>    * Like get_user_pages_fast() except it's IRQ-safe in that it won't fall back to
>    * the regular GUP. It will only return non-negative values.
> + *
> + * Careful, careful! COW breaking can go either way, so a non-write
> + * access can get ambiguous page results. If you call this function without
> + * 'write' set, you'd better be sure that you're ok with that ambiguity.
>    */
>   int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>   			  struct page **pages)
> @@ -1375,6 +1394,12 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>   	 *
>   	 * We do not adopt an rcu_read_lock(.) here as we also want to
>   	 * block IPIs that come from THPs splitting.
> +	 *
> +	 * NOTE! We allow read-only gup_fast() here, but you'd better be
> +	 * careful about possible COW pages. You'll get _a_ COW page, but
> +	 * not necessarily the one you intended to get depending on what
> +	 * COW event happens after this. COW may break the page copy in a
> +	 * random direction.
>   	 */
>   
>   	local_irq_save(flags);
> @@ -1385,15 +1410,22 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>   		next = pgd_addr_end(addr, end);
>   		if (pgd_none(pgd))
>   			break;
> +		/*
> +		 * The FAST_GUP case requires FOLL_WRITE even for pure reads,
> +		 * because get_user_pages() may need to cause an early COW in
> +		 * order to avoid confusing the normal COW routines. So only
> +		 * targets that are already writable are safe to do by just
> +		 * looking at the page tables.
> +		 */
>   		if (unlikely(pgd_huge(pgd))) {
> -			if (!gup_huge_pgd(pgd, pgdp, addr, next, write,
> +			if (!gup_huge_pgd(pgd, pgdp, addr, next, 1,
>   					  pages, &nr))
>   				break;
>   		} else if (unlikely(is_hugepd(__hugepd(pgd_val(pgd))))) {
>   			if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr,
> -					 PGDIR_SHIFT, next, write, pages, &nr))
> +					 PGDIR_SHIFT, next, 1, pages, &nr))
>   				break;
> -		} else if (!gup_pud_range(pgd, addr, next, write, pages, &nr))
> +		} else if (!gup_pud_range(pgd, addr, next, 1, pages, &nr))
>   			break;
>   	} while (pgdp++, addr = next, addr != end);
>   	local_irq_restore(flags);
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 6404e4fcb4ed..fae45c56e2ee 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1268,13 +1268,12 @@ out_unlock:
>   }
>   
>   /*
> - * FOLL_FORCE can write to even unwritable pmd's, but only
> - * after we've gone through a COW cycle and they are dirty.
> + * FOLL_FORCE or a forced COW break can write even to unwritable pmd's,
> + * but only after we've gone through a COW cycle and they are dirty.
>    */
>   static inline bool can_follow_write_pmd(pmd_t pmd, unsigned int flags)
>   {
> -	return pmd_write(pmd) ||
> -	       ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pmd_dirty(pmd));
> +	return pmd_write(pmd) || ((flags & FOLL_COW) && pmd_dirty(pmd));
>   }
>   
>   struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
>