Message-ID: <dc286824-ec2c-ce22-bc0d-1dbe688dc5ab@linux.intel.com>
Date: Tue, 9 Dec 2025 18:45:04 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Christian Marangi <ansuelsmth@...il.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, 9 Dec 2025, Christian Marangi wrote:
> 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())

Requiring to use DEFINE_RES_UNSET() (or like) risks many devs not knowing 
about it and using DEFINE_RES() instead.

One could consider making a plain DEFINE_RES() without any arguments 
default to this behavior using preprocessor argument 
COUNT_ARGS(__VA_ARGS__) trickery (used e.g. in 
pci_dev_for_each_resource() if you want an example how it invokes other 
macros).

So this code would result in start = 0, end = -1, flags = 
IORESOURCE_UNSET:

	struct resource res = DEFINE_RES();

-- 
 i.

> > 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.
> 
> 
> 

Powered by blists - more mailing lists