linux-kernel - Re: [PATCH] resource: add WARN_ON_ONCE for resource

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c80f567-d632-b37e-c419-db059b1f2771@linux.intel.com>
Date: Tue, 9 Dec 2025 12:25:38 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Christian Marangi <ansuelsmth@...il.com>
cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    Andrew Morton <akpm@...ux-foundation.org>, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] resource: add WARN_ON_ONCE for resource_size() and
 document misusage

On Mon, 8 Dec 2025, Christian Marangi wrote:

> 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
> probably result in resource_size() returning unexpected values. (for
> example returning 1 if the resource descriptor is all set to zero)

Move . after the () part. Or make the parenthesis part a real sentence.

In general, you should first explain the problem in the changelog and then 
the solution, not to go directly to the solution. Especially in the 
changes such as this that are non-trivial to understand and are meant to 
enforce good code hygiene/standard. So I think you should reorder some of 
the sentences to fix the ordering.

> Historically, it was assumed that resource_size was ALWAYS used AFTER

For function names, always use () in the changelog.

> correct API fill the data of the resource descriptor (or errors out)

Add .

Somehow I fail to connect that "(or errors out)" to rest of what is being 
said in the sentence, maybe rephrase it?

> 
> But lack of comments might have introduced some logic error and

some logic error -> logic errors

> confusion in any user of resource_size() with it used on resource
> description initialized to all zero.

description -> descriptor

It's hard to understand what this tries to say, I suggest you try to 
rephrase it. Maybe something along these lines:

... and using resource_size() for a resource descriptor that is
initialized to all zeros. 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 some callers.

> The normal way to inizialize an "uninizialized" resource descriptor

2x iniz typos

> would be to use DEFINE_RES macro or resource_set_range() ideally with a
> proper flag set to it.

proper flag -> proper type ?

I'd remove the resource_set_range() part actually since it's meant to be 
used for a resource that is already initialized and the code only wants to 
change the address range.

> Hence initializing a resource descriptor to all zero and passing it to
> resource_size() would actually produce a size of 1.

This would naturally belong earlier when you talk about using 
resource_size() for a descriptor that is initialized to all zero.
(I ended up writing this into the earlier suggestion how to rephrase 
things so if you use it, you can drop this as it then becomes duplicate).

> Correct comments on the usage of this helper in conjunction of WARN

Add kernel doc to resource_size() ...

> should prevent from now on any possible misusage of this 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>
> ---
>  include/linux/ioport.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index e8b2d6aa4013..8b60f820993c 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -286,8 +286,20 @@ 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
> + *
> + * This MUST be used ONLY with correctly inizialized resource descriptor.

initialized

I'd add:

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

> + * Passing a resource descriptor with zeroed flags will produce a WARN

The caller must ensure @res is properly initialized.

> + * signaling a misusage of this helper and probably a BUG in the user
> + * of this helper.
> + *
> + * Return: Size of the resource calculated from resource end - start + 1.

I'd put this into the main description before the notes/warning on what to 
not do. And only say here:

 * Return: size of the resource.

> + */
>  static inline resource_size_t resource_size(const struct resource *res)
>  {
> +	WARN_ON_ONCE(!res->flags);

As lkp also found out, you need to add #include <linux/bug.h>

>  	return res->end - res->start + 1;
>  }
>  static inline unsigned long resource_type(const struct resource *res)
> 

-- 
 i.