* [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()
@ 2022-08-01 10:19 Dan Carpenter
2022-08-01 10:20 ` [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl() Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dan Carpenter @ 2022-08-01 10:19 UTC (permalink / raw)
To: Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl()
2022-08-01 10:19 [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Carpenter
@ 2022-08-01 10:20 ` Dan Carpenter
2022-08-01 19:09 ` Dan Williams
2022-08-01 10:20 ` [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach() Dan Carpenter
2022-08-01 19:07 ` [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Williams
2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2022-08-01 10:20 UTC (permalink / raw)
To: Alison Schofield, Ben Widawsky
Cc: Vishal Verma, Ira Weiny, Dan Williams, Jonathan Cameron,
linux-cxl, kernel-janitors
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()
2022-08-01 10:19 [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Carpenter
2022-08-01 10:20 ` [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl() Dan Carpenter
@ 2022-08-01 10:20 ` Dan Carpenter
2022-08-01 10:56 ` Dan Carpenter
2022-08-01 19:11 ` Dan Williams
2022-08-01 19:07 ` [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Williams
2 siblings, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2022-08-01 10:20 UTC (permalink / raw)
To: Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()
2022-08-01 10:20 ` [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach() Dan Carpenter
@ 2022-08-01 10:56 ` Dan Carpenter
2022-08-01 19:49 ` Dan Williams
2022-08-01 19:11 ` Dan Williams
1 sibling, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2022-08-01 10:56 UTC (permalink / raw)
To: Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()
2022-08-01 10:19 [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Carpenter
2022-08-01 10:20 ` [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl() Dan Carpenter
2022-08-01 10:20 ` [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach() Dan Carpenter
@ 2022-08-01 19:07 ` Dan Williams
2022-08-02 6:43 ` Dan Carpenter
2 siblings, 1 reply; 9+ messages in thread
From: Dan Williams @ 2022-08-01 19:07 UTC (permalink / raw)
To: Dan Carpenter, Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl()
2022-08-01 10:20 ` [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl() Dan Carpenter
@ 2022-08-01 19:09 ` Dan Williams
0 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2022-08-01 19:09 UTC (permalink / raw)
To: Dan Carpenter, Alison Schofield, Ben Widawsky
Cc: Vishal Verma, Ira Weiny, Dan Williams, Jonathan Cameron,
linux-cxl, kernel-janitors
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()
2022-08-01 10:20 ` [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach() Dan Carpenter
2022-08-01 10:56 ` Dan Carpenter
@ 2022-08-01 19:11 ` Dan Williams
1 sibling, 0 replies; 9+ messages in thread
From: Dan Williams @ 2022-08-01 19:11 UTC (permalink / raw)
To: Dan Carpenter, Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()
2022-08-01 10:56 ` Dan Carpenter
@ 2022-08-01 19:49 ` Dan Williams
0 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2022-08-01 19:49 UTC (permalink / raw)
To: Dan Carpenter, Alison Schofield, Dan Williams
Cc: Vishal Verma, Ira Weiny, Ben Widawsky, Jonathan Cameron,
linux-cxl, kernel-janitors
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa()
2022-08-01 19:07 ` [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Williams
@ 2022-08-02 6:43 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2022-08-02 6:43 UTC (permalink / raw)
To: Dan Williams
Cc: Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky,
Jonathan Cameron, linux-cxl, kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-02 6:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 10:19 [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Carpenter
2022-08-01 10:20 ` [PATCH 2/3] cxl/region: prevent underflow in ways_to_cxl() Dan Carpenter
2022-08-01 19:09 ` Dan Williams
2022-08-01 10:20 ` [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach() Dan Carpenter
2022-08-01 10:56 ` Dan Carpenter
2022-08-01 19:49 ` Dan Williams
2022-08-01 19:11 ` Dan Williams
2022-08-01 19:07 ` [PATCH 1/3] cxl/region: uninitialized variable in alloc_hpa() Dan Williams
2022-08-02 6:43 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.