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 1FB7DC2BA4C for ; Wed, 26 Jan 2022 18:44:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A8D1B6B0071; Wed, 26 Jan 2022 13:44:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A3C9E6B0073; Wed, 26 Jan 2022 13:44:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 903FC6B0074; Wed, 26 Jan 2022 13:44:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0159.hostedemail.com [216.40.44.159]) by kanga.kvack.org (Postfix) with ESMTP id 7F1426B0071 for ; Wed, 26 Jan 2022 13:44:01 -0500 (EST) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 3C6A88249980 for ; Wed, 26 Jan 2022 18:44:01 +0000 (UTC) X-FDA: 79073312682.30.80447E0 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by imf22.hostedemail.com (Postfix) with ESMTP id CB466C0005 for ; Wed, 26 Jan 2022 18:44:00 +0000 (UTC) Received: by mail-ej1-f48.google.com with SMTP id ka4so413415ejc.11 for ; Wed, 26 Jan 2022 10:44:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=xhMYYbSJZTw22nCa/vVztqO0cHAPZe/cx7Ub7sfyKHM=; b=OypkHNl/QZB3tAuPAy71GYYwokVZsZhgIQhWbQa4MyKDx9k8grUA4ckm+Ii68YAsMJ exTVYHl8HptKpwEDgkd6z+ULRg2sJFi2d7zy5hcfheW+ypaBk4rMxtunIvjZ5MipeX2J QJkIIgSZeq1v5vXlTY8z7sJ9LSvW5tfTYyd6cgVs1WAPFeuARaTD8eUy5zGk1tZYgotY PiIousOzS3Cj/ppcG9eClwrMwN76JsC2/eRkKNAowO65t/FYzOrRST5mUb8R2U5/LCcI e88gEsJC9+5DYo7s8KHhHncEFexu3Xy0tFqcoq8qFQKMtz7JtOoOhAzNu2e7jiFhBKUf XZMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=xhMYYbSJZTw22nCa/vVztqO0cHAPZe/cx7Ub7sfyKHM=; b=wLse3W5VL+ucoV9j8zhtH8/HzuwZKLy7GrTSnAxBZ8J/n7s0hVesuf7WliqREuzwUQ kaFaHLCb4350+oteG9/pGKHzdbzzXyJh6TzTc8ivOUek7rJ+jx8q+mlKHM/r614JCx9O gtwXLcN1S9GXQ4Wynfx+dEy2G2wvjYVZrgjlxmhhBvps2T83skEMTqMJwlcXz7AdQscA 3xV4TsJs733iAhy11BlS2OlfzAT31z+t4RZMYosucvFyMISNCSpBMuXGEFf1VejAwyr2 1FHBEo5AgU8PeOb+nyDaVEXhLBFS0UAl4qXRc2kZjsKi8hVnncrvxZUuZAGNAI2HAXUp cC4A== X-Gm-Message-State: AOAM533sYGvkiwBhVrCtO0fmPlDpTlJot1oYHxaiFJyQOdhEGZzDpZP0 fgA7uDDSeA1BenKaXzh8wbN9XPDabsmjN8Pa2VE= X-Google-Smtp-Source: ABdhPJwjkwANIA7P2TAchz64JmzTOs7F9nutdH7UV5Cr94JxsU8lvCDSv3GQMoEe801HHkLVoT/aWn+tS2QpPzm9Ebo= X-Received: by 2002:a17:907:6089:: with SMTP id ht9mr63061ejc.612.1643222639502; Wed, 26 Jan 2022 10:43:59 -0800 (PST) MIME-Version: 1.0 References: <20220120202805.3369-1-shy828301@gmail.com> <5b4e2c29-8f1a-5a68-d243-a30467cc02d4@redhat.com> <5a565d5a-0540-4041-ce63-a8fd5d1bb340@redhat.com> In-Reply-To: <5a565d5a-0540-4041-ce63-a8fd5d1bb340@redhat.com> From: Yang Shi Date: Wed, 26 Jan 2022 10:43:47 -0800 Message-ID: Subject: Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration entry To: David Hildenbrand Cc: Jann Horn , "Kirill A. Shutemov" , Matthew Wilcox , Andrew Morton , Linux MM , Linux Kernel Mailing List , stable Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: CB466C0005 X-Stat-Signature: dq7ir53b77b3aaasijojjuhi5xgtsxrb Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="OypkHNl/"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf22.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.48 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspam-User: nil X-HE-Tag: 1643222640-954518 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 Wed, Jan 26, 2022 at 8:58 AM David Hildenbrand wrote: > > On 26.01.22 17:53, Yang Shi wrote: > > On Wed, Jan 26, 2022 at 3:57 AM David Hildenbrand wrote: > >> > >> On 26.01.22 12:48, Jann Horn wrote: > >>> On Wed, Jan 26, 2022 at 12:38 PM David Hildenbrand wrote: > >>>> On 26.01.22 12:29, Jann Horn wrote: > >>>>> On Wed, Jan 26, 2022 at 11:51 AM David Hildenbrand wrote: > >>>>>> On 20.01.22 21:28, Yang Shi wrote: > >>>>>>> The syzbot reported the below BUG: > >>>>>>> > >>>>>>> kernel BUG at include/linux/page-flags.h:785! > >>> [...] > >>>>>>> RIP: 0010:PageDoubleMap include/linux/page-flags.h:785 [inline] > >>>>>>> RIP: 0010:__page_mapcount+0x2d2/0x350 mm/util.c:744 > >>> [...] > >>>>>> Does this point at the bigger issue that reading the mapcount without > >>>>>> having the page locked is completely unstable? > >>>>> > >>>>> (See also https://lore.kernel.org/all/CAG48ez0M=iwJu=Q8yUQHD-+eZDg6ZF8QCF86Sb=CN1petP=Y0Q@mail.gmail.com/ > >>>>> for context.) > >>>> > >>>> Thanks for the pointer. > >>>> > >>>>> > >>>>> I'm not sure what you mean by "unstable". Do you mean "the result is > >>>>> not guaranteed to still be valid when the call returns", "the result > >>>>> might not have ever been valid", or "the call might crash because the > >>>>> page's state as a compound page is unstable"? > >>>> > >>>> A little bit of everything :) > >>> [...] > >>>>> In case you mean "the result might not have ever been valid": > >>>>> Yes, even with this patch applied, in theory concurrent THP splits > >>>>> could cause us to count some page mappings twice. Arguably that's not > >>>>> entirely correct. > >>>> > >>>> Yes, the snapshot is not atomic and, thereby, unreliable. That what I > >>>> mostly meant as "unstable". > >>>> > >>>>> > >>>>> In case you mean "the call might crash because the page's state as a > >>>>> compound page could concurrently change": > >>>> > >>>> I think that's just a side-product of the snapshot not being "correct", > >>>> right? > >>> > >>> I guess you could see it that way? The way I look at it is that > >>> page_mapcount() is designed to return a number that's at least as high > >>> as the number of mappings (rarely higher due to races), and using > >>> page_mapcount() on an unlocked page is legitimate if you're fine with > >>> the rare double-counting of references. In my view, the problem here > >>> is: > >>> > >>> There are different types of references to "struct page" - some of > >>> them allow you to call page_mapcount(), some don't. And in particular, > >>> get_page() doesn't give you a reference that can be used with > >>> page_mapcount(), but locking a (real, non-migration) PTE pointing to > >>> the page does give you such a reference. > >> > >> I assume the point is that as long as the page cannot be unmapped > >> because you block it from getting unmapped (PT lock), the compound page > >> cannot get split. As long as the page cannot get unmapped from that page > >> table you should have at least a mapcount of 1. > > > > If you mean holding ptl could prevent THP from splitting, then it is > > not true since you may be in the middle of THP split just exactly like > > the race condition solved by this patch. > > While you hold the PT lock and discover a mapped page, unmap_page() > cannot continue and unmap the page. That's what I meant "as long as the > page cannot be unmapped". > > What doesn't work is if you hold the PT lock and discover a migration > entry, because then you're already past unmap_page(). That's the issue > you're fixing. Yeah, it means you lose the race :-( > > > > > Just page lock or elevated page refcount could serialize against THP > > split AFAIK. > > > >> > >> But yeah, using the mapcount of a page that is not even mapped > >> (migration entry) is clearly wrong. > >> > >> To summarize: reading the mapcount on an unlocked page will easily > >> return a wrong result and the result should not be relied upon. reading > >> the mapcount of a migration entry is dangerous and certainly wrong. > > > > Depends on your usecase. Some just want to get a snapshot, just like > > smaps, they don't care. > > Right, but as discussed, even the snapshot might be slightly wrong. That > might be just fine for smaps (and I would have enjoyed a comment in the > code stating that :) ). I think that is documented already, see Documentation/filesystems/proc.rst: Note: reading /proc/PID/maps or /proc/PID/smaps is inherently racy (consistent output can be achieved only in the single read call). Of course, if the extra note is preferred in the code, I could try to add some in a separate patch. > > > -- > Thanks, > > David / dhildenb >