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 829F6C77B75 for ; Fri, 19 May 2023 14:51:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E8AA1900004; Fri, 19 May 2023 10:51:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E3AD1900003; Fri, 19 May 2023 10:51:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CDCC1900004; Fri, 19 May 2023 10:51:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id BBF1A900003 for ; Fri, 19 May 2023 10:51:57 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 80AD8160AE5 for ; Fri, 19 May 2023 14:51:57 +0000 (UTC) X-FDA: 80807294274.16.73D220F Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf16.hostedemail.com (Postfix) with ESMTP id 89351180006 for ; Fri, 19 May 2023 14:51:55 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=VPDps51p; spf=pass (imf16.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1684507915; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZMQZECFtXym11ziYXl+wIoNNwzMeR1Bxy8iSX/Q997A=; b=HBXKS26qbvg3L/qDC4pulIQ5n5yb4EsT283C+/Y1KLtAYvAq6BqirgX8w5ZgXOY8c3/zFO lWCIxC8uwYrs5aTzcI+Bdl4OKrnumKLNa7uDdFHcEMNGoim3FjSKs0+tEXZVPtEbi4kN7u btnjRZ/r+gK2LXUUc864CLEgaaa3tf4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684507915; a=rsa-sha256; cv=none; b=2ZDLUI3GwyJJ6oLB2CjVt8FhI0ondNuElmnCKMD9M0Pzdx+xj7l0ml/wtF7BPr5NL56KIG +2OZ/a4PdXvtUuGQ/+Ooeldlj2552y0adwl4A5Ure+ge89x0MINS8yshD4JHYAVbPZP/b9 pTxu4hE5sSU2RkJo5qyxh3OT1t2eYDE= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=VPDps51p; spf=pass (imf16.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-3f417ea5252so23231075e9.0 for ; Fri, 19 May 2023 07:51:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684507914; x=1687099914; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ZMQZECFtXym11ziYXl+wIoNNwzMeR1Bxy8iSX/Q997A=; b=VPDps51pUY/EJFHxafPUzqLLQCefvQWUOZysrtOX/RzXO060iImM98vSPbI4oyXa04 BCHUKWnNwi3h8bw/86pZYipH+lVfrBYqOfNJugL55cJuGQRV97p3NZWLoYbe/ZyTDDDZ eGKWC2MF68BfcvWuz5P5H2FZ8s/fOXyPNoNlVYZc6mwmG4wWhfiLut2Ad6qjlr3TrrOE 7gnX+tCpMafN61s7aA2ngKLyuNMifrbogrS7bkjBvQgYAXb1aZrg5eSPT5zXPzZ3hzcj i2K/9x9/U/9LN6LiNB/fiIVO0OFGsvZkmbsEVLFdKUEAf9cfmVjDQQkrQMLrjP5XEuui hFlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684507914; x=1687099914; h=in-reply-to: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=ZMQZECFtXym11ziYXl+wIoNNwzMeR1Bxy8iSX/Q997A=; b=PdEAVBLaSrFNqLjCH5PLaZgZWZtUNBusv85jnuYTtR1nz226No6UsNrTBZKxHWuSf+ Ig9oyf0wsS4jvgFi3XMP5wYPZjeFY3n8rLHL0Udq3vV1x3/VvNpF53KkP3/jEkJZcBRe p1j0FYzez7P4g8+xezp4Z3vhnqn98e3DLHm4AjfYtaa51BEdX4bh+zT65AdaqDGOS3Td OqDenFwMV37qFlrORCN/6P64QusaxFudw07tnV7v56tKHClO5AYqexjnBqCzpY0tfyhS ocwixSeOKN2+uwik4X8JqSmqFlXytTk6lLjkhsNsTHLRFUaJjta9eYnJpe0+TyoJJmj2 DGEg== X-Gm-Message-State: AC+VfDxnCGHHgRrlVjCzrRFmHrzSzYk23656q++PTZ8IXwRprzT+oHaG xE+atAASrRdLLA1WPi6OWto= X-Google-Smtp-Source: ACHHUZ41Leun+h+YErHotZC3zd8Np65RkNQeBYIROMArGqFz6AajgszDwsnVl4ekPuFpw5pJrEFoPA== X-Received: by 2002:a5d:4811:0:b0:307:9a49:da89 with SMTP id l17-20020a5d4811000000b003079a49da89mr1642332wrq.38.1684507913493; Fri, 19 May 2023 07:51:53 -0700 (PDT) Received: from localhost (host81-154-179-160.range81-154.btcentralplus.com. [81.154.179.160]) by smtp.gmail.com with ESMTPSA id 18-20020a05600c229200b003f42d8dd7d1sm2691633wmf.7.2023.05.19.07.51.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 May 2023 07:51:52 -0700 (PDT) Date: Fri, 19 May 2023 15:51:51 +0100 From: Lorenzo Stoakes To: Arnd Bergmann Cc: Andrew Morton , Arnd Bergmann , Catalin Marinas , Will Deacon , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Adrian Hunter , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-perf-users@vger.kernel.org Subject: Re: [PATCH] [suggestion] mm/gup: avoid IS_ERR_OR_NULL Message-ID: <5b071f65-7f87-4a7b-a76a-f4a1c1568ae7@lucifer.local> References: <20230519093953.10972-1-arnd@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230519093953.10972-1-arnd@kernel.org> X-Stat-Signature: meoyjnzj85jgys99aputbyngnb531946 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 89351180006 X-Rspam-User: X-HE-Tag: 1684507915-217335 X-HE-Meta: U2FsdGVkX1+ODZQlvV/IUR3OOn92BssBr/9mjtnWW7dTDtRB/DH16uqlWlw7gz+wxm+Hja5gIgFYZZRWAUyAGlFp27+GQGOyxCeRcCPI5wSuFZfXJ4TkqFSUoQ5yfFm6Plg5ON++EvqgFdut04vQNTVUlqvnIaPIpd6Tu5qbEROrUorZdpDZNwB3bW9dAesF/6APckxcLrTQKMUNYCO+vPrXRN4fjzLddkWWepK8ms/a2VCEhKc6pqPrXe+n4OEExMJ3drTLX3oFKiwr3Ver/sbhd67Q/Ww/fveDvpjIuHlfXAJpJCDVDOOdMkU9xzfXgZUuyhYIrZ1TdDzf6q5D0aZyH9sk3pHRUTQ8wcYrjymeXPxdSGHFSHyLMu8SR6vdp09GypHPUumy52LLeYAmGyzglmJTLKi9E0xy6y9OYdE4R/MKJZO44COd9hoCHt/s22dIlFkrabLzE9fmK39bObA5fy5vszlmp5Uvnm+EdBGqJTIuH7QKcjfItm4Ei4R4cabT2gj15G9tqOjrKM9cx34r+LMYp/jWdUXxhmme2JwlCCfwBA2x2Th30MOZ7/ufSuY2Q6UrptoyUpCEydob24ZfIJkNHnNnM/VbY+NgkSuGdhs/DNFhdIB5xZU6Rr4YicyZ7fjAIRzfIADeFBcfOxuppKCQwfPIizrtONi5qTiv5PSijG8l9aKQh6yt1sM9fp5z0IC0oDpLnRXc++2gIYANyq7Llo5K+pZS8MAKsCh16kXMzMZd/+gihu+SqKZp51zw8JRPSvNcrZaizNkLmIeV3+2Kkj92NP3B6KGK9u5c7ejwSNFBJxfsXzGdeMc+X2KL7keFF8w0KY4VP09SicnUjt2it/trb1wnDet2aHLfddDaTyUZeTrDcO3GKYnDi2Mid5y7ky4MLJYF4mB8T7M0Xr02ruEWrzp+l4HR4AYirKZw0WYPkpFW7m47+xR/E5XKufUP4JlwHnRlCWC Z02G/Ncy 7I1KNc75cnEHAZ9uuwi2bBAly6wnR1lzquQuwBtVJvKwpTo3P44A8cd9bS1I3wFKNhRmhEa4FewCgS87nUmwvjngimRnZ32IkrrxvatSxN+1ORkPlpxjrjWjJV9MJTNKTaoin7w4a/JLqMDB54RxrKpen2YccsHGSpWbjJhMhy/aKAKso9tC0kju1EYNRLFrsepxOBksO+lSLo9vjM8aDhPRQLgdMDOOtvZLezHDBLdEtTiKFPjPmdZXHgCb6c9fo0xFr1++AuftDQj/Xw6MEZjeTaab8DWS+MvV8/WIGR09/ZKznc0MBI1CJbYxMeHpiwVTC4yconJYdVn9rPb8UETWDD5/xQbJNwoZuc2hDRgpqFuab489CTepq7CrnPsDuFgHn2ZjZdWFYEtzEqs46aW5iWtz6s3nXeS/5HNTM6V9saqhfTEKfBvZCXA== 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: Given you are sharply criticising the code I authored here, is it too much to ask for you to cc- me, the author on commentaries like this? Thanks. On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann > > While looking at an unused-variable warning, I noticed a new interface coming > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad > interface design and is usually surprising to users. I am not sure I understand your reasoning, why does it 'tend to indicate bad interface design'? You say that as if it is an obvious truth. Not obvious to me at all. There are 3 possible outcomes from the function - an error, the function failing to pin a page, or it succeeding in doing so. For some of the callers that results in an error, for others it is not an error. Overloading EIO on the assumption that gup will never, ever return this indicating an error seems to me a worse solution. > > Change get_user_page_vma_remote() to return -EIO when no pages were > found and adapt the callers to match. > > Fixes: eca1a00155df ("mm/gup: remove vmas parameter from get_user_pages_remote()") > Signed-off-by: Arnd Bergmann > --- > I see the real bug is already fixed, but this seemed worth pointing out still. > Not sure if this is the best way to handle the return types here, but the version > in linux-next doesn't look great either. > --- > arch/arm64/kernel/mte.c | 4 ++-- > include/linux/mm.h | 2 +- > kernel/events/uprobes.c | 5 ++++- > mm/memory.c | 2 +- > 4 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index 4c5ef9b20065..6983ba35ce16 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -434,8 +434,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > struct page *page = get_user_page_vma_remote(mm, addr, > gup_flags, &vma); > > - if (IS_ERR_OR_NULL(page)) { > - err = page == NULL ? -EIO : PTR_ERR(page); > + if (IS_ERR(page)) { > + err = PTR_ERR(page); > break; > } > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 42ff3e04c006..4bb172e4818c 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2397,7 +2397,7 @@ static inline struct page *get_user_page_vma_remote(struct mm_struct *mm, > if (got < 0) > return ERR_PTR(got); > if (got == 0) > - return NULL; > + return ERR_PTR(-EIO); > > vma = vma_lookup(mm, addr); > if (WARN_ON_ONCE(!vma)) { > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index cac3aef7c6f7..9cf2d4ba760e 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -474,7 +474,10 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > gup_flags |= FOLL_SPLIT_PMD; > /* Read the page with vaddr into memory */ > old_page = get_user_page_vma_remote(mm, vaddr, gup_flags, &vma); > - if (IS_ERR_OR_NULL(old_page)) > + if (old_page == ERR_PTR(-EIO)) > + return 0; > + > + if (IS_ERR(old_page)) > return PTR_ERR(old_page); I hate this, you're now using an error to indicate a non-error state. Also you have no idea whether get_user_page_vma_remote() has encountered an error condition returning -EIO rather than not pinning anything so this could also be broken. > > ret = verify_opcode(old_page, vaddr, &opcode); > diff --git a/mm/memory.c b/mm/memory.c > index 8358f3b853f2..f9a81278e76d 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5604,7 +5604,7 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, > struct page *page = get_user_page_vma_remote(mm, addr, > gup_flags, &vma); > > - if (IS_ERR_OR_NULL(page)) { > + if (IS_ERR(page)) { > #ifndef CONFIG_HAVE_IOREMAP_PROT > break; > #else > -- > 2.39.2 > Not a fan at all of this patch, it doesn't achieve anything useful, is in service of some theoretical improvement, and actually introduces a new class of bug (differentiating EIO and failing to pin).