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 X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56CE8C433E0 for ; Sat, 13 Mar 2021 19:24:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B948964ED1 for ; Sat, 13 Mar 2021 19:24:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B948964ED1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 125E16B0006; Sat, 13 Mar 2021 14:24:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0D65E6B006E; Sat, 13 Mar 2021 14:24:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E90FA6B0070; Sat, 13 Mar 2021 14:24:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0116.hostedemail.com [216.40.44.116]) by kanga.kvack.org (Postfix) with ESMTP id CC14B6B0006 for ; Sat, 13 Mar 2021 14:24:01 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 7BF19180AC17D for ; Sat, 13 Mar 2021 19:24:01 +0000 (UTC) X-FDA: 77915826282.13.30C86AF Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf13.hostedemail.com (Postfix) with ESMTP id ABBC2E0011C0 for ; Sat, 13 Mar 2021 19:24:00 +0000 (UTC) Received: by mail-lf1-f52.google.com with SMTP id k9so50717439lfo.12 for ; Sat, 13 Mar 2021 11:24:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fkMUwflg0okgtiiFHyBwNautlkx4oxTesZnPR+Zbdm0=; b=bSs9U8+fE70nTPl25+3Ah15euLoLlf6KJftxZU0Bk7mfBawqk4ClAgFvihRohYSlmf Q8cF7V7jozp1ZSnDK19SRHNVt+iv9oUAO1jA+nrFirHrwpiRa8mdMZuuUFq7BPWS87Wd gxP/e6JcYnycxahzA1sONPqIC7MV8++JX76Kg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fkMUwflg0okgtiiFHyBwNautlkx4oxTesZnPR+Zbdm0=; b=ccBfeQptdOnINq+qyJ5rVlonz+ERpILSwc8hzK7PoD+QzQlDXmo8minV9DDbSb/Q2E 7gwYCT78o9Mi24oM3EOxOuaVg8XHTbhefLMntUlPeNz0/27+qwDyxRnbdWlOllOVYr1B qyumlcf+fqsukyQQNXFzRGQy4grNarjkye35aOxHS8HuJQnWjyK2bEu+i0htSK9/lSK1 x7rH65IVjhhoL0l+WqURPd/7386cXr1HNGLJSqpCu7NBTqo/FsyPKy7MptNi7OHWKlJV pp9dUDMI/rMxkKNf2t42fgtunGgPrRff/9yGbT5CMFH1R+eYSmWujxP7dD/jXXLQbO7i TIcg== X-Gm-Message-State: AOAM5307iU4dUEU7kp12HxwF3qxs7s5nT3tMJ9qd0SfGXeXU4sJD03Oq XGN3vUA5s9Pgw9ow+p5QguTjtVl13lBgxg== X-Google-Smtp-Source: ABdhPJzG2F5DP3p8Kvc6auiEAdZlA+ZeHehhmw+Ivk8aAtjEgpXDHgffvhK1vwxi2YXan/DEajcHYQ== X-Received: by 2002:a05:6512:2191:: with SMTP id b17mr3299465lft.267.1615663438434; Sat, 13 Mar 2021 11:23:58 -0800 (PST) Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com. [209.85.167.52]) by smtp.gmail.com with ESMTPSA id a26sm2331897ljm.107.2021.03.13.11.23.57 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 13 Mar 2021 11:23:57 -0800 (PST) Received: by mail-lf1-f52.google.com with SMTP id r3so42561140lfc.13 for ; Sat, 13 Mar 2021 11:23:57 -0800 (PST) X-Received: by 2002:a05:6512:33cc:: with SMTP id d12mr3118409lfg.487.1615663436752; Sat, 13 Mar 2021 11:23:56 -0800 (PST) MIME-Version: 1.0 References: <20210312210632.9b7d62973d72a56fb13c7a03@linux-foundation.org> <20210313050820.EoPGLS3gj%akpm@linux-foundation.org> In-Reply-To: <20210313050820.EoPGLS3gj%akpm@linux-foundation.org> From: Linus Torvalds Date: Sat, 13 Mar 2021 11:23:40 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [patch 23/29] mm, hwpoison: do not lock page again when me_huge_page() successfully recovers To: Andrew Morton Cc: aneesh.kumar@linux.vnet.ibm.com, Linux-MM , Michal Hocko , mm-commits@vger.kernel.org, naoya.horiguchi@nec.com, Oscar Salvador , stable , Tony Luck Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: ABBC2E0011C0 X-Stat-Signature: 7bxgg48n9pd63qjxii5bfyirm6qyxymq Received-SPF: none (linuxfoundation.org>: No applicable sender policy available) receiver=imf13; identity=mailfrom; envelope-from=""; helo=mail-lf1-f52.google.com; client-ip=209.85.167.52 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1615663440-197234 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 Fri, Mar 12, 2021 at 9:08 PM Andrew Morton wrote: > > From: Naoya Horiguchi > Subject: mm, hwpoison: do not lock page again when me_huge_page() successfully recovers This patch can not possibly be correct, and is dangerous and very very wrong. > out: > - unlock_page(head); > + if (PageLocked(head)) > + unlock_page(head); You absolutely CANNOT do things like this. It is fundamentally insane. You have two situations: (a) you know you locked the page. In this case an unconditional "unlock_page()" is the only correct thing to do. (b) you don't know whether you maybe unlocked it again. In this case SOMEBODY ELSE might have locked the page, and you doing "if it's locked, then unlock it" is pure and utter drivel, and fundamentally and seriously wrong. You're unlocking SOMEBODY ELSES lock, after having already unlocked your own once. Now, it is possible that this kind of pattern could be ok if you absolutely 100% know - and it is obvious from the code - that you are the only thread that can possibly access the page. But if that is the case, then the page should never have been locked in the first place, because that would be an insane and pointless operation, since the whole - and only - point of locking is to enforce some kind of exclusivity - and if exclusivity is explicit in the code-path, then locking the page is pointless. And yes, in this case I can see a remote theoretical model: "all good cases keep the page locked, and the only trhing that unlocks the page also guarantees nothing else can possiblly see it". But no. I don't actually believe this is fuaranteed to the case here, and even if it were, I don't think this is a code sequence we can or should accept. End result: there is no possible situation that I can think of where the above if (PageLocked(head)) unlock_page(head); kind of sequence can EVER possibly be correct, and it shows a complete disregard of everything that locking is all about. Now, the memory error handling may be so special that this effectively "works" in practice, but there is no way I can apply such a fundamentally broken patch. I see two options: - make the result of identify_page_state() *tell* you whether the page was already unlocked (and then make the unlock conditional on *that*) This is valid, because now you removed that whole "somebody else might have transferred the lock" issue: you know you already unlocked the page based on the return value, so you don't unlock it again. That's perfectly valid. - probably better: make identify_page_state() itself always unlock the page, and turn the two different code sequences that currently do res = identify_page_state(pfn, p, page_flags); out: unlock_page(head); return res; into just doing return identify_page_state(pfn, p, page_flags); out: unlock_page(head); return -EBUSY; instead, and move that now pointless "res" variable into the only if-statement that actually uses it (for other things entirely! It should be a "enum mf_result" there!) And yes, that second (and imho better) rule means that now {page_action()" itself needs to be the thing that unlocks the page, and that in turn might be done a few different ways: (a) either add a separate "MF_UNLOCKED" state bit to the possible return codes and make that me_huge_page() that unlocks the page set that bit (NOTE! It still needs to also return either MF_FAILED or MF_RECOVERED, so this "MF_UNLOCKED" bit really should be a separate bitmask entirely from the other MF_xyz bits) (b) make the rule be that *all* the ->action() functions need to just unlock the page. ( suspect (b) is the better model to aim for, because honestly, static code rules for locking are basically almost always superior to dynamic "based on this flag" locking rules. You can in theory actually have static tooling check that the locking nests correctly with the unlocking that way (and it's just conceptually simpler to have a hard rule about "this function always unlocks the page"). But that existing patch is *so* fundamentally wrong that I cannot possibly apply it, and I feel bad about the fact that it has made it all the way to me with sign-offs and reviewed-by's and everything. Linus