Re: [PATCH v9 10/27] resource: harden resource_contains

On 1/21/25 23:01, Dan Williams wrote:

Alejandro Lucero Palau wrote:

quoted

On 1/18/25 02:03, Dan Williams wrote:

quoted

alejandro.lucero-palau@ wrote:

quoted

From: Alejandro Lucero <redacted>

While resource_contains checks for IORESOURCE_UNSET flag for the
resources given, if r1 was initialized with 0 size, the function
returns a false positive. This is so because resource start and
end fields are unsigned with end initialised to size - 1 by current
resource macros.

Make the function to check for the resource size for both resources
since r2 with size 0 should not be considered as valid for the function
purpose.

Signed-off-by: Alejandro Lucero <redacted>
Suggested-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <redacted>
---
   include/linux/ioport.h | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 5385349f0b8a..7ba31a222536 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h

@@ -296,6 +296,8 @@ static inline unsigned long resource_ext_type(const struct resource *res)
   /* True iff r1 completely contains r2 */
   static inline bool resource_contains(const struct resource *r1, const struct resource *r2)
   {
+	if (!resource_size(r1) || !resource_size(r2))
+		return false;

I just worry that some code paths expect the opposite, that it is ok to
pass zero size resources and get a true result.

That is an interesting point, I would say close to philosophic
arguments. I guess you mean the zero size resource being the one that is
contained inside the non-zero one, because the other option is making my
vision blurry. In fact, even that one makes me feel trapped in a
window-less room, in summer, with a bunch of economists, I mean
philosophers, and my phone without signal for emergency calls.

The regression risk is not philosophic relative to how long this
function has returned 'true' for the size == 0 case.

Would not this regression be a good thing?

Because I argue that is not the right thing to do.

quoted

But maybe it is just  my lack of understanding and there exists a good
reason for this possibility.

Questions like the following are good to answer when changing long
standing behavior:

Did you look at any other resource_contains() user to get a sense of
that regression risk?

Is the benefit to changing this higher than that risk?

Would it be better to just document the expectation that the caller
should only pass non-zero sized resources to this function?