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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 3 Oct 2023 17:59:46 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> 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.

I'm confused.  This accesses the sysfs stuff via
sec = sysfs_get_dirent(dev->kobj.sd, "security");
where dev = cxlds->dev
and cxlds is embedded in mds.

So from a code point of view I can't see the restriction.
Is it more a semantic issue that it naturally feels better to use
the cxl_mdev?

Jonathan


>