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 C22D1C77B75 for ; Sat, 6 May 2023 16:35:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 185056B007B; Sat, 6 May 2023 12:35:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1353D6B007D; Sat, 6 May 2023 12:35:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 024226B007E; Sat, 6 May 2023 12:35:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by kanga.kvack.org (Postfix) with ESMTP id A58706B007B for ; Sat, 6 May 2023 12:35:52 -0400 (EDT) Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-965e4be7541so345897966b.1 for ; Sat, 06 May 2023 09:35:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; t=1683390951; x=1685982951; 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=iqqqXb22we8OKwm1xpjpxee+QM1A6+Xg9qamTaplXFs=; b=A8DCHkFWUzXdsail7vwybg9mK5VvGIri9rsoNiKAJFar9bylTKMA0jF9anL2QGpTVr 0jXTLxUJukk8P6A/Lcr4EqDeNJ4HRX3saa+RBpJuaPRgQyFIAo3lkI0DflS27CiZKHmk qudhg848Dwf/Qwec/bVWOdbhVOXSafcVN9jo4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683390951; x=1685982951; 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=iqqqXb22we8OKwm1xpjpxee+QM1A6+Xg9qamTaplXFs=; b=j71JtGbLmiz/oaZqSyPOEjR7xpAHkvp+Ay3u6McIl5HWcLtPgGdzBnLVgpIZi1ETJe lzeHzVDXlfGSfZ12oyIr1Vp7V3VN1s99oW73+JEZp6diHK9lvKS1x9tkHrtyIHmFyAZ1 PStZIl6DQ0u3HjsLJqDA+1Zv1Dw6fzxXVhx7ryLdxmrokXzEWSdH89NAxqhmRnTCfMFn 5hG7Bf9k2CQMj+Tp+TC/vwCektmU97Wsmv3xvnVPaY/G4dEVj05qNY2aT4kPIekzyWKT mI7HFpe9IIsqw1MlAEOfidAsB5x7WhOSrFubp3lQlnaips+5QSZHxH00lW6aIfZL7wxh rTIw== X-Gm-Message-State: AC+VfDziMstdFI6fSBYJTq+rKjhN6R2xzkubedhfCNJMGBdenFjxrAyh 60EaqKo3+Vxog52+p0WtOo/lI7LVLd14hlIcHAp76w== X-Google-Smtp-Source: ACHHUZ6W4HVASdKP5nTOvZ/77Eo9r2vqqn5HsyVt9UltIk+dXoU0Qrl5sEVSIwLB651H6DDyjJP9Dw== X-Received: by 2002:a17:907:a41e:b0:965:e556:8f6d with SMTP id sg30-20020a170907a41e00b00965e5568f6dmr4680246ejc.63.1683390951414; Sat, 06 May 2023 09:35:51 -0700 (PDT) Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com. [209.85.208.53]) by smtp.gmail.com with ESMTPSA id cb24-20020a170906a45800b0095fde299e83sm2475256ejb.214.2023.05.06.09.35.50 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 May 2023 09:35:50 -0700 (PDT) Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-50b9ef67f35so5348049a12.2 for ; Sat, 06 May 2023 09:35:50 -0700 (PDT) X-Received: by 2002:a17:907:628c:b0:94f:2a13:4e01 with SMTP id nd12-20020a170907628c00b0094f2a134e01mr4183212ejc.74.1683390949920; Sat, 06 May 2023 09:35:49 -0700 (PDT) MIME-Version: 1.0 References: <20230506160415.2992089-1-willy@infradead.org> In-Reply-To: From: Linus Torvalds Date: Sat, 6 May 2023 09:35:33 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] filemap: Handle error return from __filemap_get_folio() To: "Matthew Wilcox (Oracle)" Cc: Andrew Morton , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Dan Carpenter , syzbot+48011b86c8ea329af1b9@syzkaller.appspotmail.com, Christoph Hellwig Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 6, 2023 at 9:09=E2=80=AFAM Linus Torvalds wrote: > > Thanks, applied directly. Actually, I take that back. I applied it, and then I looked at the context some more, and decided that I hated it. It's not that the patch itself was wrong, but the whole original if (folio) folio_put(folio); was wrong in that context, and shouldn't have been done that way. Converting the conditional to use !IS_ERR() fixes a conversion error, but doesn't fix the original problem with the code. The only path that triggered that "we have no folio" was wrong to jump to that "should I drop this folio" code sequence in the first place. So the fix is to not use "out_retry" at all for the IS_ERR(folio) case. Yes, yes, compilers can do jump threading, and in a perfect world might realize that there are two consecutive tests for IS_ERR(folio) and just do this kind of conversion automatically. But the fact that compilers *might* fix out our code to do the right thing doesn't mean that the code shouldn't have been written to just have the proper error handling nesting in the first place. And yes, the simplest fix for the "wrong test" would be to just add a new "out_nofolio" error case after "out_retry", and use that. However, even that seems wrong, because the return value for that path is the wrong one. So the "goto out_retry" was actually *doubly* wrong. If the __filemap_get_folio(FGP_CREAT) failed, then we should not return VM_FAULT_RETRY at all. We should return VM_FAULT_OOM, like it does for the no-fpin case. So the whole if (IS_ERR(folio)) { if (fpin) goto out_retry; sequence seems to be completely wrong in several ways. It shouldn't go to "out_retry" at all, because this is simply not a "retry" case. And that in turn means that "out_retry" shouldn't be testing folio at all (whether for IS_ERR() _or_ for NULL). So i really think the proper fix is this (whitespace-damaged) patch instead= : --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3291,7 +3291,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf) vmf->gfp_mask); if (IS_ERR(folio)) { - if (fpin) - goto out_retry; filemap_invalidate_unlock_shared(mapping); + if (fpin) + fput(fpin); return VM_FAULT_OOM; } @@ -3379,6 +3379,5 @@ vm_fault_t filemap_fault(struct vm_fault *vmf) * page. */ - if (folio) - folio_put(folio); + folio_put(folio); if (mapping_locked) filemap_invalidate_unlock_shared(mapping); which fixes both problems by simply avoiding a bogus 'goto' entirely. Hmm? Linus