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 04246C77B7D for ; Sat, 20 May 2023 09:14:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8592C280001; Sat, 20 May 2023 05:14:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7E1FB900003; Sat, 20 May 2023 05:14:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 683FB280001; Sat, 20 May 2023 05:14:55 -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 5A9BC900003 for ; Sat, 20 May 2023 05:14:55 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 32740140B41 for ; Sat, 20 May 2023 09:14:55 +0000 (UTC) X-FDA: 80810073750.23.E016DCF Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by imf07.hostedemail.com (Postfix) with ESMTP id 5372740012 for ; Sat, 20 May 2023 09:14:53 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=oOl3jkJ6; spf=pass (imf07.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 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=1684574093; 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=p26w75jnOzAEdo+Byo9aO1qQn64wYCOl3bY13+vxNmk=; b=48Jre3wIi+O3nG/k0Vww7ArtfbqRzOtQGeogAaBygputeG3gZ0LHzBnH5dCgNA0mDZcsmI hlb6qbvJ/up0F2npi0Xz2ZcqYWqePM5puElPbp2wevR70OmGfwJ99sTInk4Hp4JMfnyCp5 W4OUozp9Z18pnqzEVT5mMneDwu9+d1U= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=oOl3jkJ6; spf=pass (imf07.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684574093; a=rsa-sha256; cv=none; b=LVeOyKakNoFGV36mUD3KPAXCB2gzYFT27EtJKi38YKTeUKplFp7RdEG4Rl4b2aqgONfixJ Rh5u5ESOiN3l73uV3Bxg0DoM26jmoYjAagJR4U1SOXonkR64jh3t9bwkDkww9dXZx0Blce 1KXOruq1Y+Aw7TJ4ghatWCvryck3C3U= Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-3063433fa66so2563681f8f.3 for ; Sat, 20 May 2023 02:14:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684574092; x=1687166092; 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=p26w75jnOzAEdo+Byo9aO1qQn64wYCOl3bY13+vxNmk=; b=oOl3jkJ6SD5TY56pKAFFeCXwzDR/dT6qZPAb1m7wFYg/h9zOoYhU7nDbmXo0+iqqrR 2sUrZXzOioUEEXZEJvSlHfHYzbOVmnOM7Rx/ANRsUAX3xWe7sfrGJPy34HpJBLxEorpd M8J78EuPVkYsaA70esysc623hyLmkF3qvl2b6xwZEZsRzDlKSEB2FvgN375kCzzSHmh6 jXbozkqpMnPyaZwwKPTCCb6fsNyclb52LFRqgi8VJkY85sLrAhCQRSZGe4h6ZjzY/7s1 fEQsQCZDvusqEObIGOmEMnuziyDw5qXkCxJ+GRVrfaaVu0Qgs7vYIv6xNit10F44NfWX VZ0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684574092; x=1687166092; 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=p26w75jnOzAEdo+Byo9aO1qQn64wYCOl3bY13+vxNmk=; b=PkIZJuX05twKSNL+zFKBGYndshFRCy/8Wo3PPj8xCz7cxSD80n2OSyR5G8ZoK9CIvg l21hRLWhLHej2xVK14NokAkVPsHQDgVzva2WHVA1iO5AJZ+zXlNNvlAkjDStvo0/zXxe qNMpfjC4tRzU4Vd0+HJ6RP4qGzun7QJuxqBTaI9B5JsCBHJEYxPvIbD7slWE+7jXMf6j PsDYk+5m1LTAbVmpQjwc16AzkpdZOym4X5D+MsfejMAgKkRCiDtmDCr1TW2i6G8SGiMf WyVItXDoIRr19Knblkw3LXt3i1WWEiE2wmIp+PNkzulnDXW1R+Fv69Z3X0XCqpY4AzrC EMaA== X-Gm-Message-State: AC+VfDzGeMEKxlR7M6pBX2Q57Mf1dXcT3eHZSDuIxGNtQDHkFlNyB/m0 vFHi2SSV8O6WKrY5WGZYOD4= X-Google-Smtp-Source: ACHHUZ71oQFelW3p0VPKhNo3IJWJEBFsEgvGe0d+bIIqgHevklfCmsbpUHXpFr1U8wMuimNDrlbMKg== X-Received: by 2002:a5d:4385:0:b0:309:436a:fc2f with SMTP id i5-20020a5d4385000000b00309436afc2fmr2826502wrq.57.1684574091429; Sat, 20 May 2023 02:14:51 -0700 (PDT) Received: from localhost (host81-154-179-160.range81-154.btcentralplus.com. [81.154.179.160]) by smtp.gmail.com with ESMTPSA id j2-20020adfff82000000b00304832cd960sm1401962wrr.10.2023.05.20.02.14.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 May 2023 02:14:50 -0700 (PDT) Date: Sat, 20 May 2023 10:12:40 +0100 From: Lorenzo Stoakes To: Jason Gunthorpe Cc: Arnd Bergmann , 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: <0bc9dc2b-0da6-4d5c-96af-e21aa287eecb@lucifer.local> References: <20230519093953.10972-1-arnd@kernel.org> <5b071f65-7f87-4a7b-a76a-f4a1c1568ae7@lucifer.local> <1ca47b8a-292c-47ab-aa6f-ca24fdfc0d3c@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 5372740012 X-Rspam-User: X-Stat-Signature: fnwma8toyc3i9fjcng3x41u8jeputng6 X-Rspamd-Server: rspam01 X-HE-Tag: 1684574093-358432 X-HE-Meta: U2FsdGVkX1/TDB3Da50Mb1CvaEUpIBkh4CHDSdLR0Dv3dPEXSHYYYaQlyljf1eLz7W5MAXL7rScRNek0hPVC1/RXSXRvKv5zM0OPT5Y2u4uSgFm/RQxrm3MAtzuTVtoI8fRtPl44zuqjOHqFO2IDg0OcQ+iuDTedToD48d3hBMXhf506CcKpf6KUmdHO7BcxQ721fu//8YZIgUHS8BiGOhpS22ceX7ZMehkWGf7OUlKUkvln7CN5rZWdYLFtO1KwB5YWOf81HSx8JZUJUKFzJzCD+y3IWv94pkWdlSk2QK2RFHA9jXhwD/CQx+UuNuIWj0AGr9welil+mHsU9ikNBlKxI091MeRbLxr01A6k20l8i3bPG+e5ICRhI7KlyoRchew9B5MWMxyclPAGguMjL4AOPqhSaR7rvSPT6Qx+gxD86P/94TP7JyL+KsaBAdA23dk7TrSG3UEXJJCpq3qteOFh6jeFzNobNWfyCzpnN1JEFwBmupLO6fHTkwBWK+//xSD7yFWS3h39iYwUsvexgRZo/aeKhSV7rRDnWjB9sLo3GTcJWfO4PlPTrhe5yas/cvEb+TQT/quHzBaNmxCd9oUnncewWsJyFjZesm8/aFgOTEy6xRo3yVqOsK3PLygRKcZ3SA+7YrEAKHOv83/io7L09mBp55ThR1EU4aXbX1kkA0jZNq92wLWHg1YAgtR9iixDV4RMutGZWCykuKmI5C+fpHDo5+IseX1MImANg40HIDa4FmVTmaBtglfqXA8DoU3ycov/hodIrUMcWexex1QdO3T/wjN80nYNx2EHwyF9iH/xCz1ms4yQ5pRYMsYN9ecpMmxQcsFkx2L9ZXZe3072QId5w99xcl8EinwvuGuaIiJCPHNt7O3K4YMUzJGaITOBwni6pkEC8DYQU7Mj8tT2jyMiQ8zNi31q7NNgGHUsboZEaW0vyS+xRPePlqJebJepEke+yxekgxkx+MX upGm3WPx RZ+9SzbE+X5cvlJZcsvhIYIdAVaQhlPMZEqwnXQn+8aKIReuxanwwpheYeL810QQcU/Rk9QGlupztjT4v9XKSNC2GJBQKWUBhxM9JWgjJviidwEZ4k99tO4II4fSPCtrHk46pjvYLhhJVJAOdcpGbPXcC/C/xL9qv77sJ7Oh4+KG2B6TybTkDzXWl2/2jcUfUBbeoUKKskd+yLgOffSd338zXcfbFHHfDqKJnnOVVJgyylOlin9qRuf9UXIr1z1NXfQT1Sf54SRxKcBMdQcq7aoJVMBV8Itp6Q9tTndSZsjumwUlBihOW4D/dwg4JKQBR9V5KEVpcGJ9TppCI4lnBn9+R/tGrakXGT6kbw9rhheN9lhwFuZEIBsgnE8zSuRefW/hNkUwIxDulOqBBjLtdh7G1hN18TbT7pAx8vfmOtgDRmyPmPHfVIFPn+w== 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 Sat, May 20, 2023 at 05:25:52AM -0300, Jason Gunthorpe wrote: > On Sat, May 20, 2023 at 06:19:37AM +0100, Lorenzo Stoakes wrote: > > On Fri, May 19, 2023 at 07:17:41PM -0300, Jason Gunthorpe wrote: > > > On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote: > > > > 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. > > > > > > No, there really isn't. > > > > > > Either it pins the page or it doesn't. Returning "NULL" to mean a > > > specific kind of failure was encountered is crazy.. Especially if we > > > don't document what that specific failure even was. > > > > > > > It's not a specific kind of failure, it's literally "I didn't pin any > > pages" which a caller may or may not choose to interpret as a failure. > > Any time gup fails it didn't pin any pages, that is the whole > point. All that is happening is some ill defined subset of gup errors > are returning 0 instead of an error code. > > If we want to enable callers to ignore certain errors then we need to > return error codes with well defined meanings, have documentation what > errors are included and actually make it sane. Yeah I agree it's not exactly great that a failure to pin can be considered an ordinary case, but I don't think a wrapper function is where we should be trying to fix this. In fact this patch isn't even fixing it, it's treating EIO as a success case for the (possibly broken) uprobe case. I think we are at the wrong level of abstraction here, basically. > > > That can be a reason for gup returning 0 but also if it you look at the > > main loop in __get_user_pages_locked(), if it can't find the VMA it will > > bail early, OR if the VMA flags are not as expected it'll bail early. > > And how does that make any sense? Missing VMA should be EFAULT. Yeah missing VMA doesn't really make sense since we invoke the function with the mmap lock held (it _could_ make sense if you were calling it via one of the unlocked functions, speculatively, though how sensible doing that is another discussion...) > > > caller differentiates between an error and not being able to pin - > > uprobe_write_opcode() - which treats failure to pin as a non-error state. > > That looks like a bug since the function returns 0 on success but it > clearly didn't succeed. The code is specifically handling a failure-to-pin separately - set_swbp() -> uprobe_write_opcode() -> install_breakpoint() explicitly does the following:- ret = set_swbp(&uprobe->arch, mm, vaddr); if (!ret) clear_bit(MMF_RECALC_UPROBES, &mm->flags); So even if this is... questionable, the code literally does want to differentiate between an error and a failure to pin. Presumably this is because of the flag check, but yeah we should be differentiating between sub-cases. > > > Also if we decided at some point to return -EIO as an error suddenly we > > would be treating an error state as not an error state in the proposed code > > which sounds like a foot gun. > > No, this returning 0 on failure is a foot gun. Failing to pin a single > page is always an error, the only question is what sub reason caused > the error to happen. There is no third case where it is not an error. > > Jason The uprobe path thinks otherwise, but maybe the answer is that we just need to -EFAULT on missing VMA and -EPERM on invalid flags. I could look into a patch that tries to undo this convention, and then we could revisit this for the wrapper function too.