From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8E479237180 for ; Tue, 9 Dec 2025 16:06:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765296367; cv=none; b=TSEHZ2RlULiOebqVpBC5X5gb9x4hOOm448QV6OcGrv6fLm4udbxpkkqDKiBaApGW+PF/tNZXS7tPrI+C36Ml6v1+ybwU7uUIoo7IBChmI8CPBdlSRU6w9EmhkvV/i1BBsSwODh9VTzjhTmK8W58WTSwcmbZ4oPbSYodeTE9tkoM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765296367; c=relaxed/simple; bh=3qbQeTf60d3kprRoHOPX8uNaC8x1ZQfAEHBaQTPDBBs=; h=Message-ID:Date:From:To:Cc:Subject:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nqx2AFH47epfjzmuhBs3/sIjG3WQTu2R0ut5XZu9juVJ2j9AcCSJqOgTXfvtsP0BSuUSznf00pOut1Fa/1RFBgUOMMy+pVnfjTi9FmUXh1UTtDEl9ON/qqnpWtX57QWB0cveC4oc6Bcnxkrvjczg+6VSPSXSz11GNrkwaPH5ba0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=STSwHmD3; arc=none smtp.client-ip=209.85.128.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="STSwHmD3" Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-477a219dbcaso60420465e9.3 for ; Tue, 09 Dec 2025 08:06:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765296364; x=1765901164; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=fuZ0EsubI5smYv/b4PMZ0V2uDnToGyeEmnLtjnm9K8k=; b=STSwHmD31mkc0pkSH0STHVBenR+GyZgc7cJbDIuIHAMzFYM2re+mXW9j8GJA0Jk1Fj dbr/ZTOjQxlI8EZNdBRw71NbPQCqf1xQqv8r4tkV5UC7+lp9DTbI8myrhski6jaKV7AD CHpXNbKHXluFcJhsckov1lSxyDWio7FfgBa/bXc0XGy6GAGZEiW4GS00SPtQc3mUQCcO PUQV+mR2tLIdcFfq1k9x0zLrbnKaKAwmW5NWVl+H8h/ZHZ4EUNozOrcKVqVrIqdN5XJk R7fTiy58dl3o4alfdBmyuEtDMCwPz6IL7hQhXS8lOi8YXDYePmx+JCiMZ4w3vkQnTm9z 0Z3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765296364; x=1765901164; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=fuZ0EsubI5smYv/b4PMZ0V2uDnToGyeEmnLtjnm9K8k=; b=AaI4rmXWG9b1rvBFzCmPPwiu6Xt2/NwfuxCXUj0Wz4VXwkKFrHuKbZ9xwi8OChj/t5 BufbdNzaFCm9dEgLDwKUnDaIvwTODI90T/Q9NYsCSftuZpi8+8MwdTNHcI2TRLDY56mq 5uBpsNmOXqz6DPea0H532N6qrEo7hzGcpImJnAyAoYtjRGYkiZj8Xz4pzuq0Bm8DWckH TwueSCYWIFEbCfKbfzvPmy8KI5woMGU/gATPACji/syFCi5ozaOQBEttcb70FxCeeZs2 SloLVYuBzz1EzLS6JYLMthWzyszNMtio69aAod7STYwlm1FEAZ7uXCtq6/CGbWDaaEaB pivA== X-Forwarded-Encrypted: i=1; AJvYcCVvRTS1ez/R6vn0RrM9cDsBF5MqyatrwOyPwGH+otF5AhWBkqRY9qU2za9IuzHaQAYH5iP/4Wd3DWwdRxA=@vger.kernel.org X-Gm-Message-State: AOJu0Yx0UM/KzY+u5ufnNROVbeAvdYn5rj9jOzER97W/btjXgi90ssUc ukhRdJnmrfccns2FZTS3Xl+OcO+Pj7affJdaYxL60ijUSoRTe70NdI6bTBXwNA== X-Gm-Gg: ASbGncu2wcwPBp2V7Yfy534n2BznDV6lgS9mlLnTa4v2zMKHnZtMQ4LEBeuqOKWNcFn Q+o2EibVqh1p+K4PYenlKG0ce2usc0UZkl+DB0XRV6FCiVLrJ1eOHIzoYQu/FkF8IRxIzPUg0YB sW4Yzybmi7yq0888YLzJ1Ki9Ncrdr0RHXySuQTSNZVXke3LpOn7VqwaGRQRL+/O6zm5zD5tsZ8k tqp71pIm+qVF5G5l0oCq03QDYrGCYzloIx5ClUvGCguk0N9q6285dmQ7cu1u7CIwpX7De87QKHv t7/9E6qsedYO3nqx/K8yYFA7OGmiz8lr/ElCWDj/D4cGb+M+QDPcCRx9AVIOBKTVZihS6xD9ZoB bt1Mb3HFjUXJgAVALlFChIbgU2NkFwsUGf/bx2DWfQtUsXrPhrJbAnHj6r5L3hJt+Ui3SmsuOpl cYrLIhkZG4lxon5hKeSashwYGkYAVz8c+OzUX323o= X-Google-Smtp-Source: AGHT+IGUGydQUuhtnYzOezJlfzSIoU6IiMiV9Nj+nbWk7Br2tefVB8rvhgHRp8SYOP5iQh3xEH9Xiw== X-Received: by 2002:a05:600c:4691:b0:477:63b5:7148 with SMTP id 5b1f17b1804b1-47939df150cmr113749275e9.6.1765296363591; Tue, 09 Dec 2025 08:06:03 -0800 (PST) Received: from Ansuel-XPS. (93-34-88-81.ip49.fastwebnet.it. [93.34.88.81]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47a7d6fa121sm44319575e9.5.2025.12.09.08.06.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Dec 2025 08:06:02 -0800 (PST) Message-ID: <693848ea.050a0220.1466f.6473@mx.google.com> X-Google-Original-Message-ID: Date: Tue, 9 Dec 2025 17:06:00 +0100 From: Christian Marangi To: Ilpo =?iso-8859-1?Q?J=E4rvinen?= Cc: Andrew Morton , Andy Shevchenko , LKML Subject: Re: [PATCH v2] resource: add WARN_ON_ONCE for resource_size() and document misusage References: <20251209150150.9525-1-ansuelsmth@gmail.com> <678c3644-47da-f72d-a72d-5cc9b4230a99@linux.intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <678c3644-47da-f72d-a72d-5cc9b4230a99@linux.intel.com> On Tue, Dec 09, 2025 at 05:48:53PM +0200, Ilpo Järvinen wrote: > On Tue, 9 Dec 2025, Christian Marangi wrote: > > > Commit 900730dc4705 ("wifi: ath: Use > > of_reserved_mem_region_to_resource() for "memory-region"") uncovered a > > fragility in the usage of the resource_size() helper that might result > > in its misusage as a way to check for initialization of a passed resource > > descriptor. > > > > In the referenced commit, resource_size() is wrongly assumed to return > > 0 when a resource descriptor is init to all zero while in reality it > > would return 1. > > > > This is caused by the fact that resource_size() calculates the size > > with the following logic: > > > > end - start + 1 > > > > that with an all zero resource descriptor: > > > > 0 - 0 + 1 > > > > returns 1. > > > > One reason the BUG in the reference commit might have been introduced > > is a logic error in the actual usage of resource_size(). > > > > Historically, it was assumed that resource_size() was ALWAYS > > used AFTER APIs filled the data of the resource descriptor (or in case of > > any error from such APIs, resource descriptor set to an invalid state) > > Missing final . > > > But lack of comments on what should be the proper usage of > > resource_size() might have introduced some confusion in the specific > > case of passing a resource descriptor initialized to all zeros. > > > > As described in the example, using resource_size() for a resource > > descriptor that has zero start and end yields to resource size of 1 > > (this is correct and necessary behavior!) which may beconfusing to > > be confusing > > > some callers. > > > > Hence it's ALWAYS wrong to initialize (and use) a resource descriptor > > to all zero following the usual pattern: > > > > struct resource res = {}; > > > > The correct way to initialize an "uninitialized" resource descriptor would > > be to use DEFINE_RES macro ideally with a proper type set to it > > (for example by initializing it to zero start/size and IORESOURCE_UNSET). > > I don't exactly like the wording here as technically IORESOURCE_UNSET is > not a resource type (IMO, it would be better to leave flags to zero > when type is not valid, and test for that and not IORESOURCE_UNSET). > Yes I guess IORESOURCE_UNSET is strictly a flag than a type. Maybe a bit OT but I think it's sensible to define for any future fix related to this. My idea here is to give good practice for the case of defining a zero resource descriptor. I feel leaving the flags as zero might pose the same current problem with user still declaring resource descriptor with struct resource res = {}; and then checking with if (!res.flags) { ... Setting the flags with IORESOURCE_UNSET seems more robust to me. And with this pattern we can also introduce 2 helper. (example DEFINE_RES_UNSET and resource_is_unsed()) > In any case, preferrably resource would be directly initialized with a > valid type, but that is not possible in the case of ath11k because the > called function is filling res. > > From the point of view of resource_size(), the more important aspect, > however, is that DEFINE_RES() handles the start and end address setup > correctly. > > > To catch any possible misusage of resource_size() helper, emit a WARN if > > we detect the passed resource descriptor have zeroed flags. This would > > signal the resource descriptor is not correctly inizialized and will > > initialized > > > probably result in resource_size() returning unexpected sizes (for > > example returning 1 if the resource descriptor is all set to zero). > > I'd remove the parenthesis part as it is already covered by what was > said above. > > > Also add kernel doc to resource_size() that in conjunction of WARN > > should prevent from now on any possible misusage of this helper and > > permit to catch and fix any possible BUG caused by this logic confusion. > > > > Link: https://lore.kernel.org/all/20251207215359.28895-1-ansuelsmth@gmail.com/T/#m990492684913c5a158ff0e5fc90697d8ad95351b > > Suggested-by: Ilpo Järvinen > > Signed-off-by: Christian Marangi > > --- > > Changes v2: > > - Improve commit description > > - Improve kdoc > > - Add bug.h include > > > > include/linux/ioport.h | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > > index e8b2d6aa4013..c087e49e1927 100644 > > --- a/include/linux/ioport.h > > +++ b/include/linux/ioport.h > > @@ -11,6 +11,7 @@ > > > > #ifndef __ASSEMBLY__ > > #include > > +#include > > #include > > #include > > #include > > @@ -286,8 +287,30 @@ static inline void resource_set_range(struct resource *res, > > resource_set_size(res, size); > > } > > > > +/** > > + * resource_size - Get the size of the resource > > + * @res: Resource descriptor > > + * > > + * Calculated size is derived from @res end and start values following > > + * the logic: > > + * > > + * end - start + 1 > > + * > > + * This MUST be used ONLY with correctly initialized @res descriptor. > > + * > > + * Do NOT use resource_size() as a proxy for checking validity of @res or > > + * for checking if @res is in a resource tree (use flags checks or call > > + * resource_assigned() instead). > > + * > > + * The caller MUST ensure @res is properly initialized, passing a @res > > This is repeating what is above but I'd not remove this but use this > wording above as it clearly states caller is responsible (instead of > a passive voice). > > > + * descriptor with zeroed flags will produce a WARN signaling a misusage > > + * of this helper and probably a BUG in the user of this helper. > > + * > > + * Return: size of the resource. > > + */ > > static inline resource_size_t resource_size(const struct resource *res) > > { > > + WARN_ON_ONCE(!res->flags); > > return res->end - res->start + 1; > > } > > static inline unsigned long resource_type(const struct resource *res) > > > > -- > i. -- Ansuel