Message-ID: <678c3644-47da-f72d-a72d-5cc9b4230a99@linux.intel.com>
Date: Tue, 9 Dec 2025 17:48:53 +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:

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

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