| Message ID | 20221101074100.1732003-1-vishal.l.verma@intel.com |
|---|---|
| State | Accepted |
| Commit | 71ee71d7adcba648077997a29a91158d20c40b09 |
| Headers | show |
| Series | [v2] cxl/region: refactor decoder allocation for region refs | expand |
On 11/1/2022 12:41 AM, 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> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 67 ++++++++++++++++++++++++--------------- > 1 file changed, 41 insertions(+), 26 deletions(-) > > Changes since v1[1]: > - Limit the new decoder alloc helper to only the decoder allocation for > new cxl_region_ref objects, not retrieval for existing refs (Dan). > > [1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 401148016978..986855e93e71 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,33 @@ 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, > + struct cxl_region_ref *cxl_rr) > +{ > + struct cxl_decoder *cxld; > + > + 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 -EBUSY; > + } > + > + 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' > @@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port, > 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)) { > @@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port, > } > 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)); > + rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr); > + if (rc) > 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; > } > + cxld = cxl_rr->decoder; > > rc = cxl_rr_ep_add(cxl_rr, cxled); > if (rc) {
On Tue, 1 Nov 2022 01:41:00 -0600 Vishal Verma <vishal.l.verma@intel.com> 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> FWIW. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/region.c | 67 ++++++++++++++++++++++++--------------- > 1 file changed, 41 insertions(+), 26 deletions(-) > > Changes since v1[1]: > - Limit the new decoder alloc helper to only the decoder allocation for > new cxl_region_ref objects, not retrieval for existing refs (Dan). > > [1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 401148016978..986855e93e71 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,33 @@ 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, > + struct cxl_region_ref *cxl_rr) > +{ > + struct cxl_decoder *cxld; > + > + 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 -EBUSY; > + } > + > + 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' > @@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port, > 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)) { > @@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port, > } > 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)); > + rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr); > + if (rc) > 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; > } > + cxld = cxl_rr->decoder; > > rc = cxl_rr_ep_add(cxl_rr, cxled); > if (rc) {
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 401148016978..986855e93e71 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,33 @@ 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, + struct cxl_region_ref *cxl_rr) +{ + struct cxl_decoder *cxld; + + 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 -EBUSY; + } + + 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' @@ -794,12 +830,6 @@ static int cxl_port_attach_region(struct cxl_port *port, 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)) { @@ -810,26 +840,11 @@ static int cxl_port_attach_region(struct cxl_port *port, } 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)); + rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr); + if (rc) 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; } + 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 | 67 ++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 26 deletions(-) Changes since v1[1]: - Limit the new decoder alloc helper to only the decoder allocation for new cxl_region_ref objects, not retrieval for existing refs (Dan). [1]: https://lore.kernel.org/linux-cxl/89acba4011d03582a1f81feb376915b826020cee.camel@intel.com