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 979E7CD37B0 for ; Sat, 16 Sep 2023 01:35:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 28EFF6B038E; Fri, 15 Sep 2023 21:35:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 23CD16B038F; Fri, 15 Sep 2023 21:35:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 104126B0391; Fri, 15 Sep 2023 21:35:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 004256B038E for ; Fri, 15 Sep 2023 21:35:19 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C8AA58019D for ; Sat, 16 Sep 2023 01:35:19 +0000 (UTC) X-FDA: 81240742758.08.86928FF Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) by imf28.hostedemail.com (Postfix) with ESMTP id 1637EC0003 for ; Sat, 16 Sep 2023 01:35:17 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=nqjj9kex; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.215.178 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694828118; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=aDP8MuAzIXg6IFVmOdjNhHZQ4nVgpXiGwm/XOpBUoIo=; b=tTIR1OCoPG2xEg/5yFuNe9KdnaVxyDOtfLWo3HEV7S/jD0k7rdzLspeSzKuVp/DsGTHZ5T vM2I7e1C2D5//bQOSgYKxiw8esbu5maxdN+2GOWch0T+g4GFwLvO2F8qqacIlGWzMRd34F S0bAfSyTKwcHFyo2QSHOAF1vBDlsPcU= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=nqjj9kex; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.215.178 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694828118; a=rsa-sha256; cv=none; b=XdDsHOkMcO9NpztDIhmEK+7LKXxaMa//1jq+ppLAVlT+LCKonCfLxENIbKr7RMTkNMPSEN Vx9+fpVf/SYRYTTfrQ6NN1BY8Fzp5qpTtIwafhYztw1cxpFVuDZZdyTonxDaxCrGDILkx2 MllKSOsfta9gBmwZhGs8uy3xCzbuCuI= Received: by mail-pg1-f178.google.com with SMTP id 41be03b00d2f7-570a432468bso2858564a12.0 for ; Fri, 15 Sep 2023 18:35:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1694828117; x=1695432917; darn=kvack.org; 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=aDP8MuAzIXg6IFVmOdjNhHZQ4nVgpXiGwm/XOpBUoIo=; b=nqjj9kexgFy1rnnBtJKbjCeOKYT/Iv2UHTHgSTT1E9D09lhZC/nPbEYhyJFe35Ixoh oFJy+8n/SIEZiUmXU+RifLwEB9y3jtCPQe5UgnizSFdgdAPWX5FiJuUTjLbBiKmtSEoW 72DIQ9tK19Wi+AWm9MwxHKaRevJ7iTIZWy8o5D6/93wu61w7G3tCTLlk8KvOeq4/Yyjt kgzZ9s50m1mjD2CYhy3HfxRKesc5e7ILQS755t1dUhxIKswgKuyGHdBkVEW+s/84Hhum us6v56rS0/VVpSmxfM1dyYvnjv6ncqXy+xKEypY9ptOJPRg5h4qcx0ohUb9TgOHykdXt CLNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694828117; x=1695432917; 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=aDP8MuAzIXg6IFVmOdjNhHZQ4nVgpXiGwm/XOpBUoIo=; b=r7j3nbzi1mbryhwA3/yVdsI13dx97cKb2pBjJXtMcRrjNZnV9cwSUtUNnYoy6M0Mdg amtQnubqEdDLjJTWKPyDd6WX25+yYjWX0/d9MRQWZ6u49aLOU5U3xtnN/L2HW0+lexe8 zt3fM9Hg8lb7McSFe9wLuSlnov9vst3WUI/MIaIeRWkVu8dlXkJ8mxy9J83QiXXNCiiT OTt8nOEEirqyv//1AwQLUWCaiQF1Nvb6db05sWERkLywo4SOFnEnfUux4uQAUmh2rYT6 4sZfDJFlkCUQDYDS93J1a6TvuwT+UIYr0EU1H6Kh5yCDNkEQPWO09dbBgzEOeY9/T/u+ f+WQ== X-Gm-Message-State: AOJu0YxDRNYSV2NJ92P1KhaPsD/rItMoC1yHaRIDe4HHF4E0f1tdSZO+ N5AbUe5QVmyEF1Az/6MgGdW1YL2FRiTjv7JjAm0= X-Google-Smtp-Source: AGHT+IGRmqsdgkpiCbQ+4EDo0CWbFWhEUlnOxk+Sw/4Xj1QOSuG9/tPxvlknACsF0eai5kU6/zaF2DpKeDy/eu8gO44= X-Received: by 2002:a17:90a:bc82:b0:274:9121:382 with SMTP id x2-20020a17090abc8200b0027491210382mr5337411pjr.22.1694828116763; Fri, 15 Sep 2023 18:35:16 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yang Shi Date: Fri, 15 Sep 2023 18:35:04 -0700 Message-ID: Subject: Re: [syzbot] [mm?] kernel BUG in vma_replace_policy To: Hugh Dickins Cc: Suren Baghdasaryan , Matthew Wilcox , Michal Hocko , Vlastimil Babka , syzbot , akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, syzkaller-bugs@googlegroups.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: cqqdicqs7iqougnqfu1qusugyqdsmark X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 1637EC0003 X-HE-Tag: 1694828117-841929 X-HE-Meta: U2FsdGVkX18RKlwe8iSkm3vWcgxus/IE4ul8V1Y7G6z/lzC17lVkTjfNeWfRVNJxPoKg5EMxxutHJU5slwzTarxmx7xXIpGR0UFBKlYN/z1UYS+TlBPUYd5VRDhsrARksyvYEg+IZkuXzjpcWtst0LJy+alxvQ+6Zh8bXwK42fgeG+h0W0n9Xa+mlYNIPQTJxdDivBCcev6i/Xk5+wvOtDMsdzsU3b7ZbQKKLebeElQyxkhzlTCaBjNuHitHoRIr6N9+UHtGdL+8DrAF+YvO0NpJa/vucFo0iVMWIeo18fp4CdfklWkZLxRRAe2RS3jxngDcAmz8HlPHI9uISj8kX+Iaf0LbtesNvJZaE0RJ/VzwaOy/wLAhAb2D8Gbs0kWpgtEirHXF3qUGS06gKml6J3ipQSod6bBzfaCi+P8UsAU/5YWKj9nfvQzf+/xenqBdYbXLQPlnBFF8WBnSGNR0y1B8pa2TI1AfelHEqjOktY6yRO26zbCk7jvoimk1W+PEPZuKc2KDLdOmyKklDDeOfDkTD/Xs6EfEaj80zmhBrAbA9yuUIZz6u2RQNeqjjRr3Ol+RkNZ0jdcmJAXcc0bwuW6zLn3YQXzYSTrOakfaqYkbmTnZZEWyPTLmRhBESdbwfOF8PAA/T2Uk/0iltlsLRcOM6MC64JUbpJWJGxas66wcsEBu6dAB1ru6q6DJUSIHE7YaHoV2/B58Z8Yn1OVAdwQl9TeRwVB+JIQSNylB95OVhx+aBsWssiUpdm+xV2CApvuYr9dtZYMbJKqWWHBYNFO6ioP5TkQAg+y5+ff+myyuNaexu1sQzXUnZaak0+jNdtM3VV7eEssENsQL/ydWv4dOvDHhkSLYqiuqLDK7msWOrtAU4ffHbNiLPaEAAJ4QbDkLoj3xXiJUwQ7h+go/c56bajk7r287OkkU8u8i9WSmd8lQdqx/98S9Sy26IZAmAWSeOIJL/3oMTHqMXka olyR/Uvz W0pcd+KHEmfzKFe7LYM93f97eGT+kxosuSLqZ1IAwyVJQ0CiI+q8DO5Aa+8lVebZL4jE+PGSeC2rvSO/plU/Nw/ukkW327GSPiwE67X7jNnD/sHYmzOvTAKUMZRlShOK3k3f5S7AjaPENOtO1HHrp1i7Jl0EP8hKIkv8FmkZcSN/6ur4Jk0Vwerdf7yZP1WiIR6scMOkCO6gNJViCwMxzpqzzPd593V1xxoT0L3KXzF9mDJGNMB0xFgQEGk6pRqCBjoVHeNfHqSRf8jA8twbBzgzxRaV9I1qEMju/MrpKlmsMuCcEkWxz0iswdAFw27dwhvg7svDTuoBj50vr7pCJnS4rQTCi2J9cgAVEC5HBQ3rUQmxuSi4Rd/WQy5zDFEF7cVI1hVbJih0ALZ1RCOjvJVWL0+GxVnBw2rXh8fHr2aW/Qdmoq2wsknUduH5xNeV2jgqyuaa797XsGHvOkZtjMBcKweHC8yrbvjV2rEWQDMvqK9V6eopfIThxHW1+0lqBEipZg+Jd3kIjFg8egL60hy9AeaFeKiyIlVbvxP+tD3XC8EmcDImi8epPnoqJ+oANfWCBnS35FKgZ31+xORWvtNnn68jM9Ye76dsil5ZPGc/ppuE2QqBz/r3mfTe8HlImdIQKBblHf0Lfj3xed5SZ2n0Wp6OwtvYHxiun9Jss5Mn5jKELVZlMxiks+YWNIHCtaMZy0tqEkiN9OE+T5mXfKB+5g0qHueG6EMgcGQfkMaTVOoU= 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 Thu, Sep 14, 2023 at 9:26=E2=80=AFPM Hugh Dickins wro= te: > > On Thu, 14 Sep 2023, Suren Baghdasaryan wrote: > > On Thu, Sep 14, 2023 at 9:24=E2=80=AFPM Matthew Wilcox wrote: > > > On Thu, Sep 14, 2023 at 08:53:59PM +0000, Suren Baghdasaryan wrote: > > > > On Thu, Sep 14, 2023 at 8:00=E2=80=AFPM Suren Baghdasaryan wrote: > > > > > On Thu, Sep 14, 2023 at 7:09=E2=80=AFPM Matthew Wilcox wrote: > > > > > > > > > > > > On Thu, Sep 14, 2023 at 06:20:56PM +0000, Suren Baghdasaryan wr= ote: > > > > > > > I think I found the problem and the explanation is much simpl= er. While > > > > > > > walking the page range, queue_folios_pte_range() encounters a= n > > > > > > > unmovable page and queue_folios_pte_range() returns 1. That c= auses a > > > > > > > break from the loop inside walk_page_range() and no more VMAs= get > > > > > > > locked. After that the loop calling mbind_range() walks over = all VMAs, > > > > > > > even the ones which were skipped by queue_folios_pte_range() = and that > > > > > > > causes this BUG assertion. > > > > > > > > > > > > > > Thinking what's the right way to handle this situation (what'= s the > > > > > > > expected behavior here)... > > > > > > > I think the safest way would be to modify walk_page_range() a= nd make > > > > > > > it continue calling process_vma_walk_lock() for all VMAs in t= he range > > > > > > > even when __walk_page_range() returns a positive err. Any obj= ection or > > > > > > > alternative suggestions? > > > > > > > > > > > > So we only return 1 here if MPOL_MF_MOVE* & MPOL_MF_STRICT were > > > > > > specified. That means we're going to return an error, no matte= r what, > > > > > > and there's no point in calling mbind_range(). Right? > > > > > > > > > > > > +++ b/mm/mempolicy.c > > > > > > @@ -1334,6 +1334,8 @@ static long do_mbind(unsigned long start,= unsigned long len, > > > > > > ret =3D queue_pages_range(mm, start, end, nmask, > > > > > > flags | MPOL_MF_INVERT, &pagelist, tr= ue); > > > > > > > > > > > > + if (ret =3D=3D 1) > > > > > > + ret =3D -EIO; > > > > > > if (ret < 0) { > > > > > > err =3D ret; > > > > > > goto up_out; > > > > > > > > > > > > (I don't really understand this code, so it can't be this simpl= e, can > > > > > > it? Why don't we just return -EIO from queue_folios_pte_range(= ) if > > > > > > this is the right answer?) > > > > > > > > > > Yeah, I'm trying to understand the expected behavior of this func= tion > > > > > to make sure we are not missing anything. I tried a simple fix th= at I > > > > > suggested in my previous email and it works but I want to underst= and a > > > > > bit more about this function's logic before posting the fix. > > > > > > > > So, current functionality is that after queue_pages_range() encount= ers > > > > an unmovable page, terminates the loop and returns 1, mbind_range() > > > > will still be called for the whole range > > > > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L134= 5), > > > > all pages in the pagelist will be migrated > > > > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L135= 5) > > > > and only after that the -EIO code will be returned > > > > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L136= 2). > > > > So, if we follow Matthew's suggestion we will be altering the curre= nt > > > > behavior which I assume is not what we want to do. > > > > > > Right, I'm intentionally changing the behaviour. My thinking is > > > that mbind(MPOL_MF_MOVE | MPOL_MF_STRICT) is going to fail. Should > > > such a failure actually move the movable pages before reporting that > > > it failed? I don't know. > > > > > > > The simple fix I was thinking about that would not alter this behav= ior > > > > is smth like this: > > > > > > I don't like it, but can we run it past syzbot to be sure it solves t= he > > > issue and we're not chasing a ghost here? > > > > Yes, I just finished running the reproducer on both upstream and > > linux-next builds listed in > > https://syzkaller.appspot.com/bug?extid=3Db591856e0f0139f83023 and the > > problem does not happen anymore. > > I'm fine with your suggestion too, just wanted to point out it would > > introduce change in the behavior. Let me know how you want to proceed. > > Well done, identifying the mysterious cause of this problem: > I'm glad to hear that you've now verified that hypothesis. > > You're right, it would be a regression to follow Matthew's suggestion. > > Traditionally, modulo bugs and inconsistencies, the queue_pages_range() > phase of do_mbind() has done the best it can, gathering all the pages it > can that need migration, even if some were missed; and proceeds to do the > mbind_range() phase if there was nothing "seriously" wrong (a gap causing > -EFAULT). Then at the end, if MPOL_MF_STRICT was set, and not all the > pages could be migrated (or MOVE was not specified and not all pages > were well placed), it returns -EIO rather than 0 to inform the caller > that not all could be done. > > There have been numerous tweaks, but I think most importantly > 5.3's d883544515aa ("mm: mempolicy: make the behavior consistent when > MPOL_MF_MOVE* and MPOL_MF_STRICT were specified") added those "return 1"s > which stop the pagewalk early. In my opinion, not an improvement - makes > it harder to get mbind() to do the best job it can (or is it justified as > what you're asking for if you say STRICT?). Hi Suren and Hugh, Thanks for figuring this out. The mbind behavior is a little bit messy and hard to follow. I tried my best to recall all the changes. IIUC, mbind did break the vma iteration early in the first place, then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") changed the behavior (didn't break vma iteration early for some cases anymore), but it messed up the return value and caused some test cases failure, also violated the manual. The return value issue was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), this commit also restored the oldest behavior (break loop early). But it also breaks the loop early when MPOL_MF_MOVE|MOVEALL is set, kernel should actually continue the loop to try to migrate all existing pages per the manual. It sounds like a regression. I will take a look at it. So the logic should conceptually look like: if (MPOL_MF_MOVE|MOVEALL) continue; if (MPOL_MF_STRICT) break; So it is still possible that some VMAs are not locked if only MPOL_MF_STRICT is set. > > But whatever, it would be a further regression for mbind() not to have > done the mbind_range(), even though it goes on to return -EIO. > > I had a bad first reaction to your walk_page_range() patch (was expecting > to see vma_start_write()s in mbind_range()), but perhaps your patch is > exactly what process_mm_walk_lock() does now demand. > > [Why is Hugh responding on this? Because I have some long-standing > mm/mempolicy.c patches to submit next week, but in reviewing what I > could or could not afford to get into at this time, had decided I'd > better stay out of queue_pages_range() for now - beyond the trivial > preferring an MPOL_MF_WRLOCK flag to your bool lock_vma.] > > Hugh