From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-183.mta0.migadu.com (out-183.mta0.migadu.com [91.218.175.183]) (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 48AB41369B0 for ; Thu, 1 Aug 2024 02:43:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.183 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722480188; cv=none; b=leLgzUqKKas+VbAyWGPsf+/fI9klROCwpA7v5CN+NzC/FZFNqxk8MziKfposxq5opfRuiV2l5ZCjdccaiaP5gTwMFLnKwvImlTi/K5aRlxjk5TwV4hdRsPkB9IyCb8GN93Qntmw+UfBZzEdfErOzXs7TFHNdhhwqySfxzWSCdD0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722480188; c=relaxed/simple; bh=fVIuLuTl5OGi9WKgnfgPL4/3K+1IPxdSwrutsPmreSM=; h=Content-Type:Mime-Version:Subject:From:In-Reply-To:Date:Cc: Message-Id:References:To; b=cZiIBLLnmQmfowsUIc2kdiRYCeZk3WCpbO/pM9aDvsKL314KDW7pswPBOJQgHkt7C8r67N0rKxbv0E5A/l5kGL3adwuPgYuT96ztgp2OZGXuNmLSljDGZ2VbAse8VvQ8VaekIdtBU2v68tzJ+QdLYSOzX2IiFPYz7/jLAJaPCXc= 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=CXut+z9v; arc=none smtp.client-ip=91.218.175.183 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="CXut+z9v" Content-Type: text/plain; charset=us-ascii DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1722480184; 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=s1pmdeYWa+Lzpv2pjdW1po1MDOe6cIk4/35GzfdEkQk=; b=CXut+z9v81ShKg7s0PJUKYgpbAXt+JbA3Ns89bC8sDIWAJJHT6Z+lAiAQ+Q/rm70pUHHS4 6s9xrN+l5dlsHnA5ihhJwHPtK1yW3XIwEArReJxUSZKksJ3nDrpEgriTqzGsAHEMl9W8AU yhnI93siI8DwuwXmAvERcCYs2oGS6jk= Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.600.62\)) Subject: Re: [PATCH] mm: list_lru: fix UAF for memory cgroup X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: Date: Thu, 1 Aug 2024 10:42:31 +0800 Cc: Andrew Morton , Muchun Song , Johannes Weiner , Nhat Pham , Linux Memory Management List , LKML Content-Transfer-Encoding: quoted-printable Message-Id: References: <20240718083607.42068-1-songmuchun@bytedance.com> <20240723174540.18992614c476d77e7d9fb1e6@linux-foundation.org> <62BBC2A6-D6C3-48B8-B049-932E3BC16F31@linux.dev> To: "Vlastimil Babka (SUSE)" X-Migadu-Flow: FLOW_OUT > On Jul 31, 2024, at 18:06, Vlastimil Babka (SUSE) = wrote: >=20 > On 7/24/24 4:23 AM, Muchun Song wrote: >>=20 >>=20 >>> On Jul 24, 2024, at 08:45, Andrew Morton = wrote: >>>=20 >>> On Thu, 18 Jul 2024 16:36:07 +0800 Muchun Song = wrote: >>>=20 >>>> The mem_cgroup_from_slab_obj() is supposed to be called under rcu >>>> lock or cgroup_mutex or others which could prevent returned memcg >>>> from being freed. Fix it by adding missing rcu read lock. >>>=20 >>> "or others" is rather vague. What others? >>=20 >> Like objcg_lock. I have added this one into obj_cgroup_memcg(). >>=20 >>>=20 >>>> @@ -109,14 +110,20 @@ EXPORT_SYMBOL_GPL(list_lru_add); >>>>=20 >>>> bool list_lru_add_obj(struct list_lru *lru, struct list_head *item) >>>> { >>>> + bool ret; >>>> int nid =3D page_to_nid(virt_to_page(item)); >>>> - struct mem_cgroup *memcg =3D list_lru_memcg_aware(lru) ? >>>> - mem_cgroup_from_slab_obj(item) : NULL; >>>> + struct mem_cgroup *memcg; >>>>=20 >>>> - return list_lru_add(lru, item, nid, memcg); >>>> + rcu_read_lock(); >>>> + memcg =3D list_lru_memcg_aware(lru) ? = mem_cgroup_from_slab_obj(item) : NULL; >>>> + ret =3D list_lru_add(lru, item, nid, memcg); >>>> + rcu_read_unlock(); >>>=20 >>> We don't need rcu_read_lock() to evaluate NULL. >>>=20 >>> memcg =3D NULL; >>> if (list_lru_memcg_aware(lru)) { >>> rcu_read_lock(); >>> memcg =3D mem_cgroup_from_slab_obj(item); >>> rcu_read_unlock(); >>=20 >> Actually, the access to memcg is in list_lru_add(), so the rcu lock = should >> also cover this function rather than only mem_cgroup_from_slab_obj(). >> Something like: >>=20 >> memcg =3D NULL; >> if (list_lru_memcg_aware(lru)) { >> rcu_read_lock(); >> memcg =3D mem_cgroup_from_slab_obj(item); >> } >> ret =3D list_lru_add(lru, item, nid, memcg); >> if (list_lru_memcg_aware(lru)) >> rcu_read_unlock(); >>=20 >> Not concise. I don't know if this is good. >=20 > At such point, it's probably best to just: >=20 > if (list_lru_memcg_aware(lru)) { > rcu_read_lock(); > ret =3D list_lru_add(lru, item, nid, = mem_cgroup_from_slab_obj(item)); > rcu_read_unlock(); > } else { > list_lru_add(lru, item, nid, NULL); > } Good. Will update v2. Thanks. >=20 > ? >=20 >>=20 >>> } >>>=20 >>> Seems worthwhile?