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.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, 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 DBA39C4727E for ; Thu, 24 Sep 2020 02:09:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 445B22311E for ; Thu, 24 Sep 2020 02:09:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Adqmaitq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 445B22311E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B14C86B006E; Wed, 23 Sep 2020 22:09:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AC5A76B0070; Wed, 23 Sep 2020 22:09:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9DE1C8E0001; Wed, 23 Sep 2020 22:09:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0091.hostedemail.com [216.40.44.91]) by kanga.kvack.org (Postfix) with ESMTP id 889316B006E for ; Wed, 23 Sep 2020 22:09:39 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 34BEE181AE862 for ; Thu, 24 Sep 2020 02:09:39 +0000 (UTC) X-FDA: 77296323678.07.sink44_450a5002715b Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin07.hostedemail.com (Postfix) with ESMTP id 1C6C31803F9AF for ; Thu, 24 Sep 2020 02:09:39 +0000 (UTC) X-HE-Tag: sink44_450a5002715b X-Filterd-Recvd-Size: 6763 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf23.hostedemail.com (Postfix) with ESMTP for ; Thu, 24 Sep 2020 02:09:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1600913377; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=pr+TFvgT15Y3MIUsJ7R07JPi6Z9CSwvWJeXmktBGVqc=; b=AdqmaitqaNaXIrjLMJf91W9V1zYq5BDaCqQi+i7j8416edJ2Rcl04iP8wq73EfcV3LnHge 6KvN3vOH0mK91nyDQTg8bb6UjiWkAzSi9Qtbwy6MH9IhvfgHGKmknJ0fpT/UUU75fiJVQC 54IWFClvf7azbIkOf5HMbN47x23x1JI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-377-b-Cy_P4sOhGY6UhuTZlcEA-1; Wed, 23 Sep 2020 22:09:34 -0400 X-MC-Unique: b-Cy_P4sOhGY6UhuTZlcEA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E3DF0801AF7; Thu, 24 Sep 2020 02:09:32 +0000 (UTC) Received: from optiplex-lnx (unknown [10.3.128.5]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A7E6060BF1; Thu, 24 Sep 2020 02:09:31 +0000 (UTC) Date: Wed, 23 Sep 2020 22:09:28 -0400 From: Rafael Aquini To: "Huang, Ying" 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 Message-ID: <20200924020928.GC1023012@optiplex-lnx> 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> <87blhwng5f.fsf@yhuang-dev.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87blhwng5f.fsf@yhuang-dev.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 24, 2020 at 08:59:40AM +0800, Huang, Ying wrote: > 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. > The bug here is quite simple: split_swap_cluster() misses checking for lock_cluster() returning NULL before committing to change cluster_info->flags. The fundamental problem has nothing to do with allocating, or not allocating a swap cluster, but it has to do with the fact that the THP deferred split scan can transiently race with swapcache insertion, and the fact that when you run your swap area on rotational storage cluster_info is _always_ NULL. split_swap_cluster() needs to check for lock_cluster() returning NULL because that's one possible case, and it clearly fails to do so. Run a workload that cause multiple THP COW, and add a memory hogger to create memory pressure so you'll force the reclaimers to kick the registered shrinkers. The trigger is not heavy swapping, and that's probably why most swap test cases don't hit it. The window is tight, but you will get the NULL pointer dereference. Regardless you find furhter bugs, or not, this patch is needed to correct a blunt coding mistake.