Re: [PATCH v2 3/4] cxl/pci: Fix sanitize notifier setup

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Fri, 29 Sep 2023 16:09:44 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Fix a race condition between the mailbox-background command interrupt
> > firing and the security-state sysfs attribute being removed.
> > 
> > The race is difficult to see due to the awkward placement of the
> > sanitize-notifier setup code and the multiple places the teardown calls
> > are made, cxl_memdev_security_init() and cxl_memdev_security_shutdown().
> > 
> > Unify setup in one place, cxl_sanitize_setup_notifier(). Arrange for
> > the paired cxl_sanitize_teardown_notifier() to safely quiet the notifier
> > and let the cxl_memdev + irq be unregistered later in the flow.
> > 
> > This fix is also needed as a preparation fix for a memdev unregistration
> > crash.
> > 
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> One trivial question inline about which parameter to pass in from the
> many many interlocking state structures...
> 
> If you do make the suggested change, it's just complex enough I want another
> look so I'm not giving a tag for now.
> 
> > ---
> >  drivers/cxl/core/memdev.c |   42 ----------------------------------------
> >  drivers/cxl/pci.c         |   47 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 2a7a07f6d165..a950091e5640 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -556,20 +556,11 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
> >  }
> >  EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, CXL);
> >  
> > -static void cxl_memdev_security_shutdown(struct device *dev)
> > -{
> > -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> > -
> > -	cancel_delayed_work_sync(&mds->security.poll_dwork);
> > -}
> > -
> >  static void cxl_memdev_shutdown(struct device *dev)
> >  {
> >  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> >  
> >  	down_write(&cxl_memdev_rwsem);
> > -	cxl_memdev_security_shutdown(dev);
> >  	cxlmd->cxlds = NULL;
> >  	up_write(&cxl_memdev_rwsem);
> >  }
> > @@ -1001,35 +992,6 @@ static const struct file_operations cxl_memdev_fops = {
> >  	.llseek = noop_llseek,
> >  };
> >  
> > -static void put_sanitize(void *data)
> > -{
> > -	struct cxl_memdev_state *mds = data;
> > -
> > -	sysfs_put(mds->security.sanitize_node);
> > -}
> > -
> > -static int cxl_memdev_security_init(struct cxl_memdev *cxlmd)
> > -{
> > -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > -	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> > -	struct device *dev = &cxlmd->dev;
> > -	struct kernfs_node *sec;
> > -
> > -	sec = sysfs_get_dirent(dev->kobj.sd, "security");
> > -	if (!sec) {
> > -		dev_err(dev, "sysfs_get_dirent 'security' failed\n");
> > -		return -ENODEV;
> > -	}
> > -	mds->security.sanitize_node = sysfs_get_dirent(sec, "state");
> > -	sysfs_put(sec);
> > -	if (!mds->security.sanitize_node) {
> > -		dev_err(dev, "sysfs_get_dirent 'state' failed\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	return devm_add_action_or_reset(cxlds->dev, put_sanitize, mds);
> > - }
> > -
> >  struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> >  {
> >  	struct cxl_memdev *cxlmd;
> > @@ -1058,10 +1020,6 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> >  	if (rc)
> >  		goto err;
> >  
> > -	rc = cxl_memdev_security_init(cxlmd);
> > -	if (rc)
> > -		goto err;
> > -
> >  	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> >  	if (rc)
> >  		return ERR_PTR(rc);
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index ac4e434b0806..b0023e479315 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -165,6 +165,49 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
> >  	mutex_unlock(&mds->mbox_mutex);
> >  }
> >  
> > +static void cxl_sanitize_teardown_notifier(void *data)
> > +{
> > +	struct cxl_memdev_state *mds = data;
> > +	struct kernfs_node *state;
> > +
> > +	/*
> > +	 * Prevent new irq triggered invocations of the workqueue and
> > +	 * flush inflight invocations.
> > +	 */
> > +	mutex_lock(&mds->mbox_mutex);
> > +	state = mds->security.sanitize_node;
> > +	mds->security.sanitize_node = NULL;
> > +	mutex_unlock(&mds->mbox_mutex);
> > +
> > +	cancel_delayed_work_sync(&mds->security.poll_dwork);
> > +	sysfs_put(state);
> > +}
> > +
> > +static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd)
> > +{
> 
> Almost everything in cxl_pci_probe() passes in the mds.
> Why not do the same here?

Because this one really is built on top of a stack of things and needs
the 'device' because it is tying the device's sysfs attributes to the
completion notifications of the background workqueue.

I mentioned this in the cover, but failed to mention it again in this
changelog:

"The special wrinkle of the sanitize notifier is that it interacts with
interrupts, which are enabled early in the flow, and it interacts with
memdev sysfs which is not initialized until late in the flow."

There are no sysfs attributes reachable from an @mds.