| Message ID | 20241230214445.27602-11-alejandro.lucero-palau@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
alejandro.lucero-palau@ wrote: > From: Alejandro Lucero <alucerop@amd.com> > > 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 <alucerop@amd.com> > 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 <Jonathan.Cameron@huawei.com> > --- > 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. Did you audit existing callers?
On 1/18/25 02:03, Dan Williams wrote: > alejandro.lucero-palau@ wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> 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 <alucerop@amd.com> >> 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 <Jonathan.Cameron@huawei.com> >> --- >> 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. But maybe it is just my lack of understanding and there exists a good reason for this possibility. Bjorn, I guess the ball is in your side ... > Did you audit existing callers?
Adding Bjorn to the thread. Not sure if he just gets the email being in an Acked-by line. On 1/20/25 16:10, Alejandro Lucero Palau wrote: > > On 1/18/25 02:03, Dan Williams wrote: >> alejandro.lucero-palau@ wrote: >>> From: Alejandro Lucero <alucerop@amd.com> >>> >>> 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 <alucerop@amd.com> >>> 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 <Jonathan.Cameron@huawei.com> >>> --- >>> 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. > > > But maybe it is just my lack of understanding and there exists a good > reason for this possibility. > > > Bjorn, I guess the ball is in your side ... > >> Did you audit existing callers?
On 1/20/25 16:16, Alejandro Lucero Palau wrote: > Adding Bjorn to the thread. Not sure if he just gets the email being > in an Acked-by line. > > > On 1/20/25 16:10, Alejandro Lucero Palau wrote: >> >> On 1/18/25 02:03, Dan Williams wrote: >>> alejandro.lucero-palau@ wrote: >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> 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 <alucerop@amd.com> >>>> 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 <Jonathan.Cameron@huawei.com> >>>> --- >>>> 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. >> I forgot to make my strongest point :-). If someone assumes it is or it should be true a zero-size resource is contained inside a non zero-size resource, we do not need to call a function since it is always true regardless of the non zero-size resource ... that headache is starting again ... >> >> But maybe it is just my lack of understanding and there exists a >> good reason for this possibility. >> >> >> Bjorn, I guess the ball is in your side ... >> >>> Did you audit existing callers?
On Mon, Jan 20, 2025 at 04:26:42PM +0000, Alejandro Lucero Palau wrote: > > On 1/20/25 16:16, Alejandro Lucero Palau wrote: > > Adding Bjorn to the thread. Not sure if he just gets the email being in > > an Acked-by line. > > > > > > On 1/20/25 16:10, Alejandro Lucero Palau wrote: > > > > > > On 1/18/25 02:03, Dan Williams wrote: > > > > alejandro.lucero-palau@ wrote: > > > > > From: Alejandro Lucero <alucerop@amd.com> > > > > > > > > > > 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 <alucerop@amd.com> > > > > > 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 <Jonathan.Cameron@huawei.com> > > > > > --- > > > > > 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. > > > > > I forgot to make my strongest point :-). If someone assumes it is or it > should be true a zero-size resource is contained inside a non zero-size > resource, we do not need to call a function since it is always true > regardless of the non zero-size resource ... that headache is starting again > ... > > Maybe start using IORESOURCE_UNSET flag - Looking back on when we first discussed this - https://lore.kernel.org/linux-cxl/Zz-fVWhTOFG4Nek-@aschofie-mobl2.lan/ where the thought was that checking for zero was helpful to all. If this path starts using the IORESOURCE_UNSET flag can it accomplish the same thing? No need to touch resource_contains(). Is that an option? -- Alison > > > > > > But maybe it is just my lack of understanding and there exists a > > > good reason for this possibility. > > > > > > > > > Bjorn, I guess the ball is in your side ... > > > > > > > Did you audit existing callers? >
Alejandro Lucero Palau wrote: > > On 1/18/25 02:03, Dan Williams wrote: > > alejandro.lucero-palau@ wrote: > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> 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 <alucerop@amd.com> > >> 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 <Jonathan.Cameron@huawei.com> > >> --- > >> 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. > 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?
On 1/21/25 20:38, Alison Schofield wrote: > On Mon, Jan 20, 2025 at 04:26:42PM +0000, Alejandro Lucero Palau wrote: >> On 1/20/25 16:16, Alejandro Lucero Palau wrote: >>> Adding Bjorn to the thread. Not sure if he just gets the email being in >>> an Acked-by line. >>> >>> >>> On 1/20/25 16:10, Alejandro Lucero Palau wrote: >>>> On 1/18/25 02:03, Dan Williams wrote: >>>>> alejandro.lucero-palau@ wrote: >>>>>> From: Alejandro Lucero <alucerop@amd.com> >>>>>> >>>>>> 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 <alucerop@amd.com> >>>>>> 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 <Jonathan.Cameron@huawei.com> >>>>>> --- >>>>>> 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. >>>> >> I forgot to make my strongest point :-). If someone assumes it is or it >> should be true a zero-size resource is contained inside a non zero-size >> resource, we do not need to call a function since it is always true >> regardless of the non zero-size resource ... that headache is starting again >> ... >> >> > Maybe start using IORESOURCE_UNSET flag - > > Looking back on when we first discussed this - > https://lore.kernel.org/linux-cxl/Zz-fVWhTOFG4Nek-@aschofie-mobl2.lan/ > where the thought was that checking for zero was helpful to all. > > If this path starts using the IORESOURCE_UNSET flag can it accomplish > the same thing? No need to touch resource_contains(). > > Is that an option? I think those are not mutually exclusive. The main reason for this change is hardening, in this particular case a resource definition/initialization apparently right, leading to this function returning something it should not. Even if you suggest the solution is hardening the resource definition/initialization, what I agree it is another thing to look at, I would leave this extra check here for correctness. This is assuming there is no case for what Dan mentioned and therefore auditing the callers being necessary. > -- Alison > > > >>>> But maybe it is just my lack of understanding and there exists a >>>> good reason for this possibility. >>>> >>>> >>>> Bjorn, I guess the ball is in your side ... >>>> >>>>> Did you audit existing callers?
On 1/21/25 23:01, Dan Williams wrote: > Alejandro Lucero Palau wrote: >> On 1/18/25 02:03, Dan Williams wrote: >>> alejandro.lucero-palau@ wrote: >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> 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 <alucerop@amd.com> >>>> 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 <Jonathan.Cameron@huawei.com> >>>> --- >>>> 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. >> 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?
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; if (resource_type(r1) != resource_type(r2)) return false; if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET)