From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=95.215.58.170 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1792727205; b=l6Lr2RcrnJCCgxK7YfWNO9y2mKLAdaJxEx7W7aUqsSiBPWN64d9/TFiFv55m8S8eIATo cuUoInumhKjpkQUwKZsdSowPtYOpU8IB377YTowKjdWbpReW8A8BDrdoGWccXIsYlG+gU zcySVTYaieOXSuEK8h8TxC9Dh7dkAG3Bso37yWmQB3fkDmP8ZGe7FtKRHwegSD2nzV+XP WIxCJr7jsYX0pyLA2B7RnztZOokLWd8ROjpYvvxeQo+TZtjuJalnQLZHDWyA88IZ607ff 0/2Iy/ZhVHAbdarqGm54B2Ep+LD2V9BVlEwLipo03ZSA4/d94WHbrcqTzMbKcPM2HPQ== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1792727205; h=DMARC-Filter:X-Envelope-To:DKIM-Signature:X-Envelope-To:Received: X-Mizu-Trace-ID:X-Migadu-Flow:Message-ID:Date:MIME-Version:User-Agent: Subject:To:Cc:References:Content-Language:From:In-Reply-To: Content-Type:Content-Transfer-Encoding; bh=ttIYp9SKvrIeTjMeme1a7v/1DuKF5crN8WKfQ1/zVqQ=; b=e8Gl8eV9M4cmJkyQpZtnjqgPMw5v+AFn5vGw4YOuTvIdykYd+fvssOu8yVxLdmLS2R9S pF09r7mNauOTGPp1y1IjoYpfDtUAju+AwYi8Kn2TG7eQSHX3q+NtSIV8u1vP1vp7n1JDs aDuzpOwoVlGYElIfq3JMBhkp/u9rWBU/thiim+apsnYQg+fW05DzMpImKtfKGpe5ehY+P mKsLB+dzcYqAKRnOJTOHyA0WbEzNARHtUZLYKoTVVznRLeEIhwQHCqRAxQcYxM2Ee09lD qlT1lfLR40DgSNBfYilPFjvwjsqs8oE6Tq2g++z2mW87qJFLHOaJPgpNREU2JVxsiNw== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=V9SxOFsb; arc=none smtp.remote-ip=95.215.58.170 Received: from mta1.migadu.com (out-170.mta1.migadu.com [95.215.58.170]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B331606A31B for ; Fri, 23 Oct 2026 03:46:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org 4B331606A31B 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, unprotected) header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=V9SxOFsb X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=Hm72Xfl6pvbKv+hg7yoR4S7LNRVKFxz7eQHzNV7biQo=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1792727201; v=1; x=1793332001; b=V9SxOFsbi2u9Uu+fiTkvQf06afGaagMarU08jGszLxPu/I6Na+u6nAq40izPufIN91vd04Dw IFzE0t9wZSNARZlpsgISwjRMup02OGVKwqkBP/JkA+CYQ8gjn6ts7vwq/9CNsbcG4MPSrShDpns II31xlaPTyn+qGAfS/TnnPc0= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id c34def1092b89765; Fri, 23 Oct 2026 03:46:41 +0000 X-Mizu-Trace-ID: c34def1092b89765 X-Migadu-Flow: FLOW_OUT Message-ID: Date: Fri, 23 Oct 2026 11:46:36 +0800 X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] dax/bus: fix Device DAX range alignment validation To: Alison Schofield , Muchun Song Cc: Dan Williams , Vishal Verma , Dave Jiang , Andrew Morton , Joao Martins , nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org References: <20261016053944.2579541-1-songmuchun@bytedance.com> Content-Language: en-US From: Muchun Song In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2026/10/23 08:05, Alison Schofield wrote: > On Fri, Oct 16, 2026 at 01:39:44PM +0800, Muchun Song wrote: >> Only range sizes are currently validated against dev_dax->align. A >> dynamic device can therefore select a large-page alignment and allocate a >> range whose start is not naturally aligned. The device binds successfully, >> but a later write to a userspace mapping may trigger a kernel panic. >> >> The automatic resize path can also split a size-aligned request across >> arbitrary free gaps. When devices with different alignments fragment a >> region, this can extend a range by less than the device alignment. A later >> allocation then fails, leaving the failed resize partially applied. >> >> Initialize the device alignment before its first allocation. Validate both >> the start and size of allocated and adjusted ranges, account only for >> aligned free space before changing ranges, skip unusable gaps, and >> serialize size validation with alignment updates. >> >> Static Device DAX already has a pre-populated pgmap whose full range was >> validated against memremap_compat_align(). Its usable data range may begin >> after namespace metadata, so require only dev_dax->align for static >> devices. Dynamic Device DAX builds its pgmap from the allocated ranges and >> must continue to satisfy both alignments. > Hi Muchun, Hi, > > One commit message comment: > > Now that this is a standalone patch, some useful context from the original > cover letter would be useful in this commit message. OK, I'll add those info to commit message. > > The cover letter explained that Sashiko identified a path where > `memmap_init_zone_device()` could initialize struct pages with a large-page > order for a physical range that wasn't naturally aligned. You also mentioned > that you reproduced the resulting kernel panic. > > That gives us both the failure mechanism and confirmation that this is a > reproduced bug, rather than just a potential problem. Those details are worth > keeping in the commit message. > > So I happened to hit this and the mapping_store() issue that was in the > original patch 1/2. Both these fixes solved my problems. > > Below I do have some style and simplification feedback, and am also wondering > about the gap handling logic. > > >> Fixes: 6d82120f4156 ("device-dax: add an 'align' attribute") >> Assisted-by: LLM >> Signed-off-by: Muchun Song >> --- >> v4: >> - Use dev_dax->align alone for static Device DAX ranges to preserve >> older 4K namespaces (reported by Alison Schofield) >> - Condense the commit message to the failure scenarios and required fixes >> (suggested by Alison Schofield) >> >> v3: >> - Serialize size alignment validation with alignment updates to avoid a >> partial shrink after a concurrent alignment change (reported by Sashiko) >> - Document the size validation serialization in the commit message >> >> v2: >> - Keep the alloc_is_aligned() name to avoid unrelated helper renaming >> (suggested by Dave Jiang) >> - Move this patch after the mapping error reporting fix for independent >> backports (suggested by Dave Jiang) >> --- >> drivers/dax/bus.c | 152 +++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 117 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c >> index b809e1a264af..b2c83a0e4ec5 100644 >> --- a/drivers/dax/bus.c >> +++ b/drivers/dax/bus.c >> @@ -848,6 +848,52 @@ static int devm_register_dax_mapping(struct dev_dax *dev_dax, int range_id) >> return 0; >> } >> >> +static inline unsigned long dev_dax_min_align(struct dev_dax *dev_dax) > Why the static inline here and also why below do you change > alloc_is_aligned() to static inline? I thought it is a one-line code, inline is better, I usually do this in MM area. > > coding-style.rst is against it in .c files and we don't do it in any > other drivers/dax/*.c code. Seems it is not a convention in DAX. I am not an expert in DAX. > >> +{ >> + /* >> + * A static device has a pre-populated pgmap whose range already >> + * satisfies memremap_compat_align(). Its data range may start after >> + * metadata at dev_dax->align granularity. >> + */ >> + if (static_dev_dax(dev_dax)) >> + return dev_dax->align; >> + >> + /* >> + * The minimum mapping granularity for a dynamic device instance is a >> + * single subsection, unless the arch says otherwise. >> + */ >> + return max_t(unsigned long, dev_dax->align, memremap_compat_align()); >> +} >> + >> +static inline bool alloc_is_aligned(struct dev_dax *dev_dax, resource_size_t size) >> +{ >> + return IS_ALIGNED(size, dev_dax_min_align(dev_dax)); >> +} >> + >> +static inline bool range_is_aligned(struct dev_dax *dev_dax, u64 start, >> + resource_size_t size) >> +{ >> + return IS_ALIGNED(start | size, dev_dax_min_align(dev_dax)); >> +} >> + >> +static resource_size_t >> +aligned_gap_size(struct dev_dax *dev_dax, resource_size_t *start, >> + resource_size_t end) > > Please keep the return type on the same line as the function name, like > rest of this file. > > Also a function comment like: > /* Return usable aligned gap size and update *aligned_start */ > > and a rename of *start to *aligned_start would make the contract clearer. Will do. > > >> +{ >> + resource_size_t aligned_start = ALIGN(*start, dev_dax_min_align(dev_dax)); >> + resource_size_t size; > > How about calculating dev_dax_min_align() once and keep it in a local variable? > We use the same value again for ALIGN_DOWN(). Make sense. > > >> + >> + if (aligned_start < *start || aligned_start > end) >> + return 0; >> + >> + size = ALIGN_DOWN(end - aligned_start + 1, dev_dax_min_align(dev_dax)); >> + if (!size) >> + return 0; > The above early return looks needless given how callers use the result Oh, yes. >> + >> + *start = aligned_start; >> + return size; >> +} >> + > snip > >> >> +static resource_size_t >> +dax_region_aligned_avail_size(struct dax_region *dax_region, >> + struct dev_dax *dev_dax) > Same as previous, keep return type on the same line here. > > The dax_region argument looks needless as we can get it from > dev-dax->region. Right. My oversight. > > > snip > > >> @@ -1062,37 +1130,51 @@ static ssize_t dev_dax_resize(struct dax_region *dax_region, >> */ >> retry: >> first = region_res->child; >> - if (!first) >> - return alloc_dev_dax_range(dev_dax, dax_region->res.start, to_alloc); >> + if (!first) { >> + resource_size_t start = region_res->start; >> + >> + alloc = aligned_gap_size(dev_dax, &start, region_res->end); >> + return alloc_dev_dax_range(dev_dax, start, min(alloc, to_alloc)); >> + } >> >> rc = -ENOSPC; >> for (res = first; res; res = res->sibling) { >> struct resource *next = res->sibling; >> + resource_size_t start, end; >> >> /* space at the beginning of the region */ >> if (res == first && res->start > dax_region->res.start) { >> - alloc = min(res->start - dax_region->res.start, to_alloc); >> - rc = alloc_dev_dax_range(dev_dax, dax_region->res.start, alloc); >> - break; >> + start = dax_region->res.start; >> + end = res->start - 1; >> + alloc = min(aligned_gap_size(dev_dax, &start, end), to_alloc); >> + if (alloc) { >> + rc = alloc_dev_dax_range(dev_dax, start, alloc); >> + break; >> + } >> } >> >> - alloc = 0; >> /* space between allocations */ >> - if (next && next->start > res->end + 1) >> - alloc = min(next->start - (res->end + 1), to_alloc); >> - >> - /* space at the end of the region */ >> - if (!alloc && !next && res->end < region_res->end) >> - alloc = min(region_res->end - res->end, to_alloc); >> + if (next) { >> + if (next->start <= res->end + 1) >> + continue; >> + end = next->start - 1; >> + } else { >> + /* space at the end of the region */ >> + if (res->end >= region_res->end) >> + continue; >> + end = region_res->end; >> + } >> >> + start = res->end + 1; >> + alloc = min(aligned_gap_size(dev_dax, &start, end), to_alloc); > We're walking the free gaps twice now. Once in dax_region_aligned_avail_size() > and again here. It seems the 2 walks need to agree about which gaps > are usable so that the pre check doesn't succeed and then the allocation > loop cannot satify the request...which would get us right back to a > partially applied resize. > > Please look at sharing the gap traversal. Good point. I will factor the gap traversal into a common iterator used by both the pre-check and the allocation path, so they share the same gap selection rules. I will include this in the next revision. Thanks for your review. Thanks, Muchun > > -- Alison > > snip