* [PATCH] cxl/region: Fix region reference target accounting
@ 2022-08-01 23:00 Dan Williams
2022-08-04 20:36 ` Ira Weiny
0 siblings, 1 reply; 3+ messages in thread
From: Dan Williams @ 2022-08-01 23:00 UTC (permalink / raw)
To: linux-cxl; +Cc: Dan Carpenter, vishal.l.verma, ira.weiny, alison.schofield
Dan reports:
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.
Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
flow is not clear. Fix the bug and the clarity by separating the 'new'
region-reference case from the 'extend' region-reference case. This also
moves the host-physical-address (HPA) validation, that the HPA of a new
region being accounted to the port is greater than the HPA of all other
regions associated with the port, to alloc_region_ref().
Introduce @nr_targets_inc to track when the error exit path needs to
clean up cxl_rr->nr_targets.
Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 28 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index aff4f736b63c..cf5d5811fe4c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
struct cxl_region *cxlr)
{
- struct cxl_region_ref *cxl_rr;
+ struct cxl_region_params *p = &cxlr->params;
+ struct cxl_region_ref *cxl_rr, *iter;
+ unsigned long index;
int rc;
+ xa_for_each(&port->regions, index, iter) {
+ struct cxl_region_params *ip = &iter->region->params;
+
+ if (ip->res->start > p->res->start) {
+ dev_dbg(&cxlr->dev,
+ "%s: HPA order violation %s:%pr vs %pr\n",
+ dev_name(&port->dev),
+ dev_name(&iter->region->dev), ip->res, p->res);
+ return ERR_PTR(-EBUSY);
+ }
+ }
+
cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
if (!cxl_rr)
- return NULL;
+ return ERR_PTR(-ENOMEM);
cxl_rr->port = port;
cxl_rr->region = cxlr;
cxl_rr->nr_targets = 1;
@@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
"%s: failed to track region reference: %d\n",
dev_name(&port->dev), rc);
kfree(cxl_rr);
- return NULL;
+ return ERR_PTR(rc);
}
return cxl_rr;
@@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
{
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
- struct cxl_region_ref *cxl_rr = NULL, *iter;
- struct cxl_region_params *p = &cxlr->params;
- struct cxl_decoder *cxld = NULL;
+ 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);
- xa_for_each(&port->regions, index, iter) {
- struct cxl_region_params *ip = &iter->region->params;
-
- if (iter->region == cxlr)
- cxl_rr = iter;
- if (ip->res->start > p->res->start) {
- dev_dbg(&cxlr->dev,
- "%s: HPA order violation %s:%pr vs %pr\n",
- dev_name(&port->dev),
- dev_name(&iter->region->dev), ip->res, p->res);
- return -EBUSY;
- }
- }
-
+ cxl_rr = cxl_rr_load(port, cxlr);
if (cxl_rr) {
struct cxl_ep *ep_iter;
int found = 0;
- cxld = cxl_rr->decoder;
+ /*
+ * 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;
@@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
}
/*
- * If this is a new target or if this port is direct connected
- * to this endpoint then add to the target count.
+ * New target port, or @port is an endpoint port that always
+ * accounts its own local decode as a target.
*/
- if (!found || !ep->next)
+ 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 (!cxl_rr) {
+ if (IS_ERR(cxl_rr)) {
dev_dbg(&cxlr->dev,
"%s: failed to allocate region reference\n",
dev_name(&port->dev));
- return -ENOMEM;
+ return PTR_ERR(cxl_rr);
}
- }
+ nr_targets_inc = true;
- if (!cxld) {
if (port == cxled_to_port(cxled))
cxld = &cxled->cxld;
else
@@ -896,6 +909,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
return 0;
out_erase:
+ if (nr_targets_inc)
+ cxl_rr->nr_targets--;
if (cxl_rr->nr_eps == 0)
free_region_ref(cxl_rr);
return rc;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] cxl/region: Fix region reference target accounting
2022-08-01 23:00 [PATCH] cxl/region: Fix region reference target accounting Dan Williams
@ 2022-08-04 20:36 ` Ira Weiny
2022-08-04 20:58 ` Dan Williams
0 siblings, 1 reply; 3+ messages in thread
From: Ira Weiny @ 2022-08-04 20:36 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl, Dan Carpenter, vishal.l.verma, alison.schofield
On Mon, Aug 01, 2022 at 04:00:21PM -0700, Dan Williams wrote:
> Dan reports:
>
> 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.
>
> Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
> flow is not clear. Fix the bug and the clarity by separating the 'new'
^^^^^^^
clarify?
> region-reference case from the 'extend' region-reference case. This also
> moves the host-physical-address (HPA) validation, that the HPA of a new
> region being accounted to the port is greater than the HPA of all other
> regions associated with the port, to alloc_region_ref().
Technically this means the kdoc 'steps list' is out of order a bit. :-(
But I like that this is only done on creation. And I'm not sure it matter much
because 'steps' is more of a list of things this function does.
>
> Introduce @nr_targets_inc to track when the error exit path needs to
> clean up cxl_rr->nr_targets.
>
> Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++++------------------
> 1 file changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index aff4f736b63c..cf5d5811fe4c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> struct cxl_region *cxlr)
> {
> - struct cxl_region_ref *cxl_rr;
> + struct cxl_region_params *p = &cxlr->params;
> + struct cxl_region_ref *cxl_rr, *iter;
> + unsigned long index;
> int rc;
>
> + xa_for_each(&port->regions, index, iter) {
> + struct cxl_region_params *ip = &iter->region->params;
> +
> + if (ip->res->start > p->res->start) {
> + dev_dbg(&cxlr->dev,
> + "%s: HPA order violation %s:%pr vs %pr\n",
> + dev_name(&port->dev),
> + dev_name(&iter->region->dev), ip->res, p->res);
> + return ERR_PTR(-EBUSY);
> + }
> + }
> +
> cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
> if (!cxl_rr)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
> cxl_rr->port = port;
> cxl_rr->region = cxlr;
> cxl_rr->nr_targets = 1;
> @@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> "%s: failed to track region reference: %d\n",
> dev_name(&port->dev), rc);
> kfree(cxl_rr);
> - return NULL;
> + return ERR_PTR(rc);
> }
>
> return cxl_rr;
> @@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
> {
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> - struct cxl_region_ref *cxl_rr = NULL, *iter;
> - struct cxl_region_params *p = &cxlr->params;
> - struct cxl_decoder *cxld = NULL;
> + 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);
>
> - xa_for_each(&port->regions, index, iter) {
> - struct cxl_region_params *ip = &iter->region->params;
> -
> - if (iter->region == cxlr)
> - cxl_rr = iter;
> - if (ip->res->start > p->res->start) {
> - dev_dbg(&cxlr->dev,
> - "%s: HPA order violation %s:%pr vs %pr\n",
> - dev_name(&port->dev),
> - dev_name(&iter->region->dev), ip->res, p->res);
> - return -EBUSY;
> - }
> - }
> -
> + cxl_rr = cxl_rr_load(port, cxlr);
> if (cxl_rr) {
> struct cxl_ep *ep_iter;
> int found = 0;
>
> - cxld = cxl_rr->decoder;
> + /*
> + * 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;
> @@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
> }
>
> /*
> - * If this is a new target or if this port is direct connected
> - * to this endpoint then add to the target count.
> + * New target port, or @port is an endpoint port that always
> + * accounts its own local decode as a target.
> */
> - if (!found || !ep->next)
> + 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 (!cxl_rr) {
> + if (IS_ERR(cxl_rr)) {
> dev_dbg(&cxlr->dev,
> "%s: failed to allocate region reference\n",
> dev_name(&port->dev));
> - return -ENOMEM;
> + return PTR_ERR(cxl_rr);
> }
> - }
> + nr_targets_inc = true;
I don't think this is technically needed but I would leave it if I were you.
I think the leak only occurs on the extend case but I think it is safe to do
this and does make the code cleaner.
Regardless of the kdoc subtlety and with the spelling fix.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
> - if (!cxld) {
> if (port == cxled_to_port(cxled))
> cxld = &cxled->cxld;
> else
> @@ -896,6 +909,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>
> return 0;
> out_erase:
> + if (nr_targets_inc)
> + cxl_rr->nr_targets--;
> if (cxl_rr->nr_eps == 0)
> free_region_ref(cxl_rr);
> return rc;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] cxl/region: Fix region reference target accounting
2022-08-04 20:36 ` Ira Weiny
@ 2022-08-04 20:58 ` Dan Williams
0 siblings, 0 replies; 3+ messages in thread
From: Dan Williams @ 2022-08-04 20:58 UTC (permalink / raw)
To: Ira Weiny, Dan Williams
Cc: linux-cxl, Dan Carpenter, vishal.l.verma, alison.schofield
Ira Weiny wrote:
> On Mon, Aug 01, 2022 at 04:00:21PM -0700, Dan Williams wrote:
> > Dan reports:
> >
> > 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.
> >
> > Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
> > flow is not clear. Fix the bug and the clarity by separating the 'new'
> ^^^^^^^
> clarify?
It's a code 'clarity' fix.
>
> > region-reference case from the 'extend' region-reference case. This also
> > moves the host-physical-address (HPA) validation, that the HPA of a new
> > region being accounted to the port is greater than the HPA of all other
> > regions associated with the port, to alloc_region_ref().
>
> Technically this means the kdoc 'steps list' is out of order a bit. :-(
>
> But I like that this is only done on creation. And I'm not sure it matter much
> because 'steps' is more of a list of things this function does.
Yeah, the steps are not compiled.
>
> >
> > Introduce @nr_targets_inc to track when the error exit path needs to
> > clean up cxl_rr->nr_targets.
> >
> > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++++------------------
> > 1 file changed, 43 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index aff4f736b63c..cf5d5811fe4c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> > static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> > struct cxl_region *cxlr)
> > {
> > - struct cxl_region_ref *cxl_rr;
> > + struct cxl_region_params *p = &cxlr->params;
> > + struct cxl_region_ref *cxl_rr, *iter;
> > + unsigned long index;
> > int rc;
> >
> > + xa_for_each(&port->regions, index, iter) {
> > + struct cxl_region_params *ip = &iter->region->params;
> > +
> > + if (ip->res->start > p->res->start) {
> > + dev_dbg(&cxlr->dev,
> > + "%s: HPA order violation %s:%pr vs %pr\n",
> > + dev_name(&port->dev),
> > + dev_name(&iter->region->dev), ip->res, p->res);
> > + return ERR_PTR(-EBUSY);
> > + }
> > + }
> > +
> > cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
> > if (!cxl_rr)
> > - return NULL;
> > + return ERR_PTR(-ENOMEM);
> > cxl_rr->port = port;
> > cxl_rr->region = cxlr;
> > cxl_rr->nr_targets = 1;
> > @@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> > "%s: failed to track region reference: %d\n",
> > dev_name(&port->dev), rc);
> > kfree(cxl_rr);
> > - return NULL;
> > + return ERR_PTR(rc);
> > }
> >
> > return cxl_rr;
> > @@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
> > {
> > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> > - struct cxl_region_ref *cxl_rr = NULL, *iter;
> > - struct cxl_region_params *p = &cxlr->params;
> > - struct cxl_decoder *cxld = NULL;
> > + 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);
> >
> > - xa_for_each(&port->regions, index, iter) {
> > - struct cxl_region_params *ip = &iter->region->params;
> > -
> > - if (iter->region == cxlr)
> > - cxl_rr = iter;
> > - if (ip->res->start > p->res->start) {
> > - dev_dbg(&cxlr->dev,
> > - "%s: HPA order violation %s:%pr vs %pr\n",
> > - dev_name(&port->dev),
> > - dev_name(&iter->region->dev), ip->res, p->res);
> > - return -EBUSY;
> > - }
> > - }
> > -
> > + cxl_rr = cxl_rr_load(port, cxlr);
> > if (cxl_rr) {
> > struct cxl_ep *ep_iter;
> > int found = 0;
> >
> > - cxld = cxl_rr->decoder;
> > + /*
> > + * 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;
> > @@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
> > }
> >
> > /*
> > - * If this is a new target or if this port is direct connected
> > - * to this endpoint then add to the target count.
> > + * New target port, or @port is an endpoint port that always
> > + * accounts its own local decode as a target.
> > */
> > - if (!found || !ep->next)
> > + 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 (!cxl_rr) {
> > + if (IS_ERR(cxl_rr)) {
> > dev_dbg(&cxlr->dev,
> > "%s: failed to allocate region reference\n",
> > dev_name(&port->dev));
> > - return -ENOMEM;
> > + return PTR_ERR(cxl_rr);
> > }
> > - }
> > + nr_targets_inc = true;
>
> I don't think this is technically needed but I would leave it if I were you.
>
> I think the leak only occurs on the extend case but I think it is safe to do
> this and does make the code cleaner.
Appreciate the detailed look. You're right that it's ultimately
pointless to do the tracking in this case because the cxl_rr instance
will be freed shortly. For the casual reader though it saves them a trip
to go read the details to convice themselves there's no leak.
> Regardless of the kdoc subtlety and with the spelling fix.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-04 20:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 23:00 [PATCH] cxl/region: Fix region reference target accounting Dan Williams
2022-08-04 20:36 ` Ira Weiny
2022-08-04 20:58 ` Dan Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.