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 4BA39C2D0A8 for ; Wed, 23 Sep 2020 13:01:52 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 63DF62176B for ; Wed, 23 Sep 2020 13:01:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="HOZSFkLe" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 63DF62176B 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 764EE6B0003; Wed, 23 Sep 2020 09:01:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 714BF6B0055; Wed, 23 Sep 2020 09:01:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 62AAC6B005A; Wed, 23 Sep 2020 09:01:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0111.hostedemail.com [216.40.44.111]) by kanga.kvack.org (Postfix) with ESMTP id 48FAB6B0003 for ; Wed, 23 Sep 2020 09:01:50 -0400 (EDT) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id ECCF08249980 for ; Wed, 23 Sep 2020 13:01:49 +0000 (UTC) X-FDA: 77294338338.11.grape25_1e0ef2e27156 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin11.hostedemail.com (Postfix) with ESMTP id C2957180F8B87 for ; Wed, 23 Sep 2020 13:01:49 +0000 (UTC) X-HE-Tag: grape25_1e0ef2e27156 X-Filterd-Recvd-Size: 4926 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf37.hostedemail.com (Postfix) with ESMTP for ; Wed, 23 Sep 2020 13:01:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1600866108; 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=9YBlT3EvuezrkgbgwTKtVPkbq/M4DSApbdGRGuPKZmU=; b=HOZSFkLeOF6ZSnS/CzrWazQGoCiV5RW7ViFccrC2/yW0BPr5xqLIirozKX9VZAORzDD1CH DhYhipVRgvUonRcb9oJxrEpqoNOxYcIPYgGn9+/rUNmj0T/3GfQechzEb9vzbnN2bpfURn ZESg9CYC3BtKvnX6ZkMiCbooXXajb9E= 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-19-HceP2J3VOyyV9JZAKQWjLQ-1; Wed, 23 Sep 2020 09:01:45 -0400 X-MC-Unique: HceP2J3VOyyV9JZAKQWjLQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5CE711882FBC; Wed, 23 Sep 2020 13:01:43 +0000 (UTC) Received: from optiplex-lnx (unknown [10.3.128.5]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0F96E75120; Wed, 23 Sep 2020 13:01:41 +0000 (UTC) Date: Wed, 23 Sep 2020 09:01:38 -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: <20200923130138.GM795820@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87sgb9oz1u.fsf@yhuang-dev.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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, 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.