[PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()

[PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()

[Dan Carpenter]

This should check "p->res" instead of "res" (which is uninitialized).

Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/cxl/core/region.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index dc71ec457608..c80932bca667 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -454,7 +454,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
 	lockdep_assert_held_write(&cxl_region_rwsem);
 
 	/* Nothing to do... */
-	if (p->res && resource_size(res) == size)
+	if (p->res && resource_size(p->res) == size)
 		return 0;
 
 	/* To change size the old size must be freed first */
-- 
2.35.1

[PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl()

[Dan Carpenter]

The "ways" variable comes from the user.  The ways_to_cxl() function
has an upper bound but it doesn't check for negatives.  Make
the "ways" variable an unsigned int to fix this bug.

Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/cxl/cxl.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 75674400cc8d..969953ce2609 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -102,7 +102,7 @@ static inline int granularity_to_cxl(int g, u16 *ig)
 	return 0;
 }
 
-static inline int ways_to_cxl(int ways, u8 *iw)
+static inline int ways_to_cxl(unsigned int ways, u8 *iw)
 {
 	if (ways > 16)
 		return -EINVAL;
-- 
2.35.1

RE: [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl()

[Dan Williams]

Dan Carpenter wrote:
> The "ways" variable comes from the user.  The ways_to_cxl() function
> has an upper bound but it doesn't check for negatives.  Make
> the "ways" variable an unsigned int to fix this bug.
> 
> Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/cxl/cxl.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 75674400cc8d..969953ce2609 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -102,7 +102,7 @@ static inline int granularity_to_cxl(int g, u16 *ig)
>  	return 0;
>  }
>  
> -static inline int ways_to_cxl(int ways, u8 *iw)
> +static inline int ways_to_cxl(unsigned int ways, u8 *iw)
>  {
>  	if (ways > 16)
>  		return -EINVAL;

Looks good, I'll go ahead and update interleave_ways_store() to also not
allow negative values.

[PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()

[Dan Carpenter]

The ++ needs a match -- on the clean up path.  If the p->nr_targets
value gets to be more than 16 it leads to uninitialized data in
cxl_port_setup_targets().

drivers/cxl/core/region.c:995 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.

Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/cxl/core/region.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c80932bca667..0450354bff4d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1217,12 +1217,14 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	if (p->nr_targets == p->interleave_ways) {
 		rc = cxl_region_setup_targets(cxlr);
 		if (rc)
-			goto err;
+			goto err_decrement;
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
 	return 0;
 
+err_decrement:
+	p->nr_targets--;
 err:
 	for (iter = ep_port; !is_cxl_root(iter);
 	     iter = to_cxl_port(iter->dev.parent))
-- 
2.35.1

Re: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()

[Dan Carpenter]

On Mon, Aug 01, 2022 at 01:20:58PM +0300, Dan Carpenter wrote:
> The ++ needs a match -- on the clean up path.  If the p->nr_targets
> value gets to be more than 16 it leads to uninitialized data in
> cxl_port_setup_targets().
> 
> drivers/cxl/core/region.c:995 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

The error handling in cxl_port_attach_region() looks like it might have
a similar bug.  The cxl_rr->nr_targets++; might want a --.

That function is more complicated.

drivers/cxl/core/region.c
   740  static int cxl_port_attach_region(struct cxl_port *port,
   741                                    struct cxl_region *cxlr,
   742                                    struct cxl_endpoint_decoder *cxled, int pos)
   743  {
   744          struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
   745          struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
   746          struct cxl_region_ref *cxl_rr = NULL, *iter;
   747          struct cxl_region_params *p = &cxlr->params;
   748          struct cxl_decoder *cxld = NULL;
   749          unsigned long index;
   750          int rc = -EBUSY;
   751  
   752          lockdep_assert_held_write(&cxl_region_rwsem);
   753  
   754          xa_for_each(&port->regions, index, iter) {
   755                  struct cxl_region_params *ip = &iter->region->params;
   756  
   757                  if (iter->region == cxlr)
   758                          cxl_rr = iter;

Should there be a break statement after this assignment

   759                  if (ip->res->start > p->res->start) {

or do we really want to test every ip->res->start?  This loop is
confusing...

   760                          dev_dbg(&cxlr->dev,
   761                                  "%s: HPA order violation %s:%pr vs %pr\n",
   762                                  dev_name(&port->dev),
   763                                  dev_name(&iter->region->dev), ip->res, p->res);
   764                          return -EBUSY;
   765                  }
   766          }
   767  
   768          if (cxl_rr) {
   769                  struct cxl_ep *ep_iter;
   770                  int found = 0;
   771  
   772                  cxld = cxl_rr->decoder;
   773                  xa_for_each(&cxl_rr->endpoints, index, ep_iter) {

regards,
dan carpenter

Re: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()

[Dan Williams]

Dan Carpenter wrote:
> On Mon, Aug 01, 2022 at 01:20:58PM +0300, Dan Carpenter wrote:
> > The ++ needs a match -- on the clean up path.  If the p->nr_targets
> > value gets to be more than 16 it leads to uninitialized data in
> > cxl_port_setup_targets().
> > 
> > drivers/cxl/core/region.c:995 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> > 
> > Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> The error handling in cxl_port_attach_region() looks like it might have
> a similar bug.  The cxl_rr->nr_targets++; might want a --.
> 
> That function is more complicated.
> 
> drivers/cxl/core/region.c
>    740  static int cxl_port_attach_region(struct cxl_port *port,
>    741                                    struct cxl_region *cxlr,
>    742                                    struct cxl_endpoint_decoder *cxled, int pos)
>    743  {
>    744          struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>    745          struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
>    746          struct cxl_region_ref *cxl_rr = NULL, *iter;
>    747          struct cxl_region_params *p = &cxlr->params;
>    748          struct cxl_decoder *cxld = NULL;
>    749          unsigned long index;
>    750          int rc = -EBUSY;
>    751  
>    752          lockdep_assert_held_write(&cxl_region_rwsem);
>    753  
>    754          xa_for_each(&port->regions, index, iter) {
>    755                  struct cxl_region_params *ip = &iter->region->params;
>    756  
>    757                  if (iter->region == cxlr)
>    758                          cxl_rr = iter;
> 
> Should there be a break statement after this assignment

Indeed. If the port already has this region attached it means that it
already passed this check previously.

> 
>    759                  if (ip->res->start > p->res->start) {
> 
> or do we really want to test every ip->res->start?  This loop is
> confusing...

Let me take a shot at reflowing this whole routine to make it less
confusing.

RE: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()

[Dan Williams]

Dan Carpenter wrote:
> The ++ needs a match -- on the clean up path.  If the p->nr_targets
> value gets to be more than 16 it leads to uninitialized data in
> cxl_port_setup_targets().
> 
> drivers/cxl/core/region.c:995 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/cxl/core/region.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c80932bca667..0450354bff4d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1217,12 +1217,14 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	if (p->nr_targets == p->interleave_ways) {
>  		rc = cxl_region_setup_targets(cxlr);
>  		if (rc)
> -			goto err;
> +			goto err_decrement;
>  		p->state = CXL_CONFIG_ACTIVE;
>  	}
>  
>  	return 0;
>  
> +err_decrement:
> +	p->nr_targets--;

Yes, looks good.

RE: [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()

[Dan Williams]

Dan Carpenter wrote:
> This should check "p->res" instead of "res" (which is uninitialized).
> 
> Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/cxl/core/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index dc71ec457608..c80932bca667 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -454,7 +454,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
>  	lockdep_assert_held_write(&cxl_region_rwsem);
>  
>  	/* Nothing to do... */
> -	if (p->res && resource_size(res) == size)
> +	if (p->res && resource_size(p->res) == size)

Yup, looks good. Surprised this was not caught by my local compiler, or
any of the compile-test robots.

Re: [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()

[Dan Carpenter]

On Mon, Aug 01, 2022 at 12:07:14PM -0700, Dan Williams wrote:
> Dan Carpenter wrote:
> > This should check "p->res" instead of "res" (which is uninitialized).
> > 
> > Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/cxl/core/region.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index dc71ec457608..c80932bca667 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -454,7 +454,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
> >  	lockdep_assert_held_write(&cxl_region_rwsem);
> >  
> >  	/* Nothing to do... */
> > -	if (p->res && resource_size(res) == size)
> > +	if (p->res && resource_size(p->res) == size)
> 
> Yup, looks good. Surprised this was not caught by my local compiler, or
> any of the compile-test robots.

Yeah.  It's weird.

We've disabled GCC uninitialized warnings which made me introduce a bug
last week...  But normally the Clang people and the kbuild bots fix
the bugs before I do.

regards,
dan carpenter