| Message ID | 169602898447.904193.4454973423100628922.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/mem: Fix shutdown order | expand |
Dan Williams 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> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
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? > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + struct device *dev = cxlds->dev; > + struct kernfs_node *sec; > + > + if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) > + return 0; > + > + sec = sysfs_get_dirent(dev->kobj.sd, "security"); > + if (!sec) { > + dev_err(dev, "sanitize notification setup failure\n"); > + return -ENOENT; > + } > + mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); > + sysfs_put(sec); > + if (!mds->security.sanitize_node) { > + dev_err(dev, "sanitize notification setup failure\n"); > + return -ENOENT; > + } > + > + return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); > +} > + > /** > * __cxl_pci_mbox_send_cmd() - Execute a mailbox command > * @mds: The memory device driver data > @@ -875,6 +918,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > + rc = cxl_sanitize_setup_notifier(cxlmd); > + if (rc) > + return rc; > + > pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); > for (i = 0; i < pmu_count; i++) { > struct cxl_pmu_regs pmu_regs; >
On 9/29/23 16:09, Dan Williams 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> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > 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) > +{ > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + struct device *dev = cxlds->dev; > + struct kernfs_node *sec; > + > + if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) > + return 0; > + > + sec = sysfs_get_dirent(dev->kobj.sd, "security"); > + if (!sec) { > + dev_err(dev, "sanitize notification setup failure\n"); > + return -ENOENT; > + } > + mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); > + sysfs_put(sec); > + if (!mds->security.sanitize_node) { > + dev_err(dev, "sanitize notification setup failure\n"); > + return -ENOENT; > + } > + > + return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); > +} > + > /** > * __cxl_pci_mbox_send_cmd() - Execute a mailbox command > * @mds: The memory device driver data > @@ -875,6 +918,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > + rc = cxl_sanitize_setup_notifier(cxlmd); > + if (rc) > + return rc; > + > pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); > for (i = 0; i < pmu_count; i++) { > struct cxl_pmu_regs pmu_regs; >
On Fri, 29 Sep 2023, Dan Williams wrote: >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) >+{ >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >+ struct device *dev = cxlds->dev; This wants to be dev = &cxlmd->dev; >+ struct kernfs_node *sec; >+ >+ if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) >+ return 0; >+ >+ sec = sysfs_get_dirent(dev->kobj.sd, "security"); >+ if (!sec) { >+ dev_err(dev, "sanitize notification setup failure\n"); Nit: Should the error message differ from the next one? Maybe s/sanitize/security? >+ return -ENOENT; >+ } >+ mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); >+ sysfs_put(sec); >+ if (!mds->security.sanitize_node) { >+ dev_err(dev, "sanitize notification setup failure\n"); >+ return -ENOENT; >+ } >+ >+ return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); >+}
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.
Davidlohr Bueso wrote: > On Fri, 29 Sep 2023, Dan Williams wrote: > > >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) > >+{ > >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; > >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > >+ struct device *dev = cxlds->dev; > > This wants to be dev = &cxlmd->dev; No, the notifier needs to be torn down in the cxl_pci teardown path. If cxlmd->dev was the devm operations host then this notifier would need to be setup from the cxl_mem driver, not cxl_pci. The teardown order of cxl_pci ends with @cxlds->dev as the devm host ends up as: cxl_sanitize_teardown_notifier() cxl_memdev_unregister() ...otherwise if the @cxlmd->dev is used then the devm callback may not fire until device_release() time since it is possible that the cxl_mem driver never attaches to trigger the typical devm action around ->remove() time. > >+ struct kernfs_node *sec; > >+ > >+ if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) > >+ return 0; > >+ > >+ sec = sysfs_get_dirent(dev->kobj.sd, "security"); > >+ if (!sec) { > >+ dev_err(dev, "sanitize notification setup failure\n"); > > Nit: Should the error message differ from the next one? Maybe s/sanitize/security? Maybe, but these things will likely never fire, as devm_cxl_add_memdev() would have failed before getting here. Maybe just delete them?
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 >
On Tue, 03 Oct 2023, Dan Williams wrote: >Davidlohr Bueso wrote: >> On Fri, 29 Sep 2023, Dan Williams wrote: >> >> >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) >> >+{ >> >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; >> >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >> >+ struct device *dev = cxlds->dev; >> >> This wants to be dev = &cxlmd->dev; > >No, the notifier needs to be torn down in the cxl_pci teardown path. If >cxlmd->dev was the devm operations host then this notifier would need to >be setup from the cxl_mem driver, not cxl_pci. The teardown order of >cxl_pci ends with @cxlds->dev as the devm host ends up as: > >cxl_sanitize_teardown_notifier() >cxl_memdev_unregister() > >...otherwise if the @cxlmd->dev is used then the devm callback may not >fire until device_release() time since it is possible that the cxl_mem >driver never attaches to trigger the typical devm action around >->remove() time. As is, the first sysfs_get_dirent() fails, fyi.
Jonathan Cameron wrote: > 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. Ah, no that's a bug I introduced. The dev should be &cxlmd->dev and I made this mistake when combining the code. > 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? This is also why I wanted a positive test result that I did not introduce some silly bug, which I did. My typical capture of silly bugs is cxl_test, but there is no cxl_test infrastructure for sanitize.
Davidlohr Bueso wrote: > On Tue, 03 Oct 2023, Dan Williams wrote: > > >Davidlohr Bueso wrote: > >> On Fri, 29 Sep 2023, Dan Williams wrote: > >> > >> >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) > >> >+{ > >> >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; > >> >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > >> >+ struct device *dev = cxlds->dev; > >> > >> This wants to be dev = &cxlmd->dev; > > > >No, the notifier needs to be torn down in the cxl_pci teardown path. If > >cxlmd->dev was the devm operations host then this notifier would need to > >be setup from the cxl_mem driver, not cxl_pci. The teardown order of > >cxl_pci ends with @cxlds->dev as the devm host ends up as: > > > >cxl_sanitize_teardown_notifier() > >cxl_memdev_unregister() > > > >...otherwise if the @cxlmd->dev is used then the devm callback may not > >fire until device_release() time since it is possible that the cxl_mem > >driver never attaches to trigger the typical devm action around > >->remove() time. > > As is, the first sysfs_get_dirent() fails, fyi. Yup, thanks for testing, I am using the wrong device to lookup the sysfs entry. What would it take to get your test scenario into cxl_test... or at least a recipe for it in QEMU that can be checked into the cxl-cli tests?
Davidlohr Bueso wrote: > On Tue, 03 Oct 2023, Dan Williams wrote: > > >Davidlohr Bueso wrote: > >> On Fri, 29 Sep 2023, Dan Williams wrote: > >> > >> >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) > >> >+{ > >> >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; > >> >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > >> >+ struct device *dev = cxlds->dev; > >> > >> This wants to be dev = &cxlmd->dev; > > > >No, the notifier needs to be torn down in the cxl_pci teardown path. If > >cxlmd->dev was the devm operations host then this notifier would need to > >be setup from the cxl_mem driver, not cxl_pci. The teardown order of > >cxl_pci ends with @cxlds->dev as the devm host ends up as: > > > >cxl_sanitize_teardown_notifier() > >cxl_memdev_unregister() > > > >...otherwise if the @cxlmd->dev is used then the devm callback may not > >fire until device_release() time since it is possible that the cxl_mem > >driver never attaches to trigger the typical devm action around > >->remove() time. > > As is, the first sysfs_get_dirent() fails, fyi. I should say that I misread your motivation for changing the dev because that would also change the devm host, and now I see the bug.
Davidlohr Bueso wrote: > On Tue, 03 Oct 2023, Dan Williams wrote: > > >Davidlohr Bueso wrote: > >> On Fri, 29 Sep 2023, Dan Williams wrote: > >> > >> >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) > >> >+{ > >> >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; > >> >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > >> >+ struct device *dev = cxlds->dev; > >> > >> This wants to be dev = &cxlmd->dev; > > > >No, the notifier needs to be torn down in the cxl_pci teardown path. If > >cxlmd->dev was the devm operations host then this notifier would need to > >be setup from the cxl_mem driver, not cxl_pci. The teardown order of > >cxl_pci ends with @cxlds->dev as the devm host ends up as: > > > >cxl_sanitize_teardown_notifier() > >cxl_memdev_unregister() > > > >...otherwise if the @cxlmd->dev is used then the devm callback may not > >fire until device_release() time since it is possible that the cxl_mem > >driver never attaches to trigger the typical devm action around > >->remove() time. > > As is, the first sysfs_get_dirent() fails, fyi. Here is the full fix, apologies for misreading your fix earlier: diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index f69f6195c465..a84932ad9392 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -187,25 +187,25 @@ static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) { struct cxl_dev_state *cxlds = cxlmd->cxlds; struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); - struct device *dev = cxlds->dev; + struct device *host = cxlds->dev; struct kernfs_node *sec; if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) return 0; - sec = sysfs_get_dirent(dev->kobj.sd, "security"); + sec = sysfs_get_dirent(cxlmd->dev.kobj.sd, "security"); if (!sec) { - dev_err(dev, "sanitize notification setup failure\n"); + dev_err(host, "sanitize notification setup failure\n"); return -ENOENT; } mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); sysfs_put(sec); if (!mds->security.sanitize_node) { - dev_err(dev, "sanitize notification setup failure\n"); + dev_err(host, "sanitize notification setup failure\n"); return -ENOENT; } - return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); + return devm_add_action_or_reset(host, cxl_sanitize_teardown_notifier, mds); } /**
On Wed, 04 Oct 2023, Dan Williams wrote: >Davidlohr Bueso wrote: >> On Tue, 03 Oct 2023, Dan Williams wrote: >> >> >Davidlohr Bueso wrote: >> >> On Fri, 29 Sep 2023, Dan Williams wrote: >> >> >> >> >+static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) >> >> >+{ >> >> >+ struct cxl_dev_state *cxlds = cxlmd->cxlds; >> >> >+ struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >> >> >+ struct device *dev = cxlds->dev; >> >> >> >> This wants to be dev = &cxlmd->dev; >> > >> >No, the notifier needs to be torn down in the cxl_pci teardown path. If >> >cxlmd->dev was the devm operations host then this notifier would need to >> >be setup from the cxl_mem driver, not cxl_pci. The teardown order of >> >cxl_pci ends with @cxlds->dev as the devm host ends up as: >> > >> >cxl_sanitize_teardown_notifier() >> >cxl_memdev_unregister() >> > >> >...otherwise if the @cxlmd->dev is used then the devm callback may not >> >fire until device_release() time since it is possible that the cxl_mem >> >driver never attaches to trigger the typical devm action around >> >->remove() time. >> >> As is, the first sysfs_get_dirent() fails, fyi. > >Here is the full fix, apologies for misreading your fix earlier: Yep, looks good (and tested). > >diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >index f69f6195c465..a84932ad9392 100644 >--- a/drivers/cxl/pci.c >+++ b/drivers/cxl/pci.c >@@ -187,25 +187,25 @@ static int cxl_sanitize_setup_notifier(struct cxl_memdev *cxlmd) > { > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >- struct device *dev = cxlds->dev; >+ struct device *host = cxlds->dev; > struct kernfs_node *sec; > > if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) > return 0; > >- sec = sysfs_get_dirent(dev->kobj.sd, "security"); >+ sec = sysfs_get_dirent(cxlmd->dev.kobj.sd, "security"); > if (!sec) { >- dev_err(dev, "sanitize notification setup failure\n"); >+ dev_err(host, "sanitize notification setup failure\n"); > return -ENOENT; > } > mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); > sysfs_put(sec); > if (!mds->security.sanitize_node) { >- dev_err(dev, "sanitize notification setup failure\n"); >+ dev_err(host, "sanitize notification setup failure\n"); > return -ENOENT; > } > >- return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); >+ return devm_add_action_or_reset(host, cxl_sanitize_teardown_notifier, mds); > } > > /** >
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) +{ + struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); + struct device *dev = cxlds->dev; + struct kernfs_node *sec; + + if (!test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) + return 0; + + sec = sysfs_get_dirent(dev->kobj.sd, "security"); + if (!sec) { + dev_err(dev, "sanitize notification setup failure\n"); + return -ENOENT; + } + mds->security.sanitize_node = sysfs_get_dirent(sec, "state"); + sysfs_put(sec); + if (!mds->security.sanitize_node) { + dev_err(dev, "sanitize notification setup failure\n"); + return -ENOENT; + } + + return devm_add_action_or_reset(dev, cxl_sanitize_teardown_notifier, mds); +} + /** * __cxl_pci_mbox_send_cmd() - Execute a mailbox command * @mds: The memory device driver data @@ -875,6 +918,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; + rc = cxl_sanitize_setup_notifier(cxlmd); + if (rc) + return rc; + pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU); for (i = 0; i < pmu_count; i++) { struct cxl_pmu_regs pmu_regs;
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> --- drivers/cxl/core/memdev.c | 42 ---------------------------------------- drivers/cxl/pci.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 42 deletions(-)