| Message ID | YuepCvUAoCtdpcoO@kili |
|---|---|
| State | Accepted |
| Commit | 5e42bcbc3fef6e759dfb4d3f4cfb394c382b4249 |
| Headers | show |
| Series | [1/3] cxl/region: uninitialized variable in alloc_hpa() | expand |
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
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.
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.
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))
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(-)