Message-ID: <693848ea.050a0220.1466f.6473@mx.google.com>
Date: Tue, 9 Dec 2025 17:06:00 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] resource: add WARN_ON_ONCE for resource_size() and
 document misusage

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 <ilpo.jarvinen@...ux.intel.com>
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> > 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 <linux/bits.h>
> > +#include <linux/bug.h>
> >  #include <linux/compiler.h>
> >  #include <linux/minmax.h>
> >  #include <linux/types.h>
> > @@ -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

Powered by blists - more mailing lists