* [PATCH v2] cxl/region: refactor decoder allocation for region refs
@ 2022-11-01 7:41 Vishal Verma
2022-11-01 18:16 ` Dave Jiang
2022-11-03 16:41 ` Jonathan Cameron
0 siblings, 2 replies; 3+ messages in thread
From: Vishal Verma @ 2022-11-01 7:41 UTC (permalink / raw)
To: linux-cxl
Cc: Dan Williams, Jonathan Cameron, Ira Weiny, Alison Schofield,
Vishal Verma
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
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) {
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] cxl/region: refactor decoder allocation for region refs
2022-11-01 7:41 [PATCH v2] cxl/region: refactor decoder allocation for region refs Vishal Verma
@ 2022-11-01 18:16 ` Dave Jiang
2022-11-03 16:41 ` Jonathan Cameron
1 sibling, 0 replies; 3+ messages in thread
From: Dave Jiang @ 2022-11-01 18:16 UTC (permalink / raw)
To: Vishal Verma, linux-cxl
Cc: Dan Williams, Jonathan Cameron, Ira Weiny, Alison Schofield
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) {
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] cxl/region: refactor decoder allocation for region refs
2022-11-01 7:41 [PATCH v2] cxl/region: refactor decoder allocation for region refs Vishal Verma
2022-11-01 18:16 ` Dave Jiang
@ 2022-11-03 16:41 ` Jonathan Cameron
1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2022-11-03 16:41 UTC (permalink / raw)
To: Vishal Verma; +Cc: linux-cxl, Dan Williams, Ira Weiny, Alison Schofield
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) {
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-03 16:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-01 7:41 [PATCH v2] cxl/region: refactor decoder allocation for region refs Vishal Verma
2022-11-01 18:16 ` Dave Jiang
2022-11-03 16:41 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox