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=-3.8 required=3.0 tests=BAYES_00, 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 7E4F4C4363D for ; Thu, 24 Sep 2020 00:59:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 173E923119 for ; Thu, 24 Sep 2020 00:59:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 173E923119 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 341A86B0003; Wed, 23 Sep 2020 20:59:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F2626B0055; Wed, 23 Sep 2020 20:59:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1DFE96B005A; Wed, 23 Sep 2020 20:59:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0152.hostedemail.com [216.40.44.152]) by kanga.kvack.org (Postfix) with ESMTP id 059676B0003 for ; Wed, 23 Sep 2020 20:59:46 -0400 (EDT) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id B2A231EFF for ; Thu, 24 Sep 2020 00:59:45 +0000 (UTC) X-FDA: 77296147530.23.neck52_00185372715a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin23.hostedemail.com (Postfix) with ESMTP id 917D837609 for ; Thu, 24 Sep 2020 00:59:45 +0000 (UTC) X-HE-Tag: neck52_00185372715a X-Filterd-Recvd-Size: 5181 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf22.hostedemail.com (Postfix) with ESMTP for ; Thu, 24 Sep 2020 00:59:43 +0000 (UTC) IronPort-SDR: JEFFGmC6foNZD+ktgkudovF00ZEI+e1ZTaF/lmIy8n5wqy584zBhzK0lSGFxifrDhR54ld43fi v1Mlsbfrr64Q== X-IronPort-AV: E=McAfee;i="6000,8403,9753"; a="161131942" X-IronPort-AV: E=Sophos;i="5.77,296,1596524400"; d="scan'208";a="161131942" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Sep 2020 17:59:42 -0700 IronPort-SDR: HSpho6kL3zwPe/VHHvrj7yevgwanFGrmVX3hFHAd1upyYQ6JywtFX6Qmv6PILP066RNfh3BndQ uROw/ULe8tPg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,296,1596524400"; d="scan'208";a="322789760" Received: from yhuang-dev.sh.intel.com (HELO yhuang-dev) ([10.239.159.164]) by orsmga002.jf.intel.com with ESMTP; 23 Sep 2020 17:59:40 -0700 From: "Huang\, Ying" To: Rafael Aquini Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org Subject: Re: [PATCH] mm: swapfile: avoid split_swap_cluster() NULL pointer dereference References: <20200922184838.978540-1-aquini@redhat.com> <878sd1qllb.fsf@yhuang-dev.intel.com> <20200923043459.GL795820@optiplex-lnx> <87sgb9oz1u.fsf@yhuang-dev.intel.com> <20200923130138.GM795820@optiplex-lnx> Date: Thu, 24 Sep 2020 08:59:40 +0800 In-Reply-To: <20200923130138.GM795820@optiplex-lnx> (Rafael Aquini's message of "Wed, 23 Sep 2020 09:01:38 -0400") Message-ID: <87blhwng5f.fsf@yhuang-dev.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii 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: Rafael Aquini writes: > On Wed, Sep 23, 2020 at 01:13:49PM +0800, Huang, Ying wrote: >> Rafael Aquini writes: >> >> > On Wed, Sep 23, 2020 at 10:21:36AM +0800, Huang, Ying wrote: >> >> Hi, Rafael, >> >> >> >> Rafael Aquini writes: >> >> >> >> > The swap area descriptor only gets struct swap_cluster_info *cluster_info >> >> > allocated if the swapfile is backed by non-rotational storage. >> >> > When the swap area is laid on top of ordinary disk spindles, lock_cluster() >> >> > will naturally return NULL. >> >> >> >> Thanks for reporting. But the bug looks strange. Because in a system >> >> with only HDD swap devices, during THP swap out, the swap cluster >> >> shouldn't be allocated, as in >> >> >> >> shrink_page_list() >> >> add_to_swap() >> >> get_swap_page() >> >> get_swap_pages() >> >> swap_alloc_cluster() >> >> >> > >> > The underlying problem is that swap_info_struct.cluster_info is always NULL >> > on the rotational storage case. >> >> Yes. >> >> > So, it's very easy to follow that constructions >> > like this one, in split_swap_cluster >> > >> > ... >> > ci = lock_cluster(si, offset); >> > cluster_clear_huge(ci); >> > ... >> > >> > will go for a NULL pointer dereference, in that case, given that lock_cluster >> > reads: >> > >> > ... >> > struct swap_cluster_info *ci; >> > ci = si->cluster_info; >> > if (ci) { >> > ci += offset / SWAPFILE_CLUSTER; >> > spin_lock(&ci->lock); >> > } >> > return ci; >> > ... >> >> But on HDD, we shouldn't call split_swap_cluster() at all, because we >> will not allocate swap cluster firstly. So, if we run into this, >> there should be some other bug, we need to figure it out. >> > > split_swap_cluster() gets called by split_huge_page_to_list(), > if the page happens to be in the swapcache, and it will always > go that way, regardless the backing storage type: > > ... > __split_huge_page(page, list, end, flags); > if (PageSwapCache(head)) { > swp_entry_t entry = { .val = page_private(head) }; > > ret = split_swap_cluster(entry); > } else > ret = 0; > ... > > The problem is not about allocating the swap_cluster -- it's obviously > not allocated in these cases. The problem is that on rotational > storage you don't even have the base structure that allows you to > keep the swap clusters (cluster_info does not get allocated, at all, > so si->cluster_info is always NULL) > > You can argue about other bugs all you want, it doesn't change > the fact that this code is incomplete as it sits, because it > misses checking for a real case where lock_cluster() will return NULL. I don't want to argue about anything. I just want to fix the bug. The fix here will hide the real bug instead of fixing it. For the situation you described (PageSwapCache() returns true for a THP backed by a normal swap entry (not swap cluster)), we will run into other troubles too. So we need to find the root cause and fix it. Can you help me to collect more information to fix the real bug? Or, how to reproduce it? Best Regards, Huang, Ying