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 E660AC001B0 for ; Wed, 19 Jul 2023 21:53:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1D104280093; Wed, 19 Jul 2023 17:53:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1823A28004C; Wed, 19 Jul 2023 17:53:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 021B4280093; Wed, 19 Jul 2023 17:53:44 -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 E5CCF28004C for ; Wed, 19 Jul 2023 17:53:44 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id BE3E080469 for ; Wed, 19 Jul 2023 21:53:44 +0000 (UTC) X-FDA: 81029713968.18.996E733 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf17.hostedemail.com (Postfix) with ESMTP id 7A91B4000F for ; Wed, 19 Jul 2023 21:53:42 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b="d/d7udCD"; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf17.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689803622; a=rsa-sha256; cv=none; b=3s2UdR57/bIWBRjq9Xtc3jnBTfm+ynM+5w0p97csZDHjOjzgDoAo5O4u4KqoqKbdDAmGY0 VqPWnqPY0TRfg/qMPl9XFOurEMqPQ6fI1914t0fsk+XcKZSJMcviOVTZHxxmWJXc4WXVrg 9lfr0zFaqgcVv9shVnk9yUMJC1u1NRw= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b="d/d7udCD"; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf17.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689803622; 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=ue7cNqzzVVz20RPZ8gKHsEKAUMq9D8/GqGljzfWyrU4=; b=30oNq0Dn4t4lYygefjZLq75v9iG00HTK4BrfFhtbA0k9XiBcc2vTv546PGRaLF8OqnWj1L 6FHy9PJx1Y8GCaQdo4sQy1VV73NC2n8+Y/uf7q73dzxFVUcwsUuQVs4A3Q8fFfn6zMvSwe 6HMwsOa5nUkJbgWS8iY099Y6/vPH8Pw= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689803621; h=from:from: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; bh=ue7cNqzzVVz20RPZ8gKHsEKAUMq9D8/GqGljzfWyrU4=; b=d/d7udCDoD1YEEtGDXdjZA7UApaH6cAcQ87DDq0f6f3Pads6/evs+LaWWQNcZeHUurBNpw zysehtL9NH9WZyxHZanxFjoGhCt8ZYqsOMX/bl7YEBCLDjFZoUkQxGOU1Hjh+nqxZfdU6O HlyAs0lAo+iwxztqgLG4gb5Rmy0GloU= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-139-t1EDLRM7OmGiBVLzUJLqjQ-1; Wed, 19 Jul 2023 17:53:40 -0400 X-MC-Unique: t1EDLRM7OmGiBVLzUJLqjQ-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-635de6f75e0so294136d6.1 for ; Wed, 19 Jul 2023 14:53:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689803620; x=1690408420; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ue7cNqzzVVz20RPZ8gKHsEKAUMq9D8/GqGljzfWyrU4=; b=AROh/2vNqK2vOLOAJpIkbLGV6XVEXkBy6lKv9AbwHp1i4xh+g9f6P1R5xcCU/ahJK1 PbPZNjHC2reVMDRj8RVRRw1KL8j9REHlaIn8RT7bKR90MdphkOELM7UELbTQR6ai4aQN gcA8D0LYjJGeHK7OalTf33igey9H5ugHnGYF9QBd5mDINUIxG5k5Swx2nW1MBCG1RZcP kAVzlSkHRK9hiptrOH2bwExgYlAS974SOt0ysiv5jxbAXtKomyZ1rbWdmmh/mdMR+x8X 6StxYr1l9tkORbad0/w8/R/FxOAOKjP7lZVaJl6YK+sbyC4fd6X7ZgWYCuis3XJf1+lr HSpw== X-Gm-Message-State: ABy/qLbz8n7wdwBxbwLEMuWZw6cAOZQlVwpkhRtaDIf/AoU/Wa7F5P9R MpQzdUV+ZZVeSy9GxxqLXmnU/jfZVBlFktpO+HDttroWmQQgOSKM9BnbUurVwstahqHnUMNBETw 4RdHGaXp8d1Y= X-Received: by 2002:a05:6214:2403:b0:639:d239:b4fd with SMTP id fv3-20020a056214240300b00639d239b4fdmr675142qvb.1.1689803620008; Wed, 19 Jul 2023 14:53:40 -0700 (PDT) X-Google-Smtp-Source: APBJJlEBTa5HtSKKBySX7SQbJ8siDPJdlaDSAxjs30dsW6dxj6tS9XmNTDTK47+DWr6fq353zhnkgw== X-Received: by 2002:a05:6214:2403:b0:639:d239:b4fd with SMTP id fv3-20020a056214240300b00639d239b4fdmr675120qvb.1.1689803619703; Wed, 19 Jul 2023 14:53:39 -0700 (PDT) Received: from x1n (cpe5c7695f3aee0-cm5c7695f3aede.cpe.net.cable.rogers.com. [99.254.144.39]) by smtp.gmail.com with ESMTPSA id c3-20020a0ce143000000b0062ffec0a18esm1732358qvl.84.2023.07.19.14.53.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jul 2023 14:53:39 -0700 (PDT) Date: Wed, 19 Jul 2023 17:53:36 -0400 From: Peter Xu To: Hugh Dickins Cc: Axel Rasmussen , Andrew Morton , Dan Carpenter , linux-mm@kvack.org Subject: Re: [PATCH mm-unstable fix] mm: userfaultfd: add new UFFDIO_POISON ioctl: fix Message-ID: References: <517aeee1-b4cd-c3fc-3236-e2a33c115af5@google.com> MIME-Version: 1.0 In-Reply-To: <517aeee1-b4cd-c3fc-3236-e2a33c115af5@google.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 7A91B4000F X-Stat-Signature: 69e7kza9une6wmgbamgycttdqaaxme1h X-HE-Tag: 1689803622-446363 X-HE-Meta: U2FsdGVkX1+1KBDLyZ9sIz34sO+Dzl/W7LgaPbEJqY/hOwaT2a8BLL/9nj4fLr2+N0Qa0dvLsgZMGP5cW7By7UgyXZcqSyA4L57fpahpPgnwC0eoxpgGydc/TrJppv5hjW0fSvyeOjzPxk7C4R0KJHwFtme+8R3TZsnDzzHqWu3leauAsJagIyUgNzSnEkm9gdSgyyVfuinoSbR0VAS+ttSVxw4wla3RxhARUiCYFcwY7cgjoWRhLtJ3ueiGUbIj9DaC0mSAVGZj2Vlhv28PITOSLu0snola3LxkvMn7+ec/ODRDSV63oRr2rTO5uMuAk+DgHz+P1wJbqJrHzho1L7vGuyfILNZ8gmu+EpJnZKj5N+9rQMzvE5YLIfQCwC/Vb+K/zN0tNSSSRTCeTKvAm6PUq7Q5ePdLyU262zQQOzl5bMcL/7IM+bnImKEYFkC+RjI7E99bl0Hj3RT8AHY8ka2YNX+olVK9rZM/oTGap9tuDOHQqn8pTjW6pRuKBM17TTeu2UdxiiFWxf2TdziEJzXANsvWFzr0cTJVdjrPRBeEV1+6c4jqaqlwFayr1be92hPaXUXyjG/eJjGvzE/TRJS0J/yAzjWK8yNMtcTzM38zhQXuuJxX8HFkNnVSjAJQ04f2+opiA4u5CvVPj7tnyUkM+5w8qoKGPqdNI/4Vdvc+k8zS7IECDhL0XNms5VON6N1ulINkPVrjzfFcy0cDAamuh25Hga9Rm4IxYWT/iU3gBxzXC3vFnhgjYulwQd+O3DUZFDDhOvGoCYOvZWy5UTWfiHpw1rXJOnn//N2j4HANizkRQ6rJlANmv/bWNaikel+WnSBoqCYZl/lgqaUCZyQ9OI9cgATliCgdKkOT5Lw9iHPKC1X8hNRtFvEzpeJwkUPnd0inKY7gjjh3JyPrMis7Caiqr+mQDlgfxeM0MTZ8fgimf34Qbkdcmn0ojTARkx50xDyiiyS9w8xzDKn 1c6TJ8I5 xV0GTu1MjDgMyMWzUYJPF0k7POSPyEb/fgdK7i9F0q03o2Q/lE2WtE2UIMR5pOTDNAtmrQ0vzkKfQmtbXJOqCf0eaLTkTeBclDnkoXT51DZnLZ78tPiOS6WWs2h4TVtqIcKIXixtK1V3VyHsh9CHjI5mhBqoc2JbcarWacQGWcE27uiegb3p5oOz3Mb2cnMadbqJAPsjmycS7dXKcRnxF6RMDuk+Slc9QReUCsLWK/WhamNObG+rbLnmKIWpEYE0Uov4Pe370MOxG3ybIetSooAXAv1rdoq3t/BOZwIIbsNwkFHRhmU6Dy4+uzSwaE8+DgLQSPoomzZx7stX0LOJ7f1LeAKxGwNUKSS+VyB4fDANbqDN3GstW/4ke2oHE8kivazrpJ1I+ECwMlIArHHrXKN8/QX2aGxhDkdlv6Pk2ucuaRTwYaPkcSo7gEQ== 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: Hello, Hugh, Axel, Sorry for a very late reply after a long PTO.. On Thu, Jul 13, 2023 at 07:40:30PM -0700, Hugh Dickins wrote: > On Wed, 12 Jul 2023, Axel Rasmussen wrote: > > On Tue, Jul 11, 2023 at 6:27 PM Hugh Dickins wrote: > > > > > > Smatch has observed that pte_offset_map_lock() is now allowed to fail, > > > and then ptl should not be unlocked. Use -EAGAIN here like elsewhere. > > > > > > Signed-off-by: Hugh Dickins > > > > Looks correct to me, thanks Hugh! If it's useful, feel free to take: > > > > Reviewed-by: Axel Rasmussen > > Thanks, that will have helped Andrew (but would vanish into the merge > with your original when it moves from mm-unstable to mm-stable). > > > > > > --- > > > Axel, Peter: this seems right as a fix to the patch in mm-unstable; > > > but in preparing this, I noticed mfill_atomic()'s code before calling > > > mfill_atomic_pte(), and think that my original choice of -EFAULT was > > > therefore better than -EAGAIN for all of these; and that mfill_atomic()'s > > > BUG_ONs there would be better deleted (and is its BUG_ON(folio) safe??). > > > Something one of us should address, after this fixup is in akpm's tree. > > > > Agreed, dealing with the BUG_ONs is something I have been meaning to > > do as checkpatch bothers me about it frequently. > > > > What this code is trying to do is to enforce the contract that: if > > mfill_atomic_pte failed, then it must have released *folio and set it > > to NULL. But, you're exactly right that if we had such a bug, it would > > mean we leaked one page in an unusual error case - not great, but not > > worth crashing the kernel either. Yes I also agree the BUG_ON might be a bit too aggressive, even if still accurate (so we should really not trigger that). WARN_ON_ONCE() should be enough. > > > > For UFFDIO_POISON this doesn't really apply because it doesn't > > allocate or get a reference to a page in any case. > > > > For other cases like UFFDIO_COPY where it does matter, I think it's > > fine as is regardless of the error code returned (as long as it's not > > -ENOENT :) ). mfill_atomic_pte_copy() is sure to set `*foliop = NULL` > > before it attempts this lock (in mfill_atomic_install_pte). So -EAGAIN > > or -EFAULT should work equally well at least from that perspective. I > > somewhat prefer -EAGAIN, but maybe I'm missing something. > > Right, when I said -EFAULT better than -EAGAIN, I was only basing that > on -EFAULT being the code which was already used for pmd_trans_huge() > in the check above mfill_atomic()'s call to mfill_atomic_pte(). > > I don't disagree with you, and Peter too preferred -EAGAIN: it's just > better if they all use the same code (or maybe mfill_atomic()'s first > check for pmd_trans_huge() can be deleted along with the BUG_ONs - but > I have not checked through the cases, there may be very good reason to > keep that preliminary check there). It's just that -EFAULT will definitely fail most of the uffd based apps, because no app can handle an -EFAULT over any ioctl(UFFDIO_*). So if we'd delete the 1st pmd_trans_huge() check we may want to make the 2nd return -EAGAIN. I think this -EFAULT (of 2nd pmd_trans_huge() check) should really also be an -EAGAIN, I'd bet ten dollars if it's really hit somewhere before it'll crash the app as explained. With Hugh's recent rework on pte_offset_map_lock() (which should contain a thp check within the map_lock()) IMHO we can safely drop the 2nd check as a whole (while keeping the 1st check as a fast path to detect existing thps), and then we should reliably return an -EAGAIN if a thp raced with us, so the userapp should just retry when race with anything else. Thanks, -- Peter Xu