diff mbox series

[v5,10/27] cxl: harden resource_contains checks to handle zero size resources

Message ID 20241118164434.7551-11-alejandro.lucero-palau@amd.com (mailing list archive)
State Not Applicable
Headers show
Series cxl: add type2 device basic support | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 6 maintainers not CCed: alison.schofield@intel.com dave.jiang@intel.com dave@stgolabs.net jonathan.cameron@huawei.com ira.weiny@intel.com vishal.l.verma@intel.com
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Lucero Palau, Alejandro Nov. 18, 2024, 4:44 p.m. UTC 
From: Alejandro Lucero <alucerop@amd.com>

For a resource defined with size zero, resource_contains returns
always true.

Add resource size check before using it.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/hdm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Dave Jiang Nov. 19, 2024, 6 p.m. UTC | #1 
On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> For a resource defined with size zero, resource_contains returns
> always true.
> 
> Add resource size check before using it.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Should this be broken out and send ahead of the type2 series?

nit below

> ---
>  drivers/cxl/core/hdm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 223c273c0cd1..c58d6b8f9b58 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  	cxled->dpa_res = res;
>  	cxled->skip = skipped;
>  
> -	if (resource_contains(&cxlds->pmem_res, res))
> +	if (resource_size(&cxlds->pmem_res) &&
> +	    resource_contains(&cxlds->pmem_res, res)) {
>  		cxled->mode = CXL_DECODER_PMEM;
> -	else if (resource_contains(&cxlds->ram_res, res))
> +	} else if (resource_size(&cxlds->ram_res) &&
> +		   resource_contains(&cxlds->ram_res, res)) {
>  		cxled->mode = CXL_DECODER_RAM;
> +	}
>  	else {

} else {
>  		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
>  			 port->id, cxled->cxld.id, cxled->dpa_res);
Zhi Wang Nov. 19, 2024, 7:50 p.m. UTC | #2 
On Mon, 18 Nov 2024 16:44:17 +0000
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> For a resource defined with size zero, resource_contains returns
> always true.
> 
> Add resource size check before using it.
> 

Does this trigger a bug? Looks like this should be with a Fixes: tag?

Z.

> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/hdm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 223c273c0cd1..c58d6b8f9b58 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct
> cxl_endpoint_decoder *cxled, cxled->dpa_res = res;
>  	cxled->skip = skipped;
>  
> -	if (resource_contains(&cxlds->pmem_res, res))
> +	if (resource_size(&cxlds->pmem_res) &&
> +	    resource_contains(&cxlds->pmem_res, res)) {
>  		cxled->mode = CXL_DECODER_PMEM;
> -	else if (resource_contains(&cxlds->ram_res, res))
> +	} else if (resource_size(&cxlds->ram_res) &&
> +		   resource_contains(&cxlds->ram_res, res)) {
>  		cxled->mode = CXL_DECODER_RAM;
> +	}
>  	else {
>  		dev_warn(dev, "decoder%d.%d: %pr mixed mode not
> supported\n", port->id, cxled->cxld.id, cxled->dpa_res);
Alejandro Lucero Palau Nov. 20, 2024, 1:44 p.m. UTC | #3 
On 11/19/24 18:00, Dave Jiang wrote:
>
> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> For a resource defined with size zero, resource_contains returns
>> always true.
>>
>> Add resource size check before using it.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>
> Should this be broken out and send ahead of the type2 series?


I could. This seems not a problem though until the new code coming along.


> nit below
>
>> ---
>>   drivers/cxl/core/hdm.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 223c273c0cd1..c58d6b8f9b58 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>>   	cxled->dpa_res = res;
>>   	cxled->skip = skipped;
>>   
>> -	if (resource_contains(&cxlds->pmem_res, res))
>> +	if (resource_size(&cxlds->pmem_res) &&
>> +	    resource_contains(&cxlds->pmem_res, res)) {
>>   		cxled->mode = CXL_DECODER_PMEM;
>> -	else if (resource_contains(&cxlds->ram_res, res))
>> +	} else if (resource_size(&cxlds->ram_res) &&
>> +		   resource_contains(&cxlds->ram_res, res)) {
>>   		cxled->mode = CXL_DECODER_RAM;
>> +	}
>>   	else {
> } else {


I'll fix it.

Thanks!


>>   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
>>   			 port->id, cxled->cxld.id, cxled->dpa_res);
Alejandro Lucero Palau Nov. 20, 2024, 1:45 p.m. UTC | #4 
On 11/19/24 19:50, Zhi Wang wrote:
> On Mon, 18 Nov 2024 16:44:17 +0000
> <alejandro.lucero-palau@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> For a resource defined with size zero, resource_contains returns
>> always true.
>>
>> Add resource size check before using it.
>>
> Does this trigger a bug? Looks like this should be with a Fixes: tag?


It seems it does not until the new code for type2 support.


> Z.
>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/hdm.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 223c273c0cd1..c58d6b8f9b58 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct
>> cxl_endpoint_decoder *cxled, cxled->dpa_res = res;
>>   	cxled->skip = skipped;
>>   
>> -	if (resource_contains(&cxlds->pmem_res, res))
>> +	if (resource_size(&cxlds->pmem_res) &&
>> +	    resource_contains(&cxlds->pmem_res, res)) {
>>   		cxled->mode = CXL_DECODER_PMEM;
>> -	else if (resource_contains(&cxlds->ram_res, res))
>> +	} else if (resource_size(&cxlds->ram_res) &&
>> +		   resource_contains(&cxlds->ram_res, res)) {
>>   		cxled->mode = CXL_DECODER_RAM;
>> +	}
>>   	else {
>>   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not
>> supported\n", port->id, cxled->cxld.id, cxled->dpa_res);
Alison Schofield Nov. 21, 2024, 2:46 a.m. UTC | #5 
On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> For a resource defined with size zero, resource_contains returns
> always true.
> 
I'm not following the premise above -

Looking at resource_contains() and the changes made below,
it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res
being zero - because we already checked that the second param
'res' is not zero a few lines above.

Looking at what happens when r1 is of size 0, I don't see how
resource_contains() returns always true.

In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end.
The func can only return true if r2 is also of size 0 and located at 
exactly r1->start. But, in this case, we are not going to get there
because we never send an r2 of size 0.

For any non-zero size r2 the func will always return false because
the size 0 r1 cannot encompass any range.

I could be misreading it all ;)

--Alison


> Add resource size check before using it.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/hdm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 223c273c0cd1..c58d6b8f9b58 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  	cxled->dpa_res = res;
>  	cxled->skip = skipped;
>  
> -	if (resource_contains(&cxlds->pmem_res, res))
> +	if (resource_size(&cxlds->pmem_res) &&
> +	    resource_contains(&cxlds->pmem_res, res)) {
>  		cxled->mode = CXL_DECODER_PMEM;
> -	else if (resource_contains(&cxlds->ram_res, res))
> +	} else if (resource_size(&cxlds->ram_res) &&
> +		   resource_contains(&cxlds->ram_res, res)) {
>  		cxled->mode = CXL_DECODER_RAM;
> +	}
>  	else {
>  		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
>  			 port->id, cxled->cxld.id, cxled->dpa_res);
> -- 
> 2.17.1
> 
>
Zhi Wang Nov. 21, 2024, 7:13 a.m. UTC | #6 
On Wed, 20 Nov 2024 13:45:54 +0000
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> 
> On 11/19/24 19:50, Zhi Wang wrote:
> > On Mon, 18 Nov 2024 16:44:17 +0000
> > <alejandro.lucero-palau@amd.com> wrote:
> >
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> For a resource defined with size zero, resource_contains returns
> >> always true.
> >>
> >> Add resource size check before using it.
> >>
> > Does this trigger a bug? Looks like this should be with a Fixes:
> > tag?
> 
> 
> It seems it does not until the new code for type2 support.
> 

I see. Then it is not necessary for a Fixes: tag.

> 
> > Z.
> >
> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >> ---
> >>   drivers/cxl/core/hdm.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >> index 223c273c0cd1..c58d6b8f9b58 100644
> >> --- a/drivers/cxl/core/hdm.c
> >> +++ b/drivers/cxl/core/hdm.c
> >> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct
> >> cxl_endpoint_decoder *cxled, cxled->dpa_res = res;
> >>   	cxled->skip = skipped;
> >>   
> >> -	if (resource_contains(&cxlds->pmem_res, res))
> >> +	if (resource_size(&cxlds->pmem_res) &&
> >> +	    resource_contains(&cxlds->pmem_res, res)) {
> >>   		cxled->mode = CXL_DECODER_PMEM;
> >> -	else if (resource_contains(&cxlds->ram_res, res))
> >> +	} else if (resource_size(&cxlds->ram_res) &&
> >> +		   resource_contains(&cxlds->ram_res, res)) {
> >>   		cxled->mode = CXL_DECODER_RAM;
> >> +	}
> >>   	else {
> >>   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not
> >> supported\n", port->id, cxled->cxld.id, cxled->dpa_res);
>
Alejandro Lucero Palau Nov. 21, 2024, 9:22 a.m. UTC | #7 
On 11/21/24 02:46, Alison Schofield wrote:
> On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> For a resource defined with size zero, resource_contains returns
>> always true.
>>
> I'm not following the premise above -
>
> Looking at resource_contains() and the changes made below,
> it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res
> being zero - because we already checked that the second param
> 'res' is not zero a few lines above.
>
> Looking at what happens when r1 is of size 0, I don't see how
> resource_contains() returns always true.
>
> In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end.
> The func can only return true if r2 is also of size 0 and located at
> exactly r1->start. But, in this case, we are not going to get there
> because we never send an r2 of size 0.
>
> For any non-zero size r2 the func will always return false because
> the size 0 r1 cannot encompass any range.
>
> I could be misreading it all ;)


The key is to know how a resource with size 0 is initialized, what can 
be understood looking at DEFINE_RES_NAMED macro. The end field is set 
as  size - 1.

With unsigned variables, as it is the case here, it means to have a 
resource as big as possible ... if you do not check first the size is 
not 0.

The pmem resource is explicitly initialized inside 
cxl_accel_state_create in the previous patch, so it has:

pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff

the resource checked against is defined with, for example, a 256MB size:

res.start =0, res.end = 0xfffffff


if you then use resource_contains(pmem_res, res), that implies always 
true, whatever the res range defined.


All this confused me as well when facing it initially. I hope this 
explanation makes sense.


>
> --Alison
>
>
>> Add resource size check before using it.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/hdm.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 223c273c0cd1..c58d6b8f9b58 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>>   	cxled->dpa_res = res;
>>   	cxled->skip = skipped;
>>   
>> -	if (resource_contains(&cxlds->pmem_res, res))
>> +	if (resource_size(&cxlds->pmem_res) &&
>> +	    resource_contains(&cxlds->pmem_res, res)) {
>>   		cxled->mode = CXL_DECODER_PMEM;
>> -	else if (resource_contains(&cxlds->ram_res, res))
>> +	} else if (resource_size(&cxlds->ram_res) &&
>> +		   resource_contains(&cxlds->ram_res, res)) {
>>   		cxled->mode = CXL_DECODER_RAM;
>> +	}
>>   	else {
>>   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
>>   			 port->id, cxled->cxld.id, cxled->dpa_res);
>> -- 
>> 2.17.1
>>
>>
Alison Schofield Nov. 21, 2024, 9 p.m. UTC | #8 
On Thu, Nov 21, 2024 at 09:22:33AM +0000, Alejandro Lucero Palau wrote:
> 
> On 11/21/24 02:46, Alison Schofield wrote:
> > On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote:
> > > From: Alejandro Lucero <alucerop@amd.com>
> > > 
> > > For a resource defined with size zero, resource_contains returns
> > > always true.
> > > 
> > I'm not following the premise above -
> > 
> > Looking at resource_contains() and the changes made below,
> > it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res
> > being zero - because we already checked that the second param
> > 'res' is not zero a few lines above.
> > 
> > Looking at what happens when r1 is of size 0, I don't see how
> > resource_contains() returns always true.
> > 
> > In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end.
> > The func can only return true if r2 is also of size 0 and located at
> > exactly r1->start. But, in this case, we are not going to get there
> > because we never send an r2 of size 0.
> > 
> > For any non-zero size r2 the func will always return false because
> > the size 0 r1 cannot encompass any range.
> > 
> > I could be misreading it all ;)
> 
> 
> The key is to know how a resource with size 0 is initialized, what can be
> understood looking at DEFINE_RES_NAMED macro. The end field is set as  size
> - 1.
> 
> With unsigned variables, as it is the case here, it means to have a resource
> as big as possible ... if you do not check first the size is not 0.
> 
> The pmem resource is explicitly initialized inside cxl_accel_state_create in
> the previous patch, so it has:
> 
> pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff
> 
> the resource checked against is defined with, for example, a 256MB size:
> 
> res.start =0, res.end = 0xfffffff
> 
> 
> if you then use resource_contains(pmem_res, res), that implies always true,
> whatever the res range defined.
> 
> 
> All this confused me as well when facing it initially. I hope this
> explanation makes sense.
> 

Thanks for the explanation! I'm wondering if we are leaving a trap for the next
developer.

resource_contains() seems to have intended that a check for IORESOURCE_UNSET
would take care of the zero size case:

(5edb93b89f6c resource: Add resource_contains)

and it would if folks used _UNSET. Some check r1->start before calling
resource_contains().

One option would be to use _UNSET in this case, but that only covers us here,
and doesn't remove the trap ;)

How about hardening resource_contains():

ie: make resource_contains() return false if either res empty

 /* 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)
		return false;
	return r1->start <= r2->start && r1->end >= r2->end;
}

-- Alison

> > 
> > --Alison
> > 
> > 
> > > Add resource size check before using it.
> > > 
> > > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > > ---
> > >   drivers/cxl/core/hdm.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index 223c273c0cd1..c58d6b8f9b58 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> > >   	cxled->dpa_res = res;
> > >   	cxled->skip = skipped;
> > > -	if (resource_contains(&cxlds->pmem_res, res))
> > > +	if (resource_size(&cxlds->pmem_res) &&
> > > +	    resource_contains(&cxlds->pmem_res, res)) {
> > >   		cxled->mode = CXL_DECODER_PMEM;
> > > -	else if (resource_contains(&cxlds->ram_res, res))
> > > +	} else if (resource_size(&cxlds->ram_res) &&
> > > +		   resource_contains(&cxlds->ram_res, res)) {
> > >   		cxled->mode = CXL_DECODER_RAM;
> > > +	}
> > >   	else {
> > >   		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
> > >   			 port->id, cxled->cxld.id, cxled->dpa_res);
> > > -- 
> > > 2.17.1
> > > 
> > >
diff mbox series

Patch

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 223c273c0cd1..c58d6b8f9b58 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -327,10 +327,13 @@  static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 	cxled->dpa_res = res;
 	cxled->skip = skipped;
 
-	if (resource_contains(&cxlds->pmem_res, res))
+	if (resource_size(&cxlds->pmem_res) &&
+	    resource_contains(&cxlds->pmem_res, res)) {
 		cxled->mode = CXL_DECODER_PMEM;
-	else if (resource_contains(&cxlds->ram_res, res))
+	} else if (resource_size(&cxlds->ram_res) &&
+		   resource_contains(&cxlds->ram_res, res)) {
 		cxled->mode = CXL_DECODER_RAM;
+	}
 	else {
 		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
 			 port->id, cxled->cxld.id, cxled->dpa_res);