From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C4DCC76196 for ; Tue, 28 Mar 2023 19:29:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1D72C6B0074; Tue, 28 Mar 2023 15:29:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1879E6B0075; Tue, 28 Mar 2023 15:29:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 04F39900002; Tue, 28 Mar 2023 15:29:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id E96CC6B0074 for ; Tue, 28 Mar 2023 15:29:39 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id AC1BE120419 for ; Tue, 28 Mar 2023 19:29:39 +0000 (UTC) X-FDA: 80619296478.08.C5B6DE9 Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) by imf25.hostedemail.com (Postfix) with ESMTP id C6C8DA000C for ; Tue, 28 Mar 2023 19:29:37 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=TucsBd+N; spf=pass (imf25.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.167.47 as permitted sender) smtp.mailfrom=axelrasmussen@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680031777; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Sze0hZQs4AzNTu34VgpoyYMvtZyrr4yHRgV/mYMoCA0=; b=E1vSpxPSihpz6KkUgqVziDLh+b9wVBszcL2yJ8kbqqqBF2pErKfNLA2BVAOXk30UcQP5om DzvWAt87ni5U+YVqev96Ufg8CyM5Dy8geHjkLmsz4hL2HB9vPZuhJ+tvtL6QoFNynN5JKS CIsZXHGu7eqQrAf3fd8ZxM9dKSE0yAs= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=TucsBd+N; spf=pass (imf25.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.167.47 as permitted sender) smtp.mailfrom=axelrasmussen@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680031777; a=rsa-sha256; cv=none; b=lE43e2J5uwbUTwvYu/0O951qhZZBWJDVTKmoZyPMeayaDB4jOjJWQ+NfBjHen2ahapFi6W 6E32e1YtY111/Nk7C7E6VnRZddNwAqZMfnkrQPssAJB7bQM581dTFZEzVXtVFDPDwzjlY2 nW4+47ddVoC7onc7q7O3mjgp2nJwzIQ= Received: by mail-lf1-f47.google.com with SMTP id h11so10242935lfu.8 for ; Tue, 28 Mar 2023 12:29:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680031776; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Sze0hZQs4AzNTu34VgpoyYMvtZyrr4yHRgV/mYMoCA0=; b=TucsBd+N/AQxtwaL0+o5NkcY9MSe4BBeB7YoVEYpkQV8wnCSGXj8+ipQbmuXWEzW2A GM8O3lhwnn4JcP6ON75Pevuhje9cNtUFHOjcmISo50lT1G41d+14fgUUHDcPconVy0fS 5PyjuLBTZNnd8kgM55m7WRtrqA2p/Dke0lm/uMTLsExLNM2SVAfj8g2kHorU2izEeIs7 JT0edO0vY6WohLcj+qnliJeULIVv2uKBg3zZMuw3hiFBAvgOPLrcRvcvRUfnSQrjuCqm 8SMJTfWqtJaEUmUpAZoIkvXoKJ2nWCFOyhRwp9XoL7vyAyRpajv034xLjjtBT8WlcCJn nKUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680031776; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Sze0hZQs4AzNTu34VgpoyYMvtZyrr4yHRgV/mYMoCA0=; b=7bvVSCdDyK6T7NKZc0/RsRG+smeychShUqcRDlQrOWhG5GP1MtgUcufXBP2sDZjPRG hEPYXcNFfZnsnix/VQOAmPaa2taaM48NL4toRrYeEqMbfJI6TpuQRZrOhYvNmLyKc1KK xV7gKHvIUZJTywIsF0r683wcw+IquMGtf8iOvLmzSuOtzrc5LSwR+YO904Vtki6cLDo4 EpXDCxE2+1/JCEnl4oI6r1ZKyt7+/6sShMqQgwUrU/ZJdzmFevCgK/h7GnFLsMR4BNme JnCa3a5FJlNr0IHAf5LomC/lnqzmdaNLM9AT7KN+voKy8aq4tFMWi432gWCnUlV15t+1 NB9w== X-Gm-Message-State: AAQBX9fABLMOlaJv8KqWK0qeKa65YDBYsWLr0ik3a4o0lzRsUWzxShPt uM8rTTVYAMJtdzAmvLJaY7itPpnrPiRdAXa2VtL05Q== X-Google-Smtp-Source: AKy350YFSKMXJnqTblo/+gcV15ljJSRH6Afdq3y71v4w821k/pohARkBmE7hu6p9kVh0UhqyN06tpfTIQVNOXx2mkYI= X-Received: by 2002:a05:6512:4ca:b0:4d5:ca32:6ed5 with SMTP id w10-20020a05651204ca00b004d5ca326ed5mr5233121lfq.3.1680031775704; Tue, 28 Mar 2023 12:29:35 -0700 (PDT) MIME-Version: 1.0 References: <20220722201513.1624158-1-axelrasmussen@google.com> In-Reply-To: From: Axel Rasmussen Date: Tue, 28 Mar 2023 12:28:59 -0700 Message-ID: Subject: Re: [PATCH] userfaultfd: don't fail on unrecognized features To: Peter Xu Cc: Alexander Viro , Andrew Morton , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: C6C8DA000C X-Stat-Signature: 77zbe6t8o7ogoa63b8e1ggruhbg3y953 X-HE-Tag: 1680031777-498355 X-HE-Meta: U2FsdGVkX1/Crzl2O7/gL9+Mq7O8TEhuokfPApzYAVzbBF5mpnXPlk7tK9jXtp9EsxnLOPTUqyHwABqVnFqDilvKIh4jaU5lV2kxqkjJJW1J3rObF2RcIlB2FhjWFZxdp4fUh3mahCYS7baAwHJoHI9HQPcyP4FSBtmSjgjxoPkWmTbz4KF2kxw5sJqrVDC5WKSSjEHV9xFlZGYrw83EJe7zU3bqdx8Gn1shOkPJh+xlinwNZdK+OaLIZpnPcWIFynxZagphp5Y2smeADK4tQ25a4xSRfO9/t1u6oz1r0hyVEuMBq1bzYaAqRycN1FoAyAK0xAZdqDjTcDpSUByEpLvgikNRrUlUeh1cVQZWhiL2dCgaLmo4UOG49Hc7mxdYrdn3AUTioDaTPDq5C3sS7zvoyhc8qoIrcQaczgVIyPWuSKHuafj6b7/R6/d4KpL0vjBrxBiUljKpIDKFkwaQoJot6uYzn3IJarQl79TG0X2pP2eowBpVTgkx0HJkWpD/aQOboxpN+Di0vCgQnbdpwn4oCVtDRDZpdRNdgX/ngFyXTPWB8LzYq6cMX4ydHncw8MOyEU2HEJZSnk7RVlKMKpB7qn8PMDQn5St/xfZ9o7KkoFDfKTCWp1nNETHl6W04NjDoTzvER/UEy6BOxR2NQMFpbehvRXrEM/LvHbR0JfWHH1fWKG8VLbhcSmcNoNhWTpmejvW5/rOZjrCXiWeSIj+3gsJjGtPGO2iK3XXIaYUIAyL0SQRbaLt/0fhspLgSGX23EqYHOxK77RMMdIXMAKc1H48izpunhanWhh3T3vLh/o/skkWK/LmTmPHdo1ZY6coP5lBBREz/YfUrrOe+4THvJL0SptYaUE3PMlCfQS478+CFnC9HR05SLE3yakAmacf0mCSgQIXA80UyIK6ltMM40neu+KCZTUNJiK+9FPm9sw8MXuKPsQx1Qh6MrSJtCF9+2AJYxGaIDPExat8 1p9VuqVL BUWpCS51Ups1w52Zap//wAByc4L8F6rN4qcjZThz51g/AcrZTVkTRs2lfdETcW/QrgQRxpcEv6Lb1Qm8f2uLL1a/7breU4BRPXJr6YGdDYrZO/ypCGICfc1Pam0MqkE7aXjXqdwBIwcS/gm3BNfNKaft9s1AOcxk/tcdExhVhV8GBVLmqiGLGSNghgdT85kfGftj/O+AaJwu/Q5QDmjMcfNmcdgXGhEJX3Aod784PxrBeeHpqV9Rvjbg2S/DwnJduAS9a7B8c/DxFRoLIJioaCH6wPhjDRdPXEP7f X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Mar 27, 2023 at 2:01=E2=80=AFPM Peter Xu wrote: > > I think I overlooked this patch.. > > Axel, could you explain why this patch is correct? Comments inline. > > On Fri, Jul 22, 2022 at 01:15:13PM -0700, Axel Rasmussen wrote: > > The basic interaction for setting up a userfaultfd is, userspace issues > > a UFFDIO_API ioctl, and passes in a set of zero or more feature flags, > > indicating the features they would prefer to use. > > > > Of course, different kernels may support different sets of features > > (depending on kernel version, kconfig options, architecture, etc). > > Userspace's expectations may also not match: perhaps it was built > > against newer kernel headers, which defined some features the kernel > > it's running on doesn't support. > > > > Currently, if userspace passes in a flag we don't recognize, the > > initialization fails and we return -EINVAL. This isn't great, though. > > Why? IIUC that's the major way for user app to detect any misconfig of > feature list so it can bail out early. > > Quoting from man page (ioctl_userfaultfd(2)): > > UFFDIO_API > (Since Linux 4.3.) Enable operation of the userfaultfd and perfor= m API handshake. > > ... > > struct uffdio_api { > __u64 api; /* Requested API version (input) */ > __u64 features; /* Requested features (input/output) */ > __u64 ioctls; /* Available ioctl() operations (output)= */ > }; > > ... > > For Linux kernel versions before 4.11, the features field must be > initialized to zero before the call to UFFDIO_API, and zero (i.e., > no feature bits) is placed in the features field by the kernel upo= n > return from ioctl(2). > > ... > > To enable userfaultfd features the application should set a bit > corresponding to each feature it wants to enable in the features > field. If the kernel supports all the requested features it will > enable them. Otherwise it will zero out the returned uffdio_api > structure and return EINVAL. > > IIUC the right way to use this API is first probe with features=3D=3D0, t= hen > the kernel will return all the supported features, then the user app shou= ld > enable only a subset (or all, but not a superset) of supported ones in th= e > next UFFDIO_API with a new uffd. Hmm, I think doing a two-step handshake just overcomplicates things. Isn't it simpler to just have userspace ask for the features it wants up front, and then the kernel responds with the subset of features it actually supports? In the common case (all features were supported), there is nothing more to do. Userspace is free to detect the uncommon case where some features it asked for are missing, and handle that however it likes. I think this patch is backwards compatible with the two-step approach, too. I do agree the man page could use some work. I don't think it describes the two-step handshake process correctly, either. It just says, "ask for the features you want, and the kernel will either give them to you or fail". If we really did want to keep the two-step process, it should describe it (set features =3D=3D 0 first, then ask only for the ones you want which are supported), and the example program should demonstrate it. But, I think it's simpler to just have the kernel do what the man page describes. Userspace asks for the features up front, kernel responds with the subset that are actually supported. No need to return EINVAL if unsupported features were requested. > > > Userspace doesn't have an obvious way to react to this; sure, one of th= e > > features I asked for was unavailable, but which one? The only option it > > has is to turn off things "at random" and hope something works. > > > > Instead, modify UFFDIO_API to just ignore any unrecognized feature > > flags. The interaction is now that the initialization will succeed, and > > as always we return the *subset* of feature flags that can actually be > > used back to userspace. > > > > Now userspace has an obvious way to react: it checks if any flags it > > asked for are missing. If so, it can conclude this kernel doesn't > > support those, and it can either resign itself to not using them, or > > fail with an error on its own, or whatever else. > > > > Signed-off-by: Axel Rasmussen > > --- > > fs/userfaultfd.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > > index e943370107d0..4974da1f620c 100644 > > --- a/fs/userfaultfd.c > > +++ b/fs/userfaultfd.c > > @@ -1923,10 +1923,8 @@ static int userfaultfd_api(struct userfaultfd_ct= x *ctx, > > ret =3D -EFAULT; > > if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api))) > > goto out; > > - features =3D uffdio_api.features; > > - ret =3D -EINVAL; > > - if (uffdio_api.api !=3D UFFD_API || (features & ~UFFD_API_FEATURE= S)) > > - goto err_out; > > What's worse is that I think you removed the only UFFD_API check. Althou= gh > I'm not sure whether it'll be extended in the future or not at all (very > possible we keep using 0xaa forever..), but removing this means we won't = be > able to extend it to a new api version in the future, and misconfig of > uffdio_api will wrongly succeed I think: > > /* Test wrong UFFD_API */ > uffdio_api.api =3D 0xab; > uffdio_api.features =3D 0; > if (ioctl(uffd, UFFDIO_API, &uffdio_api) =3D=3D 0) > err("UFFDIO_API should fail but didn't"); Agreed, we should add back the UFFD_API check - I am happy to send a patch for this. > > > + /* Ignore unsupported features (userspace built against newer ker= nel) */ > > + features =3D uffdio_api.features & UFFD_API_FEATURES; > > ret =3D -EPERM; > > if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRA= CE)) > > goto err_out; > > -- > > 2.37.1.359.gd136c6c3e2-goog > > > > -- > Peter Xu >