Re: [PATCH] cxl/region: refactor decoder allocation for region refs

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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.