Received-SPF: pass (google.com: domain of linux-kernel+bounces-62910-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 12 Feb 2024 22:33:07 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Lokesh Gidra <lokeshgidra@google.com>
Cc: akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        selinux@vger.kernel.org, surenb@google.com, kernel-team@android.com,
        aarcange@redhat.com, peterx@redhat.com, david@redhat.com,
        axelrasmussen@google.com, bgeffon@google.com, willy@infradead.org,
        jannh@google.com, kaleshsingh@google.com, ngeoffray@google.com,
        timmurray@google.com, rppt@kernel.org
Subject: Re: [PATCH v5 3/3] userfaultfd: use per-vma locks in userfaultfd
 operations
Message-ID: <20240213033307.zbhrpjigco7vl56z@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Lokesh Gidra <lokeshgidra@google.com>, akpm@linux-foundation.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, selinux@vger.kernel.org,
	surenb@google.com, kernel-team@android.com, aarcange@redhat.com,
	peterx@redhat.com, david@redhat.com, axelrasmussen@google.com,
	bgeffon@google.com, willy@infradead.org, jannh@google.com,
	kaleshsingh@google.com, ngeoffray@google.com, timmurray@google.com,
	rppt@kernel.org
References: <20240213001920.3551772-1-lokeshgidra@google.com>
 <20240213001920.3551772-4-lokeshgidra@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240213001920.3551772-4-lokeshgidra@google.com>
User-Agent: NeoMutt/20220429
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?OI7wi9622aXP3xyc0MXu7T3GiewACXMrxUkSK7dqmUb39NJQbQhjNR8pPyxr?=
 =?us-ascii?Q?I4dI4WGKD5mCDo7fMzsOR51NEaqJ6TW7W4Y6H5R5m2esyvN8G2v/JDklQKZn?=
 =?us-ascii?Q?wOeKT0MY2Z6B5zkEw/MWHcd5gmdo0r75pYlQsFqxVv8lTpH3YPsan6aFIZoH?=
 =?us-ascii?Q?nNoiHkSfrZ7cbiPRyeKqQIe9v0tJQ2Ob2PVZHbexsPquTWAKfPzMQqBWVy7t?=
 =?us-ascii?Q?EQtIurMEBAiIg9KSEPjN8HVDLrl+4yFC+CfZlViP/4fvYHp7hafuYZXC8iVl?=
 =?us-ascii?Q?VoKacVuAs/bCwCMu2d8H5rvVBovs2f9mvUUdsCfinBNUwlBwW8GI/fG7hfri?=
 =?us-ascii?Q?qSYco8Uqe9IbdfePkcJPhlGzOg+OFpF6x5tJHhFRpWYpwZtlmezzZhiyv7ma?=
 =?us-ascii?Q?gsG7Hc8tMR2GICVFkGbnu2TctwmjXu/2q9YgAaWRpxxswRBwKdgmvSDzU0QP?=
 =?us-ascii?Q?KLUXruK1dfV/kxYEINvavC8E3zt81uWBnO4UqHBULUoP6/O1+Ah8X5roW0JR?=
 =?us-ascii?Q?ETzrRa9ej/Hy4UH6I47FZBUQyBZFbrZ/q85KCfLPUlvrwtZhKGxVfO6amccB?=
 =?us-ascii?Q?JrHGG9F3TN7xrhCMYd/lpHTU1I8m9+cXjmIfRtb4XtL7PmbS8tR6DZXGEXto?=
 =?us-ascii?Q?5xxg2iHJ9thiVbaC9tT0rzgKM4tTApaEw3UxEqUvkU+QaqsJJT+s9Gi8RwkY?=
 =?us-ascii?Q?Xis9nYu6eiLfzr8eSV255sm6L2KkPStDjmXcOfZWOTzPAzV1Qug1GaR3wdAG?=
 =?us-ascii?Q?pfzCPyEf9YKXB4f15scyiJrXsZAGT3YnfSAPfjpRIFceGIu6odtZGJ7iu8DU?=
 =?us-ascii?Q?kIXsquKZ17Sgc26vHMHHU4Pzq7jUYMxv+y9Zw9w2Aqe5Hj4jeiwQDqHO1doy?=
 =?us-ascii?Q?mYrl/NLiDpdhWHIlFK/hnf+ER4MJeAMFEkh1sdBVq/CZ+P9vqjTXyVJYkqR4?=
 =?us-ascii?Q?WKD8OWYGzHh+Ez9ZSSXLnRp/kFy30AQxYYpd2U5sFylW2ieR6Pcr+6/qSWff?=
 =?us-ascii?Q?0GsJ9wrtP2hE8lXQ9ILIOYoD+ZFzjGL3Z5jCi/KLb2C8r9Sx4Ey1FlopFxkE?=
 =?us-ascii?Q?ZO2G5ZE3R5qwdcR2qOtAKvT/Spom2Dl7z56UuuagDe8QhzcnzAKEM7naULdi?=
 =?us-ascii?Q?DfxF0zlbP2TbLsPgnYKyd4tc4LGw0HhNjC9gZaQ2EMVDsD4DbT768nHDsTvB?=
 =?us-ascii?Q?2lg9UzfFicvOipptC3n7dPR78MzSbrTg9BwS0QJMdf1YGz68kbUcXy5/ooNs?=
 =?us-ascii?Q?TLk8DNuxAAB8Vk/05MzD7//YDJKqDP2Eg1xY2y42KeF2Y5HaX9GJuWqIpfZD?=
 =?us-ascii?Q?6zCGWNp3o86o8yOuYVpyJheaa4fbdtbdWKwoQ2fz1ltnzExn99W1khouUO0e?=
 =?us-ascii?Q?usWHxpQYdl4rswxK1k4iYZwoRjhdYV6crasgkZqPuIBgL3CpukrcaJH/4fIO?=
 =?us-ascii?Q?Yemun3hGesIySCIFG/l4AFrWHD6XdQ9xk0oidVoNmucLjLMwhvtLvAuwC9EH?=
 =?us-ascii?Q?7Z+slTqgVaSuBBbCF7w6oqrhrbJyiHT1ma+EgjgsNBVxDTHFrZYarg327bSS?=
 =?us-ascii?Q?Rga4wX6E/sL8Ej5INND8wioCkkV3YRBmf54NJTE9I+V2xk0y1dNuTansAb5I?=
 =?us-ascii?Q?LA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ywVDlB47Qn+XLELbsi500/cPBMDWr5QhuuiNr0FzmaHmrWuqTixhkKiN+KOeT/gqgOjVvQL8emPceih7G7PEgjscUwvc2ZFiIkJ8lv3jkMOh1OOD1ZgpRa87nhV5M2H2vGVJqnoNVmpxBS26J0Ts1KrL3gsy9anxZ1NVyvjgnkuR3NMwJsl26V096Q+hqp9oPHeYasgs8/2fQN5XSCcPV5XxztR9O4ApzUXsJP5mKttSks5QHNULa0PPs2uZ4gyWZD6TnlTrH8FBmUJKETd9IPCYX1UGCCfvE06p/37u6LJ56mBcAM5IF8wAxlfVO9LAEcxU4NppBsJX0MoaJQMzlOVZ6Ss7Upv6+LfpVSUNT4nK8A0mTOiiLgK47vwNoU4/Jfe5M9nroagHQDrbSVWK9L3wO22AFUS2eGi4sBIvf4KKdz2ZyKpSJkJhErnI6okt5MtAnfIOZjTjVEZHR3Yc1uYeDJyLFjTcYw5jHUTfct8Yh4B8zo+g4vBjwHWlVl/FxEBHn1kv88AqmrrzN3zGeBL4juFxyO+MUUXmexK/HElpjHMw/4l/6SSmBUe1ZWQ3oXevH8PrrWUPsO0fqQT614BEeJjX0Lq+OqCFELRhKLU=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0db9f876-79f0-44ee-a894-08dc2c44801a
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 03:33:10.0003
 (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: iDZB0glLSxCo2lAJsg2LjV/TOULMikzzVgwyxWgy3Kt2uubZwAVa57tNMONki81TajIRXZRts05pzjmB88bH2Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7356
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-12_20,2024-02-12_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 suspectscore=0
 phishscore=0 adultscore=0 spamscore=0 mlxlogscore=480 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402130024
X-Proofpoint-GUID: -eIMHdBhzgIX5EhvBdfki-Fn1spmN2JZ
X-Proofpoint-ORIG-GUID: -eIMHdBhzgIX5EhvBdfki-Fn1spmN2JZ

* Lokesh Gidra <lokeshgidra@google.com> [240212 19:19]:
> All userfaultfd operations, except write-protect, opportunistically use
> per-vma locks to lock vmas. On failure, attempt again inside mmap_lock
> critical section.
> 
> Write-protect operation requires mmap_lock as it iterates over multiple
> vmas.
> 
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
>  fs/userfaultfd.c              |  13 +-
>  include/linux/userfaultfd_k.h |   5 +-
>  mm/userfaultfd.c              | 392 ++++++++++++++++++++++++++--------
>  3 files changed, 312 insertions(+), 98 deletions(-)
> 
..

> +
> +static __always_inline
> +struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
> +						 unsigned long addr)
> +{
> +	struct vm_area_struct *vma;
> +
> +	mmap_assert_locked(mm);
> +	vma = vma_lookup(mm, addr);
> +	if (!vma)
> +		vma = ERR_PTR(-ENOENT);
> +	else if (!(vma->vm_flags & VM_SHARED) && anon_vma_prepare(vma))
> +		vma = ERR_PTR(-ENOMEM);

Nit: I just noticed that the code below says anon_vma_prepare() is unlikely.

..

> +static struct vm_area_struct *lock_mm_and_find_dst_vma(struct mm_struct *dst_mm,
> +						       unsigned long dst_start,
> +						       unsigned long len)
> +{
> +	struct vm_area_struct *dst_vma;
> +	int err;
> +
> +	mmap_read_lock(dst_mm);
> +	dst_vma = find_vma_and_prepare_anon(dst_mm, dst_start);
> +	if (IS_ERR(dst_vma)) {
> +		err = PTR_ERR(dst_vma);

It's sort of odd you decode then re-encode this error, but it's correct
the way you have it written.  You could just encode ENOENT instead?

> +		goto out_unlock;
> +	}
> +
> +	if (validate_dst_vma(dst_vma, dst_start + len))
> +		return dst_vma;
> +
> +	err = -ENOENT;
> +out_unlock:
> +	mmap_read_unlock(dst_mm);
> +	return ERR_PTR(err);
>  }
> +#endif
>  
..

> +static __always_inline
> +long find_vmas_mm_locked(struct mm_struct *mm,

int would probably do?
> +			 unsigned long dst_start,
> +			 unsigned long src_start,
> +			 struct vm_area_struct **dst_vmap,
> +			 struct vm_area_struct **src_vmap)
> +{
> +	struct vm_area_struct *vma;
> +
> +	mmap_assert_locked(mm);
> +	vma = find_vma_and_prepare_anon(mm, dst_start);
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
> +	*dst_vmap = vma;
> +	/* Skip finding src_vma if src_start is in dst_vma */
> +	if (src_start >= vma->vm_start && src_start < vma->vm_end)
> +		goto out_success;
> +
> +	vma = vma_lookup(mm, src_start);
> +	if (!vma)
> +		return -ENOENT;
> +out_success:
> +	*src_vmap = vma;
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PER_VMA_LOCK
> +static long find_and_lock_vmas(struct mm_struct *mm,

This could also be an int return type, I must be missing something?

..

> +	*src_vmap = lock_vma_under_rcu(mm, src_start);
> +	if (likely(*src_vmap))
> +		return 0;
> +
> +	/* Undo any locking and retry in mmap_lock critical section */
> +	vma_end_read(*dst_vmap);
> +
> +	mmap_read_lock(mm);
> +	err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
> +	if (!err) {
> +		/*
> +		 * See comment in lock_vma() as to why not using
> +		 * vma_start_read() here.
> +		 */
> +		down_read(&(*dst_vmap)->vm_lock->lock);
> +		if (*dst_vmap != *src_vmap)
> +			down_read(&(*src_vmap)->vm_lock->lock);
> +	}
> +	mmap_read_unlock(mm);
> +	return err;
> +}
> +#else
> +static long lock_mm_and_find_vmas(struct mm_struct *mm,
> +				  unsigned long dst_start,
> +				  unsigned long src_start,
> +				  struct vm_area_struct **dst_vmap,
> +				  struct vm_area_struct **src_vmap)
> +{
> +	long err;
> +
> +	mmap_read_lock(mm);
> +	err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
> +	if (err)
> +		mmap_read_unlock(mm);
> +	return err;
>  }
> +#endif

This section is much easier to understand.  Thanks.

Thanks,
Liam