| Message ID | 20221028193351.408950-1-vishal.l.verma@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: refactor decoder allocation for region refs | expand |
Vishal Verma wrote: > When an intermediate port's decoders have been exhausted by existing > regions, and creating a new region with the port in question in it's > hierarchical path is attempted, cxl_port_attach_region() fails to find a > port decoder (as would be expected), and drops into the failure / cleanup > path. > > However, during cleanup of the region reference, a sanity check attempts > to dereference the decoder, which in the above case didn't exist. This > causes a NULL pointer dereference BUG. > > To fix this, refactor the decoder allocation and de-allocation into > helper routines, and in this 'free' routine, check that the decoder, > @cxld, is valid before attempting any operations on it. > > Cc: Dan Williams <dan.j.williams@intel.com> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > drivers/cxl/core/region.c | 164 +++++++++++++++++++++++--------------- > 1 file changed, 99 insertions(+), 65 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 401148016978..78176f7ccff3 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port, > return cxl_rr; > } > > -static void free_region_ref(struct cxl_region_ref *cxl_rr) > +static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr) > { > - struct cxl_port *port = cxl_rr->port; > struct cxl_region *cxlr = cxl_rr->region; > struct cxl_decoder *cxld = cxl_rr->decoder; > > + if (!cxld) > + return; > + > dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n"); > if (cxld->region == cxlr) { > cxld->region = NULL; > put_device(&cxlr->dev); > } > +} > > +static void free_region_ref(struct cxl_region_ref *cxl_rr) > +{ > + struct cxl_port *port = cxl_rr->port; > + struct cxl_region *cxlr = cxl_rr->region; > + > + cxl_rr_free_decoder(cxl_rr); > xa_erase(&port->regions, (unsigned long)cxlr); > xa_destroy(&cxl_rr->endpoints); > kfree(cxl_rr); > @@ -728,6 +737,83 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr, > return 0; > } > > +static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, > + bool *nr_targets_inc) > +{ > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_ep *ep = cxl_ep_load(port, cxlmd); > + struct cxl_region_ref *cxl_rr; > + struct cxl_decoder *cxld; > + unsigned long index; > + > + cxl_rr = cxl_rr_load(port, cxlr); > + if (cxl_rr) { > + struct cxl_ep *ep_iter; > + int found = 0; > + > + /* > + * Walk the existing endpoints that have been attached to > + * @cxlr at @port and see if they share the same 'next' port > + * in the downstream direction. I.e. endpoints that share common > + * upstream switch. > + */ > + xa_for_each(&cxl_rr->endpoints, index, ep_iter) { > + if (ep_iter == ep) > + continue; > + if (ep_iter->next == ep->next) { > + found++; > + break; > + } > + } > + > + /* > + * New target port, or @port is an endpoint port that always > + * accounts its own local decode as a target. > + */ > + if (!found || !ep->next) { > + cxl_rr->nr_targets++; > + *nr_targets_inc = true; > + } > + > + /* > + * The decoder for @cxlr was allocated when the region was first > + * attached to @port. > + */ > + cxld = cxl_rr->decoder; > + } else { > + cxl_rr = alloc_region_ref(port, cxlr); The region_ref, 'cxl_rr', is being created here, but the function is called cxl_rr_alloc_decoder(), which to me means the 'cxl_rr' should be passed in / already created. > + if (IS_ERR(cxl_rr)) { > + dev_dbg(&cxlr->dev, > + "%s: failed to allocate region reference\n", > + dev_name(&port->dev)); > + return PTR_ERR(cxl_rr); > + } > + *nr_targets_inc = true; > + > + if (port == cxled_to_port(cxled)) > + cxld = &cxled->cxld; > + else > + cxld = cxl_region_find_decoder(port, cxlr); > + if (!cxld) { > + dev_dbg(&cxlr->dev, "%s: no decoder available\n", > + dev_name(&port->dev)); > + return -ENXIO; > + } > + > + if (cxld->region) { > + dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n", > + dev_name(&port->dev), dev_name(&cxld->dev), > + dev_name(&cxld->region->dev)); > + return -EBUSY; > + } > + > + cxl_rr->decoder = cxld; I was thinking cxl_rr_alloc_decoder() to just be this above bit of taking an existing cxl_rr and appending the decoder. The flow would then go: alloc_region_ref() \->cxl_rr_alloc_decoder() free_region_ref() \->cxl_rr_free_decoder() ...so the symmetry is more apparent.
On Fri, 2022-10-28 at 13:42 -0700, Dan Williams wrote: > Vishal Verma wrote: > > When an intermediate port's decoders have been exhausted by existing > > regions, and creating a new region with the port in question in it's > > hierarchical path is attempted, cxl_port_attach_region() fails to find a > > port decoder (as would be expected), and drops into the failure / cleanup > > path. > > > > However, during cleanup of the region reference, a sanity check attempts > > to dereference the decoder, which in the above case didn't exist. This > > causes a NULL pointer dereference BUG. > > > > To fix this, refactor the decoder allocation and de-allocation into > > helper routines, and in this 'free' routine, check that the decoder, > > @cxld, is valid before attempting any operations on it. > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > drivers/cxl/core/region.c | 164 +++++++++++++++++++++++--------------- > > 1 file changed, 99 insertions(+), 65 deletions(-) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index 401148016978..78176f7ccff3 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port, > > return cxl_rr; > > } > > > > -static void free_region_ref(struct cxl_region_ref *cxl_rr) > > +static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr) > > { > > - struct cxl_port *port = cxl_rr->port; > > struct cxl_region *cxlr = cxl_rr->region; > > struct cxl_decoder *cxld = cxl_rr->decoder; > > > > + if (!cxld) > > + return; > > + > > dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n"); > > if (cxld->region == cxlr) { > > cxld->region = NULL; > > put_device(&cxlr->dev); > > } > > +} > > > > +static void free_region_ref(struct cxl_region_ref *cxl_rr) > > +{ > > + struct cxl_port *port = cxl_rr->port; > > + struct cxl_region *cxlr = cxl_rr->region; > > + > > + cxl_rr_free_decoder(cxl_rr); > > xa_erase(&port->regions, (unsigned long)cxlr); > > xa_destroy(&cxl_rr->endpoints); > > kfree(cxl_rr); > > @@ -728,6 +737,83 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr, > > return 0; > > } > > > > +static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr, > > + struct cxl_endpoint_decoder *cxled, > > + bool *nr_targets_inc) > > +{ > > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > > + struct cxl_ep *ep = cxl_ep_load(port, cxlmd); > > + struct cxl_region_ref *cxl_rr; > > + struct cxl_decoder *cxld; > > + unsigned long index; > > + > > + cxl_rr = cxl_rr_load(port, cxlr); > > + if (cxl_rr) { > > + struct cxl_ep *ep_iter; > > + int found = 0; > > + > > + /* > > + * Walk the existing endpoints that have been attached to > > + * @cxlr at @port and see if they share the same 'next' port > > + * in the downstream direction. I.e. endpoints that share common > > + * upstream switch. > > + */ > > + xa_for_each(&cxl_rr->endpoints, index, ep_iter) { > > + if (ep_iter == ep) > > + continue; > > + if (ep_iter->next == ep->next) { > > + found++; > > + break; > > + } > > + } > > + > > + /* > > + * New target port, or @port is an endpoint port that always > > + * accounts its own local decode as a target. > > + */ > > + if (!found || !ep->next) { > > + cxl_rr->nr_targets++; > > + *nr_targets_inc = true; > > + } > > + > > + /* > > + * The decoder for @cxlr was allocated when the region was first > > + * attached to @port. > > + */ > > + cxld = cxl_rr->decoder; > > + } else { > > + cxl_rr = alloc_region_ref(port, cxlr); > > The region_ref, 'cxl_rr', is being created here, but the function is > called cxl_rr_alloc_decoder(), which to me means the 'cxl_rr' should be > passed in / already created. Ah yeah makes sense. > > > + if (IS_ERR(cxl_rr)) { > > + dev_dbg(&cxlr->dev, > > + "%s: failed to allocate region reference\n", > > + dev_name(&port->dev)); > > + return PTR_ERR(cxl_rr); > > + } > > + *nr_targets_inc = true; > > + > > + if (port == cxled_to_port(cxled)) > > + cxld = &cxled->cxld; > > + else > > + cxld = cxl_region_find_decoder(port, cxlr); > > + if (!cxld) { > > + dev_dbg(&cxlr->dev, "%s: no decoder available\n", > > + dev_name(&port->dev)); > > + return -ENXIO; > > + } > > + > > + if (cxld->region) { > > + dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n", > > + dev_name(&port->dev), dev_name(&cxld->dev), > > + dev_name(&cxld->region->dev)); > > + return -EBUSY; > > + } > > + > > + cxl_rr->decoder = cxld; > > I was thinking cxl_rr_alloc_decoder() to just be this above bit of taking an > existing cxl_rr and appending the decoder. The flow would then go: > > alloc_region_ref() > \->cxl_rr_alloc_decoder() > free_region_ref() > \->cxl_rr_free_decoder() > > ...so the symmetry is more apparent. Yep sounds reasonable, I'll send a v2 with this.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 401148016978..78176f7ccff3 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -686,18 +686,27 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port, return cxl_rr; } -static void free_region_ref(struct cxl_region_ref *cxl_rr) +static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr) { - struct cxl_port *port = cxl_rr->port; struct cxl_region *cxlr = cxl_rr->region; struct cxl_decoder *cxld = cxl_rr->decoder; + if (!cxld) + return; + dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n"); if (cxld->region == cxlr) { cxld->region = NULL; put_device(&cxlr->dev); } +} +static void free_region_ref(struct cxl_region_ref *cxl_rr) +{ + struct cxl_port *port = cxl_rr->port; + struct cxl_region *cxlr = cxl_rr->region; + + cxl_rr_free_decoder(cxl_rr); xa_erase(&port->regions, (unsigned long)cxlr); xa_destroy(&cxl_rr->endpoints); kfree(cxl_rr); @@ -728,6 +737,83 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr, return 0; } +static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr, + struct cxl_endpoint_decoder *cxled, + bool *nr_targets_inc) +{ + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct cxl_ep *ep = cxl_ep_load(port, cxlmd); + struct cxl_region_ref *cxl_rr; + struct cxl_decoder *cxld; + unsigned long index; + + cxl_rr = cxl_rr_load(port, cxlr); + if (cxl_rr) { + struct cxl_ep *ep_iter; + int found = 0; + + /* + * Walk the existing endpoints that have been attached to + * @cxlr at @port and see if they share the same 'next' port + * in the downstream direction. I.e. endpoints that share common + * upstream switch. + */ + xa_for_each(&cxl_rr->endpoints, index, ep_iter) { + if (ep_iter == ep) + continue; + if (ep_iter->next == ep->next) { + found++; + break; + } + } + + /* + * New target port, or @port is an endpoint port that always + * accounts its own local decode as a target. + */ + if (!found || !ep->next) { + cxl_rr->nr_targets++; + *nr_targets_inc = true; + } + + /* + * The decoder for @cxlr was allocated when the region was first + * attached to @port. + */ + cxld = cxl_rr->decoder; + } else { + cxl_rr = alloc_region_ref(port, cxlr); + if (IS_ERR(cxl_rr)) { + dev_dbg(&cxlr->dev, + "%s: failed to allocate region reference\n", + dev_name(&port->dev)); + return PTR_ERR(cxl_rr); + } + *nr_targets_inc = true; + + if (port == cxled_to_port(cxled)) + cxld = &cxled->cxld; + else + cxld = cxl_region_find_decoder(port, cxlr); + if (!cxld) { + dev_dbg(&cxlr->dev, "%s: no decoder available\n", + dev_name(&port->dev)); + return -ENXIO; + } + + if (cxld->region) { + dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n", + dev_name(&port->dev), dev_name(&cxld->dev), + dev_name(&cxld->region->dev)); + return -EBUSY; + } + + cxl_rr->decoder = cxld; + } + + return 0; +} + /** * cxl_port_attach_region() - track a region's interest in a port by endpoint * @port: port to add a new region reference 'struct cxl_region_ref' @@ -761,75 +847,23 @@ static int cxl_port_attach_region(struct cxl_port *port, struct cxl_region_ref *cxl_rr; bool nr_targets_inc = false; struct cxl_decoder *cxld; - unsigned long index; int rc = -EBUSY; lockdep_assert_held_write(&cxl_region_rwsem); + rc = cxl_rr_alloc_decoder(port, cxlr, cxled, &nr_targets_inc); + if (rc) + goto out_erase; + cxl_rr = cxl_rr_load(port, cxlr); - if (cxl_rr) { - struct cxl_ep *ep_iter; - int found = 0; - - /* - * Walk the existing endpoints that have been attached to - * @cxlr at @port and see if they share the same 'next' port - * in the downstream direction. I.e. endpoints that share common - * upstream switch. - */ - xa_for_each(&cxl_rr->endpoints, index, ep_iter) { - if (ep_iter == ep) - continue; - if (ep_iter->next == ep->next) { - found++; - break; - } - } - - /* - * New target port, or @port is an endpoint port that always - * accounts its own local decode as a target. - */ - if (!found || !ep->next) { - cxl_rr->nr_targets++; - nr_targets_inc = true; - } - - /* - * The decoder for @cxlr was allocated when the region was first - * attached to @port. - */ - cxld = cxl_rr->decoder; - } else { - cxl_rr = alloc_region_ref(port, cxlr); - if (IS_ERR(cxl_rr)) { - dev_dbg(&cxlr->dev, - "%s: failed to allocate region reference\n", - dev_name(&port->dev)); - return PTR_ERR(cxl_rr); - } - nr_targets_inc = true; - - if (port == cxled_to_port(cxled)) - cxld = &cxled->cxld; - else - cxld = cxl_region_find_decoder(port, cxlr); - if (!cxld) { - dev_dbg(&cxlr->dev, "%s: no decoder available\n", - dev_name(&port->dev)); - goto out_erase; - } - - if (cxld->region) { - dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n", - dev_name(&port->dev), dev_name(&cxld->dev), - dev_name(&cxld->region->dev)); - rc = -EBUSY; - goto out_erase; - } - - cxl_rr->decoder = cxld; + if (!cxl_rr) { + dev_dbg(&cxlr->dev, + "%s: expected to find a region reference but failed\n", + dev_name(&port->dev)); + rc = -ENXIO; + goto out_erase; } + cxld = cxl_rr->decoder; rc = cxl_rr_ep_add(cxl_rr, cxled); if (rc) {
When an intermediate port's decoders have been exhausted by existing regions, and creating a new region with the port in question in it's hierarchical path is attempted, cxl_port_attach_region() fails to find a port decoder (as would be expected), and drops into the failure / cleanup path. However, during cleanup of the region reference, a sanity check attempts to dereference the decoder, which in the above case didn't exist. This causes a NULL pointer dereference BUG. To fix this, refactor the decoder allocation and de-allocation into helper routines, and in this 'free' routine, check that the decoder, @cxld, is valid before attempting any operations on it. Cc: Dan Williams <dan.j.williams@intel.com> Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/cxl/core/region.c | 164 +++++++++++++++++++++++--------------- 1 file changed, 99 insertions(+), 65 deletions(-)