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 26B32EB64DA for ; Wed, 12 Jul 2023 18:17:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8AD678D0005; Wed, 12 Jul 2023 14:16:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 85CE48D0002; Wed, 12 Jul 2023 14:16:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7260B8D0005; Wed, 12 Jul 2023 14:16:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 6130D8D0002 for ; Wed, 12 Jul 2023 14:16:59 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 1E706120346 for ; Wed, 12 Jul 2023 18:16:59 +0000 (UTC) X-FDA: 81003766158.15.36C0967 Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by imf25.hostedemail.com (Postfix) with ESMTP id 2FF5DA0012 for ; Wed, 12 Jul 2023 18:16:56 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=AjgYdUPI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689185817; 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=x1j+60NVN+zXXxPjpNsyDo/Se0MWb1kRDzd0k4DV6VA=; b=SOXbMgvO/ydmX0SHylga1O4iYNNLhaePsTLdQJyl30fp5/AOJOlPbFCC7DmxlA1wJmzozD InqTicfpPI3DzlzYTfdVeWduXJbTosEo5ODRAYrE8KdQEycHB/ATQXZ1ogIsnBKwE5FsK8 qO5rYwuKyRK2gUs9MJQDt5AtAvIrr58= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=AjgYdUPI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689185817; a=rsa-sha256; cv=none; b=smhlV+6ziC/cSiTqpimN4m6E1oLAe9p4OCOYIKdX4NShUcoOv/Si7j5KCcQMV+2qljAUyM KvW9ZdLx7Yl/n8Lma+8QT/jucmqcReI5WJHDiSdS/n27cf1EDnIP3o/TIonvUYGB9+Dafg HXCOWXy+bUe3ecUi/YtG0PffP3eQHLM= Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-9939fbb7191so238849166b.0 for ; Wed, 12 Jul 2023 11:16:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689185815; x=1691777815; 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=x1j+60NVN+zXXxPjpNsyDo/Se0MWb1kRDzd0k4DV6VA=; b=AjgYdUPIhYbxj9/V4lXZbRVLFnqfKgWCJZLifN4178P5a0XZ1pCteTJshcrRFUjknQ fwzqBELZBirRmokdOPsMQZMr1y7NQvAasmrngU5o1B8b3UmV25W3Mec/n6TDRZ2hrXng FQfGVQBrY/AClxoBrRiFgbSPjSreV/o1Cy9JC3CsW4vyGcuB2Q6tCxXsJvzFCdry/3yD /NTBVElJDxr+W8ewTVZXECb1ouxhtSE67I1ZNVdlNii1cIt6X92oLFHMI8MXujlzNy1F nIvANvnNqVKeIjrFqd7DFv5Jzl2fZPl6x8QqDMbdOF8Id2qre/ryVU/L3mABQ/miPLfu NlUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689185815; x=1691777815; 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=x1j+60NVN+zXXxPjpNsyDo/Se0MWb1kRDzd0k4DV6VA=; b=V974FWFXD5vcJnZD21c/TaZnyMnaMvjtpKtccveC+keEerDkItbkX0xHEmttIu3s6/ AnX/HXtQ8YA1UGiCP+ov0yDkvAlY6xJepiXV6oFz9ROsbkICPdacxY0msKEYvzl0a/EW mUwReeXu0BvUfezpp57ne75GlCiuwpMgyx6vr2RvLAq+BpeB8w+TOoQ4sQjKLlCHVXOT nWdmZypPUMsLe4Gw96JYA/1vl/9hHw3MCXdWR+y0yhuyt+HMFg8UuSL323Jubo52ODuF YzUJCnch3hV3Ad3ARoRT/q5vbfvYds37QqAmUIx1PHw69Xw4+ObpWI1tAfkbozfhrv0/ XOsQ== X-Gm-Message-State: ABy/qLYiuKpUguKcC7nEkC8cqpGNPwMVpp+QVexC/NVhnR4LiJcr2ZkK Wu20cvpQGIbfwR9TEq7RyyuA7FgN0/XN80Eq9QDF9A== X-Google-Smtp-Source: APBJJlE3RoxCKnQFNQHkTnKdS40DGd+CJUVk75YWK2SQ7l0il84eF/Ahj/TpL9bGZYswsGU/QyAfd22egdORNHKqH2w= X-Received: by 2002:a17:907:16a5:b0:98e:48cc:4cbf with SMTP id hc37-20020a17090716a500b0098e48cc4cbfmr3790022ejc.26.1689185815090; Wed, 12 Jul 2023 11:16:55 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Axel Rasmussen Date: Wed, 12 Jul 2023 11:16:18 -0700 Message-ID: Subject: Re: [PATCH mm-unstable fix] mm: userfaultfd: add new UFFDIO_POISON ioctl: fix To: Hugh Dickins Cc: Andrew Morton , Dan Carpenter , Peter Xu , linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: hzeq59q61arsqmnxzksrcz13y7qpke3r X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 2FF5DA0012 X-HE-Tag: 1689185816-689484 X-HE-Meta: U2FsdGVkX18SpQhje1c/artSqsgUngLWgHhadJWz9mBQZdYYm+Bq0OtbQQERuuXYavxlorqDEFwV2mWiGhK4mjIxOf6wf7jGmU0/cIKzYdgAhOWemRtJ3h91yoamcss2i+/U3gy9kW3sLnFkWejoNfCyC3RK+IiXkLbcPkwXHngV3IOM0HZXMJ1oC/t/GCdmw9GLddwxFP43756gFjVOtnYW6vT/qSiZNLCBGGYjWy4a+S70n1S36s7POQVkchHTsJxOCpbgaoKVZDTqCr22b5VkE2hHkj/hJb3o1mUPJ14hVdwtymZ6ueuoL/Rgz46mBsOxGFgvtRytePDgVvnPOt2ffy5MXlhwNBDhixgzfl2PoepOW2jXSoWSpDtGfHqrkEG9pqJrXHS+cIJ3CzGVf4CQf4Y7FgDw4DTs7yYYWOps2rNXxCchlHZxhtMVZyB2m5U6aZN1r/aXsXsNNJNuaGypwJHjSg2Yo/CAE1Dt8pWP/QoV8qEaoXDXf6NIQAs2QLk0OlSaZ1z5aOrb1FM24LZCNidJab9EFtVlOHFfsrK+8/DkDBGxLdMGzaaClhFYVk4VLb6Y0PXjsAQwwPZvAEZ/cWQWxKvJMQtBFXp7oUzu2fBk1ZYm0OgZRxh11CJhBHGG6DxRG5xZ257TEj15on0WT8jk49HlKJXdhfesODevxH0eZisln6LK3kDzoP76A03LokVEX4UDhcRe/4jjgSfG92TQgZ5+XF69ja9ZWRfxK8Ky/LT+mJT5Sj8x0bAuT4MsKo/7fnC16sKvSaMk16nChzSoRSQ4qjcYApmlJUkjoKrVg40KSwzxEnGdx2Nu4r8PXpyODWg2YuGzsnfUD+hnyWTJLvBDQfk4XvtW56bAncSvKYrGnEY14AKaMP427KB/L11TyBP+ZVrGWRDy2d6jGt+ce0QkdnX0mtw5A3etGZFTd0ak+o7iphvICvuwHW/2S2Ygu4ac6Wxs/52 +VIHvyhi TbvEPhX6N47C+0A67nwClmQ+Rk0M2qbO7i6VTpOi1Q5Rbh1iIpHBkzjApon+A95LkxzYlxXDvM2Uga3sOqhmAH0iRNkgQwh7J1vZpabZ6sgO5UPI1Hq3AWM6m4tpjO0j6ixFllKi7UpNqWGNA39HUvNshhRFICfNvM+Y9Xg4HTLGmZIWM88COU/K1GluMoUbNb/3Fjxmj2Hp8fAWzaAFKTD5ZcYRTBV1b2uz7cQ7dob2WnCE4XgiT7EAyZCBHMU5zSOXb+hUxixKnjlKKFUEEYoztXvRnWu2zfgoe 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 Tue, Jul 11, 2023 at 6:27=E2=80=AFPM Hugh Dickins wro= te: > > 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 > --- > 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. 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 =3D 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. > > mm/userfaultfd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -300,7 +300,10 @@ static int mfill_atomic_pte_poison(pmd_t *dst_pmd, > spinlock_t *ptl; > > _dst_pte =3D make_pte_marker(PTE_MARKER_POISONED); > + ret =3D -EAGAIN; > dst_pte =3D pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl); > + if (!dst_pte) > + goto out; > > if (mfill_file_over_size(dst_vma, dst_addr)) { > ret =3D -EFAULT; > @@ -319,6 +322,7 @@ static int mfill_atomic_pte_poison(pmd_t *dst_pmd, > ret =3D 0; > out_unlock: > pte_unmap_unlock(dst_pte, ptl); > +out: > return ret; > } >