From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-182.mta1.migadu.com (out-182.mta1.migadu.com [95.215.58.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5D76233D1 for ; Mon, 23 Dec 2024 06:59:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734937171; cv=none; b=L+Z0QpoO1bvZOF0Eqhx44NuIZDCJrRnO0cyJhuKtcJUVXygtLdjNZlRovvwBBNllTtmStrhyJqgVX93b8lcMCt7mdsP3RaB1jw5EH3dqoIGnRy9RYNIOZFaUiKKaFddpt5o20J7tYwtWxD0AOuIuRk2jKJl+bFej6p3CSVC+HKc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734937171; c=relaxed/simple; bh=BKd2rAMh8Drm40qmHqE+QNHqFjumpHBUNlkvcqkGYko=; h=Content-Type:From:Mime-Version:Subject:Date:Message-Id:References: Cc:In-Reply-To:To; b=rHBrZATiHPg2RkRjmEqkWtoZchBtbt6DwtvKNwBYpgX4hCF2XWGNWC8DEuaNNFM1Iq+6tPSXIZJ0WJ5aOgfmX/VxXGJzOChwVIHrq4/tLZN8lkwXAwJv+AaAyJSYY3d+yy/txQBi9g/Y/93+Bwv+3sM4dSf3unEpw3Xj1o2vdqg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=moHAoNAT; arc=none smtp.client-ip=95.215.58.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="moHAoNAT" Content-Type: text/plain; charset=utf-8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1734937165; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SNj5OAreCtWnyMSUyYMFWWVofqOXNGMfoPvDSwrklQ8=; b=moHAoNATiiPlQsSqS5MR+uZms+TpPalyXrKrEIA47lpvhUwHByzzQGdJDJLkBVeZPEF28K i1/v2Lv0LM4oRjdGPVkd4a1jLZGZeZdXAe8ZOFINPOVMEOR4r52QytPK2HxPXG+nwfLxQZ peCOsJemOl4hHc0pm6iSpfZ+m3r+Tuc= Content-Transfer-Encoding: quoted-printable X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (1.0) Subject: Re: [PATCH] mm, madvise: fix potential workingset node list_lru leaks Date: Mon, 23 Dec 2024 14:58:48 +0800 Message-Id: References: <20241222122936.67501-1-ryncsn@gmail.com> Cc: linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Johannes Weiner , Roman Gushchin , Shakeel Butt , Michal Hocko , Chengming Zhou , Qi Zheng , Yu Zhao , Sasha Levin , linux-kernel@vger.kernel.org, syzbot+38a0cbd267eff2d286ff@syzkaller.appspotmail.com, Kairui Song In-Reply-To: <20241222122936.67501-1-ryncsn@gmail.com> To: Kairui Song X-Migadu-Flow: FLOW_OUT > On Dec 22, 2024, at 20:29, Kairui Song wrote: >=20 > =EF=BB=BFFrom: Kairui Song >=20 > Since commit 5abc1e37afa0 ("mm: list_lru: allocate list_lru_one > only when needed"), all list_lru users need to allocate the items using > the new infrastructure that provides list_lru info for slab allocation, > ensuring that the corresponding memcg list_lru is allocated before use. >=20 > For workingset shadow nodes (which are xa_node), users are converted to > use the new infrastructure by commit 9bbdc0f32409 ("xarray: use > kmem_cache_alloc_lru to allocate xa_node"). The xas->xa_lru will be > set correctly for filemap users. However, there is a missing case: > xa_node allocations caused by madvise(..., MADV_COLLAPSE). >=20 > madvise(..., MADV_COLLAPSE) will also read in the absent parts of file > map, and there will be xa_nodes allocated for the caller's memcg > (assuming it's not rootcg). However, these allocations won't trigger > memcg list_lru allocation because the proper xas info was not set. >=20 > If nothing else has allocated other xa_nodes for that memcg to trigger > list_lru creation, and memory pressure starts to evict file pages, > workingset_update_node will try to add these xa_nodes to their > corresponding memcg list_lru, and it does not exist (NULL). So they > will be added to rootcg's list_lru instead. >=20 > This shouldn't be a significant issue in practice, but it is indeed > unexpected behavior, and these xa_nodes will not be reclaimed > effectively. And may lead to incorrect counting of the > list_lru->nr_items counter. >=20 > This problem wasn't exposed until recent commit 28e98022b31ef > ("mm/list_lru: simplify reparenting and initial allocation") added a > sanity check: only dying memcg could have a NULL list_lru when > list_lru_{add,del} is called. This problem triggered this WARNING. >=20 > So make madvise(..., MADV_COLLAPSE) also call xas_set_lru() to pass > the list_lru which we may want to insert xa_node into later. And move > mapping_set_update to mm/internal.h, and turn into a macro to avoid > including extra headers in mm/internal.h. >=20 > Fixes: 9bbdc0f32409 ("xarray: use kmem_cache_alloc_lru to allocate xa_node= ") Hi Kairui, This should be 7d8faaf15545 introducing MADV_COLLAPSE, right? Because it is d= eveloped after 9bbdc0f32409. Thanks. > Reported-by: syzbot+38a0cbd267eff2d286ff@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/lkml/675d01e9.050a0220.37aaf.00be.GAE@goog= le.com/ > Signed-off-by: Kairui Song > --- > mm/filemap.c | 9 --------- > mm/internal.h | 6 ++++++ > mm/khugepaged.c | 3 +++ > 3 files changed, 9 insertions(+), 9 deletions(-) >=20 > diff --git a/mm/filemap.c b/mm/filemap.c > index f61cf51c2238..33b60d448fca 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -124,15 +124,6 @@ > * ->private_lock (zap_pte_range->block_dirty_folio) > */ >=20 > -static void mapping_set_update(struct xa_state *xas, > - struct address_space *mapping) > -{ > - if (dax_mapping(mapping) || shmem_mapping(mapping)) > - return; > - xas_set_update(xas, workingset_update_node); > - xas_set_lru(xas, &shadow_nodes); > -} > - > static void page_cache_delete(struct address_space *mapping, > struct folio *folio, void *shadow) > { > diff --git a/mm/internal.h b/mm/internal.h > index cb8d8e8e3ffa..4e7a3a93d0a2 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -1510,6 +1510,12 @@ static inline void shrinker_debugfs_remove(struct d= entry *debugfs_entry, > /* Only track the nodes of mappings with shadow entries */ > void workingset_update_node(struct xa_node *node); > extern struct list_lru shadow_nodes; > +#define mapping_set_update(xas, mapping) do { \ > + if (!dax_mapping(mapping) && !shmem_mapping(mapping)) { \ > + xas_set_update(xas, workingset_update_node); \ > + xas_set_lru(xas, &shadow_nodes); \ > + } \ > +} while (0) >=20 > /* mremap.c */ > unsigned long move_page_tables(struct vm_area_struct *vma, > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 6f8d46d107b4..653dbb1ff05c 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include >=20 > #include > @@ -1837,6 +1838,8 @@ static int collapse_file(struct mm_struct *mm, unsig= ned long addr, > if (result !=3D SCAN_SUCCEED) > goto out; >=20 > + mapping_set_update(&xas, mapping); > + > __folio_set_locked(new_folio); > if (is_shmem) > __folio_set_swapbacked(new_folio); > -- > 2.47.1 >=20