* [PATCH] cxl/region: refactor decoder allocation for region refs
@ 2022-10-28 19:33 Vishal Verma
2022-10-28 20:42 ` Dan Williams
0 siblings, 1 reply; 3+ messages in thread
From: Vishal Verma @ 2022-10-28 19:33 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 | 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);
+ 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) {
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* RE: [PATCH] cxl/region: refactor decoder allocation for region refs
2022-10-28 19:33 [PATCH] cxl/region: refactor decoder allocation for region refs Vishal Verma
@ 2022-10-28 20:42 ` Dan Williams
2022-10-28 21:10 ` Verma, Vishal L
0 siblings, 1 reply; 3+ messages in thread
From: Dan Williams @ 2022-10-28 20:42 UTC (permalink / raw)
To: Vishal Verma, linux-cxl
Cc: Dan Williams, Jonathan Cameron, Ira Weiny, Alison Schofield,
Vishal Verma
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] cxl/region: refactor decoder allocation for region refs
2022-10-28 20:42 ` Dan Williams
@ 2022-10-28 21:10 ` Verma, Vishal L
0 siblings, 0 replies; 3+ messages in thread
From: Verma, Vishal L @ 2022-10-28 21:10 UTC (permalink / raw)
To: Williams, Dan J, linux-cxl@vger.kernel.org
Cc: Schofield, Alison, Jonathan.Cameron@huawei.com, Weiny, Ira
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-28 21:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-28 19:33 [PATCH] cxl/region: refactor decoder allocation for region refs Vishal Verma
2022-10-28 20:42 ` Dan Williams
2022-10-28 21:10 ` Verma, Vishal L
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox