From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.14]) (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 2B3AF2D47FF for ; Tue, 9 Dec 2025 15:48:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.14 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765295341; cv=none; b=iCgyxFSsFf84ZmqcR1srzO0s1kpmqtyN6QnQs+1krPLIKPTrKsuBQZy3iaAIMuWdXz+Hif2cda9n2G8lCpeiPVz3zeCPY25CLQ/vJSqXtAeFC0cDLiCS/gjOmSqd0PwpeAkOJOsoO1LOWtarTStW5PNPJ03Drw8oSBNgUqPwQFk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765295341; c=relaxed/simple; bh=DVKWrg8DMOWqMgfZ8HNLL/EQpH/Uy3Ps0+I6tfiezo8=; h=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=lJYH9+aT41wmLpuRbD3+UfiIyzNeArJClb2rIRIBcjS0VhAoZ2MiP4/UxxNFmV7MOWeLxi1d+ZgXRnQKoaOJqkTIittyKWDMrppDUyVLjW8mdoIa3NR5lpCGRCDJBwvCQW0wNCLfrgUdCgqyb5Pbdu8I/itTax+an5sxNGXsgoE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=KNM7lLBn; arc=none smtp.client-ip=198.175.65.14 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="KNM7lLBn" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1765295339; x=1796831339; h=from:date:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=DVKWrg8DMOWqMgfZ8HNLL/EQpH/Uy3Ps0+I6tfiezo8=; b=KNM7lLBnMskSBjHzWQuqL6FaU3IG/VG2f+PuClz0g7k6v+Hh3e2shQw8 0eudmiaJPJ9w4WW7ra0xbGE0VUGlx31KZsBf64rvlacO2v7/2hSWm2TtA ErH1rV4K1BylIoV0jri+ScFgfw/liNHT5WY83mnM3dsXxP64anelmHLr/ FqJL9sapLcHppCmEiNP2+o5OLw3u89qX+nlEgIxYN68AnllsjQ1BXivaH zWnzeovpVevrLaXzyc7JjW6n/wxgUXrCxjo1PeezjGvJYh2+410c88NIm mQhhIFxYPyW2UyEO0wvGJ0wCVJenl8s9vS68h+Trc65p6gG0Cq4pSUHu+ A==; X-CSE-ConnectionGUID: Kqfv+DhaTOylmrKtf3/J1A== X-CSE-MsgGUID: 9fTy6froTk2emk+14FFJlQ== X-IronPort-AV: E=McAfee;i="6800,10657,11637"; a="71115133" X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="71115133" Received: from fmviesa007.fm.intel.com ([10.60.135.147]) by orvoesa106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 07:48:58 -0800 X-CSE-ConnectionGUID: wmpUN0NhRfapY8iJf7JlMQ== X-CSE-MsgGUID: oA4oz/qoRuaDz5xHSl1e/w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="195860392" Received: from ijarvine-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.245.139]) by fmviesa007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 07:48:56 -0800 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Tue, 9 Dec 2025 17:48:53 +0200 (EET) To: Christian Marangi cc: Andrew Morton , Andy Shevchenko , LKML Subject: Re: [PATCH v2] resource: add WARN_ON_ONCE for resource_size() and document misusage In-Reply-To: <20251209150150.9525-1-ansuelsmth@gmail.com> Message-ID: <678c3644-47da-f72d-a72d-5cc9b4230a99@linux.intel.com> References: <20251209150150.9525-1-ansuelsmth@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323328-1313032559-1765293078=:1137" Content-ID: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1313032559-1765293078=:1137 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: QUOTED-PRINTABLE Content-ID: <29f7184c-01b1-3b05-2165-6173cc95c472@linux.intel.com> 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. >=20 > 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. >=20 > This is caused by the fact that resource_size() calculates the size > with the following logic: >=20 > =09end - start + 1 >=20 > that with an all zero resource descriptor: >=20 > =090 - 0 + 1 >=20 > returns 1. >=20 > One reason the BUG in the reference commit might have been introduced > is a logic error in the actual usage of resource_size(). >=20 > 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. >=20 > 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. >=20 > Hence it's ALWAYS wrong to initialize (and use) a resource descriptor > to all zero following the usual pattern: >=20 > =09struct resource res =3D {}; >=20 > The correct way to initialize an "uninitialized" resource descriptor woul= d > 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=20 not a resource type (IMO, it would be better to leave flags to zero=20 when type is not valid, and test for that and not IORESOURCE_UNSET). In any case, preferrably resource would be directly initialized with a=20 valid type, but that is not possible in the case of ath11k because the=20 called function is filling res. From=20the point of view of resource_size(), the more important aspect,=20 however, is that DEFINE_RES() handles the start and end address setup=20 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=20 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. >=20 > Link: https://lore.kernel.org/all/20251207215359.28895-1-ansuelsmth@gmail= =2Ecom/T/#m990492684913c5a158ff0e5fc90697d8ad95351b > Suggested-by: Ilpo J=E4rvinen > Signed-off-by: Christian Marangi > --- > Changes v2: > - Improve commit description > - Improve kdoc > - Add bug.h include >=20 > include/linux/ioport.h | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) >=20 > 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 @@ > =20 > #ifndef __ASSEMBLY__ > #include > +#include > #include > #include > #include > @@ -286,8 +287,30 @@ static inline void resource_set_range(struct resourc= e *res, > =09resource_set_size(res, size); > } > =20 > +/** > + * resource_size - Get the size of the resource > + * @res: Resource descriptor > + * > + * Calculated size is derived from @res end and start values following > + * the logic: > + * > + *=09end - 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 o= r > + * 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=20 wording above as it clearly states caller is responsible (instead of=20 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) > { > +=09WARN_ON_ONCE(!res->flags); > =09return res->end - res->start + 1; > } > static inline unsigned long resource_type(const struct resource *res) >=20 --=20 i. --8323328-1313032559-1765293078=:1137--