From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 54A4C3C8721 for ; Fri, 10 Apr 2026 13:55:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775829309; cv=none; b=V74rlLotrRce/Sr+8nBONxSjXEbjaN9KmmHfm4DTNCEwGbCDWsPK9tPEb8AftsCCiK6eqXbKt/qt813uEX9LmObS3qq/0wS+3EpowoAsjqRfCfeO3myntRWMqI5XBzEXHlLwQjkabL/qzSWZJFTdbqKU9JbCM4cJHafasoQM5VE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775829309; c=relaxed/simple; bh=trKoI4Hn/kEu3b4jFhAlMdwBIggZuH+tnFVJh94uDwg=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=TxXtwqJGjYdm5vw7oE5SKQRSX8Bp6rRsMrol9/3icst8QF581ZR1IQLnS+4aU/5Drh53tP6YdBvphNg0yH2pB91ajdBtun29ZojAFSduIcBcmA4Yvh0kQ89Sk8+rWP9+Pu9NYzkfc/r3OLnwcgYJoJYCe0kriY4yqk4ULk7yiGI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=PHzl6lDj; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="PHzl6lDj" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C3222C19421; Fri, 10 Apr 2026 13:55:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1775829309; bh=trKoI4Hn/kEu3b4jFhAlMdwBIggZuH+tnFVJh94uDwg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=PHzl6lDj3eE3f7u3YbcqoSb9aDfj+KrMEfAiSaeHoptZzNj/zQZ6d6YAdnJt9CsfK 5eWugCDExfoi0HjDhe/1LFhwm5+N347s5dhd991iKEoU4YjMN8eeTKOisC2Mv03uxF hUfswlvGHrHe39K5i9pl4vDLSrTN69uiRCxHlF9o0XlncqaNtqxCKR5G29sK8ICwfN Egfn7/cFvp4mx7KmxWhEGKXwRkfXvlpd51jC+hBgcOuxE7wRs/q5oJPM0M6ORelTR7 TU3y1NHmqgqAOHGdsTZuI8CozT9FdSZNl0knYXJeSD7Ri6BUi06/MVzV0K3reMkAX0 7z6qe7TnMqdtA== From: SeongJae Park To: Liew Rui Yan Cc: SeongJae Park , damon@lists.linux.dev, linux-mm@kvack.org Subject: Re: (sashiko review) [PATCH v4 1/2] mm/damon/lru_sort: validate min_region_size to be power of 2 Date: Fri, 10 Apr 2026 06:55:00 -0700 Message-ID: <20260410135500.81989-1-sj@kernel.org> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260410094004.193555-1-aethernet65535@gmail.com> References: Precedence: bulk X-Mailing-List: damon@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan wrote: > Sashiko found some issues. Clarifying what is the following action for the sashiko finding would be helpful. For example, if you say "Sashiko found some issues, so I will post a new versiuon soon", and if I'm busy, I can save my time by saying "Ok, then I will skip or delaying review of this version and wait for your next review". Seems you think Sashiko found issues are not worthy to post a new version, though, so I'm commenting below. > > Forwarding full sashiko review in a reply format with my > inline comments below, for sharing details of my view and > doing followup discussions via mails if needed. > > > # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com > > # start of sashiko.dev inline review > > commit bc048f9325d07815d621e80f3c40408c0210af75 > > Author: Liew Rui Yan > > > > mm/damon/lru_sort: validate min_region_size to be power of 2 > > > > This commit adds early validation for min_region_sz in > > damon_lru_sort_apply_parameters(). If the user configures an addr_unit that > > produces a non-power-of-2 min_region_sz, it immediately returns -EINVAL to > > avoid setting maybe_corrupted and unexpectedly terminating kdamond. > > > > > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c > > > index 554559d729760..3fd176ef9d9c1 100644 > > > --- a/mm/damon/lru_sort.c > > > +++ b/mm/damon/lru_sort.c > > > @@ -294,6 +294,11 @@ static int damon_lru_sort_apply_parameters(void) > > > param_ctx->addr_unit = addr_unit; > > > param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1); > > > > > > + if (!is_power_of_2(param_ctx->min_region_sz)) { > > > + err = -EINVAL; > > > + goto out; > > > + } > > > + > > > > Is the DAMON sysfs interface also susceptible to this regression? > > > > When a user configures addr_unit via sysfs and writes 'on' to the state file, > > damon_sysfs_turn_damon_on() builds the context. The min_region_sz is > > calculated directly from addr_unit without a power-of-two check, and then > > damon_start() is called, bypassing the damon_commit_ctx() safeguard. > > > > Since the DAMON core uses min_region_sz as an alignment mask in macros like > > ALIGN() and ALIGN_DOWN(), can this cause memory corruption or infinite loops > > in functions like damon_fill_regions_holes() if a non-power-of-2 value is > > provided via sysfs? > > This issue has been discussed before [1]. While it's a valid concern, > it's outside the scope of this backport patch. SeongJae Park has > indicated he will work on this. That's correct. It's on my todo list. > > > > > This patch works around damon_commit_ctx() failing due to !is_power_of_2() > > by performing the check early, but does damon_commit_ctx() still have a > > regression leading to unexpected termination if other allocations fail? > > Yes, unexpected termination still exists. > > This patch provides a minimal workaround for the specific case of > non-power-of-2 'min_region_sz', which is suitable for backport. I agree. > > > > > If damon_commit_ctx() fails midway, for example returning -ENOMEM from > > damon_commit_schemes(), it leaves dst->maybe_corrupted = true and bails > > out without rolling back the partial updates. > > > > Even the DAMON sysfs interface, which attempts to mitigate this using a > > test_ctx dry-run, could experience a time-of-check to time-of-use race > > where the test allocation succeeds but the final commit allocation fails. > > > > Since these modules operate under memory pressure, if an -ENOMEM occurs > > during parameter updates, will the running reclaim daemon permanently > > observe maybe_corrupted = true and unexpectedly terminate itself? > > You're correct. If damon_commit_ctx() fails midway, it leaves > 'maybe_corrupted=true', causing kdamond to terminate permanently. This > is a core error handling issue that needs to be fixed, but it's outside > the scope of this backport patch. Agreed. This was unclear to me in previous disucssions, though. I still agree it is out of the scope of this patch. But now I think we need to let users force-restart. Adding this to my todo list. > > > > > > if (!damon_lru_sort_mon_attrs.sample_interval) { > > > err = -EINVAL; > > > goto out; > > > > > > # end of sashiko.dev inline review > > # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com Thanks, SJ [...]