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 7A2EEC76196 for ; Fri, 31 Mar 2023 16:52:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0A0706B0096; Fri, 31 Mar 2023 12:52:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0520F6B0098; Fri, 31 Mar 2023 12:52:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E82176B009A; Fri, 31 Mar 2023 12:52:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id D77336B0096 for ; Fri, 31 Mar 2023 12:52:56 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id A99A4AC9E6 for ; Fri, 31 Mar 2023 16:52:56 +0000 (UTC) X-FDA: 80629787952.18.CFE12FE Received: from mail-lf1-f45.google.com (mail-lf1-f45.google.com [209.85.167.45]) by imf05.hostedemail.com (Postfix) with ESMTP id B844B100004 for ; Fri, 31 Mar 2023 16:52:54 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=iGu2B9Io; spf=pass (imf05.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.167.45 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=1680281574; 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=KRqGjA4IFBSpXp+ilR/BAk1W8vT0JhFNe6v3C7wlzoo=; b=dV6zWlO4jJj/kGeswJaOgqg/xYbVf4oGxOMJ0CLjmDk7yNlojJolbH+kENKSCokExKVM1P oNaiYEdPAkpnYyEdyFsEv6G7zCrcmbY6rknib/GGXzsm0c3Cgf+DYo7Q6XHJPloDGtS7Ze q50fXZav3iZFlkmMEsPndMmfVZfhjO0= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=iGu2B9Io; spf=pass (imf05.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.167.45 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=1680281574; a=rsa-sha256; cv=none; b=f8VDWgZiO3Qa/DKkN+QTmNITt6nQb4lRRTTHn2MbTtJclPbfXTUV28UDHb8Wvrl57/etpm QkJy/oA6SjJ+9O6XgUJuXOigh5+qifTW8XpTh8ATILIcDQ/xWGAtpadtt0F7DTdfecfLsa llXCaYJPWMhEAI8A9kCXXjs0DFOGdKQ= Received: by mail-lf1-f45.google.com with SMTP id y20so29754523lfj.2 for ; Fri, 31 Mar 2023 09:52:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680281573; x=1682873573; 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=KRqGjA4IFBSpXp+ilR/BAk1W8vT0JhFNe6v3C7wlzoo=; b=iGu2B9IotHWel6Gv85IfDHmIchNd4aW0ojCsJ7VnYnRa+9MecczZ9+5cJv8b3jW7ev PZMfkriPP13RxLuKDvARNTn2qTt42mS9FzQDrqpp+8Yd6MrazVA6gEUYnxXQh3GAW96e /k4WkALWmV8yTnzR8lJ7Ih3reb7oTWanoEu4y99Zh59JqpDa2cPjXCbfLXdF6fpGyjwn 4EJtt94dvioyIJlNgmoh9EFRAH/Z0Q0vl8pXprHTNym2WPhHJMUYzquDCYm+Q8MvI4i9 BqAor3JCRQ/LZRGRMwyQPlVFWTs0eJrfTOLgmdvsZr0MFZgEkcsyy0ZuwZT2bAPU0Tn4 i6hQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680281573; x=1682873573; 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=KRqGjA4IFBSpXp+ilR/BAk1W8vT0JhFNe6v3C7wlzoo=; b=lQtNs5x2l2AotE7H/MGtHceRwNgTLiaGSNrrtY9DOCQB4qoh3c0tXn77ljDkFbrGg2 tKTUCzUtURjLVgz2MlvCbr8RD0t/MJ6uWRqrt1VpB3aqXufpeU3DCyZSGeGdyYwWnkHg TbSuPGn/CEo2Ee/VtN2No+8zLiI78ZcMlzLqmsw5UQCWrvms+41y4MjGd0vKMj0Zii8x a8TzkLOtJZNo3zmU0dXJ70Pox218HcvUDlZ2Hz7S8nMdHfHgh3Ep4qKv6r7VhGEqpsho JmNDbZ+NGmY9Vs5eioi0Kkm0ehuxpxBPBlLNRjPqNB659HXHzmMh46WjJuCaG6ikOlZm ePpA== X-Gm-Message-State: AAQBX9f1fBThFu+xs+4cCAJZaIBQnr2ZBOhakwJHPI1DxTeS7+GxWuc8 QxKow9SagxKZpCo4lLducFVDjEAOJcPMtSN3kE/Frw== X-Google-Smtp-Source: AKy350ZDBdotJtsCIiTvfV4i36oUIym0exfSUykMqeFq2GnYu4xjZXuYqedJhRVqQCVhN8/MUcw2n83ZK2jY6xuaJMc= X-Received: by 2002:ac2:4434:0:b0:4dc:807a:d140 with SMTP id w20-20020ac24434000000b004dc807ad140mr8163687lfl.10.1680281572661; Fri, 31 Mar 2023 09:52:52 -0700 (PDT) MIME-Version: 1.0 References: <20230330155707.3106228-1-peterx@redhat.com> <20230330155707.3106228-2-peterx@redhat.com> In-Reply-To: From: Axel Rasmussen Date: Fri, 31 Mar 2023 09:52:16 -0700 Message-ID: Subject: Re: [PATCH 01/29] Revert "userfaultfd: don't fail on unrecognized features" To: Peter Xu Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Mike Kravetz , Andrew Morton , Andrea Arcangeli , Mike Rapoport , Nadav Amit , Leonardo Bras Soares Passos , David Hildenbrand , linux-stable Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: yenugowftskii5fbyjghprnc5u9udooq X-Rspamd-Queue-Id: B844B100004 X-HE-Tag: 1680281574-317604 X-HE-Meta: U2FsdGVkX19PJvXKbFE1IrjMe4QuU9f9vg4o9ohqiSgGKkp/oH0/vBJbf37c78Rt/53V7q/9WZXByFwW69MOtMH0eZoWnJnyU+5Pq3E55NS/hk6ZWNSJt+yOzjOndISknWrnGafF8NNPzDukyXUVdzc/D8Pm1ix3S0z4isX5JA5mx6lypeTF76guN1IFahUKPvan5HGWU7HggSYleKX54/zZulje2SU6B/s+nUNDrX2KRJIBxG7nQeApORr1264mqAe3TMoOY6C7zpAxGYbThPqoB7AwktauypoE6dLK3QqtsXLxopXTDGq6ZsEOz5sScupfQC4m6yf6DEEgrKmvt93d2RMJKI9MFXIDPcGccx6uOyR2ZwqH5+mdNtrFAXv9qS8EhSahfY4NWv55B0lmYl9SURfMlsB56JUEZzxpOJ4r2uvA/CrpgOPfJjxw3Y7uNvHRds/DhwJMQbFPRw4AEnQbaHl7WOIl032c81gqaSffktl5qe7aNMudu2YKc5hw4gOgHvgzSDcjjuMHCq8yPdIUYiWTnv5VDU69NKcRFeGrMB6p8OaOXuDf1mii+XA8BZMDnz+AIUh65qA/H3lcdJW+T+bAL2efYDn5hfE+PPh5zGNI/TtMD7aoOLrjCEs5/kN/6lwuofA9gnSeAYMNlCMUilHcj5NSDIsp9MBFSbLLeecJ/3SXDReGQOpEV8oqW8Aa8KAXl9HJmxViFce7hy9srCUkWXcQIW7Zp2oSbWdKsBiEaZWFl/E8+m38f+4MWMxH1lbURHujvFdpc7TcjnE5rlrt0CnFOqcoNefbX57Z4BhEj52NHDcUqN/83j5dXCArvjZRE1jOkeiZcEH1UvtWr+2orxBdLNkLDUsSLADXBOBpBasqC1cSgxVPX/V41m06krcw2++zQYOxhEoBb97KG9oiOEn1ARNyi8rfHFpNwA93roYr9Da28klablwD2JrirPJyOG7deQ9OCq7 RdgBhkpO BtN2hMcfrUb0aGUpQeAkyEueGdJkJkAIgdu28404PrgrRHiWKB0zdR8ATVp/nU1f0JAP13PhOJ7udHZoaTsyJofl+bA1ueiKJ+xhrat0xqPQf+5j43lLEL6mrMg/Et7PFp7lozRj3eYWcBpaQHI5U2CfXfeunsw9nfwzR1p5gnZ6R/VhQFBJhoyl9vYe1HqhUE27jH9UgbYtl+beIRiJV0EasdsMjv7BRvSz6rVu56YK1lMbZQIWsjqxEcW3GXKEXJkSShx16ImAI8my22+U97uiW0bwFIsfakvJcQaiTS6Dhh63p5W6++QYR0hOrOtfLaYGYzXu0ZMLkaX9cMeQSMQwjYqIdxDBIrj+fUToWssPHw3Y= X-Bogosity: Ham, tests=bogofilter, spamicity=0.066415, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Mar 30, 2023 at 3:27=E2=80=AFPM Peter Xu wrote: > > On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote: > > On Thu, Mar 30, 2023 at 8:57=E2=80=AFAM Peter Xu wr= ote: > > > > > > This is a proposal to revert commit 914eedcb9ba0ff53c33808. > > > > > > I found this when writting a simple UFFDIO_API test to be the first u= nit > > > test in this set. Two things breaks with the commit: > > > > > > - UFFDIO_API check was lost and missing. According to man page, th= e > > > kernel should reject ioctl(UFFDIO_API) if uffdio_api.api !=3D 0xaa.= This > > > check is needed if the api version will be extended in the future, = or > > > user app won't be able to identify which is a new kernel. > > > > 100% agreed, this was a mistake. > > > > > > > > - Feature flags checks were removed, which means UFFDIO_API with a > > > feature that does not exist will also succeed. According to the ma= n > > > page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if > > > unknown features passed in. > > > > I still strongly disagree with reverting this part, my feeling is > > still that doing so makes things more complicated for no reason. > > > > Re: David's point, it's clearly wrong to change semantics so a thing > > that used to work now fails. But this instead makes it more permissive > > - existing userspace programs continue to work as-is, but *also* one > > can achieve the same thing more simply (combine probing + > > configuration into one step). I don't see any problem with that, > > generally. > > > > But, if David and others don't find my argument convincing, it isn't > > the end of the world. It just means I have to go update my userspace > > code to be a bit more complicated. :) > > I'd say it's fine if it's your own program that you can in full control > easily. :) Sorry again for not noticing that earlier. > > There's one reason that we may consider keeping the behavior. IMHO it is > when there're major softwares that uses the "wrong" ABI (let's say so; > because it's not following the man pages). If you're aware any such majo= r > softwares (especially open sourced) will break due to this revert patch, > please shoot. Well, I did find one example, criu: https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L266 It doesn't do the two-step probing process, it looks to me like it does what my patch was intending users to do: It just asks for the requested features up-front, without any probing. And then it returns the subset of features it *actually* got, ostensibly so the caller can compare that vs. what it requested. Then again, it looks like the caller doesn't *actually* compare the features it got vs. what it asked for. I don't know enough about criu to know if this is a bug, or if they actually just don't care. https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L312 > > > > > I also still think the man page is incorrect or at least incomplete no > > matter what we do here; we should be sure to update it as a follow-up. > > So far it looks still fine if with this revert. Maybe I overlooked > somewhere? > > I'll add this into my todo, but with low priority. If you have suggestio= n > already on how to improve the man page please do so before me! My thinking is that it describes the bits and pieces, but doesn't explicitly describe end-to-end how to configure a userfaultfd using the two-step probing approach. (Or state that this is actually *required*, unless you only want to set features=3D0 in any case.) Maybe it will be easiest if I just send a patch myself with what I'm thinking, and we can see what folks think. Always easier to just look at a patch vs. talking about it in the abstract. :) > > -- > Peter Xu >