Re: [PATCH v3 07/10] cxl/memdev: Fix sanitize vs decoder setup locking

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

Ira Weiny wrote:
> Dan Williams wrote:
> > The sanitize operation is destructive and the expectation is that the
> > device is unmapped while in progress. The current implementation does a
> > lockless check for decoders being active, but then does nothing to
> > prevent decoders from racing to be committed. Introduce state tracking
> > to resolve this race.
> > 
> > This incidentally cleans up unpriveleged userspace from triggering mmio
> > read cycles by spinning on reading the 'securiry/state' attribute. Which
> > at a minimum is a waste since the kernel state machine can cache the
> > completion result.
> > 
> > Lastly cxl_mem_sanitize() was mistakenly marked EXPORT_SYMBOL() in the
> > original implementation, but an export was never required.
> > 
> > Fixes: 0c36b6ad436a ("cxl/mbox: Add sanitization handling machinery")
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> 
> [snip]
> 
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -650,6 +650,25 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> >  		return -EBUSY;
> >  	}
> >  
> > +	/*
> > +	 * For endpoint decoders hosted on CXL memory devices that
> > +	 * support the sanitize operation, make sure sanitize is not in-flight.
> > +	 */
> > +	if (is_endpoint_decoder(&cxld->dev)) {
> > +		struct cxl_endpoint_decoder *cxled =
> > +			to_cxl_endpoint_decoder(&cxld->dev);
> > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +		struct cxl_memdev_state *mds =
> > +			to_cxl_memdev_state(cxlmd->cxlds);
> > +
> > +		if (mds && mds->security.sanitize_active) {
> 
> I'm curious why this check does not need to hold the mds->mbox_mutex?  Or
> how this may be protected by the cxl_dpa_rwsem?

...because cxl_decoder_commit() knows it is holding the cxl_dpa_rwsem for
write which means that sanitize_active can not transition from false to
true in cxl_mem_sanitize().

It does mean that in-flight completions may race, but that's a benign race
where whatever triggered cxl_decoder_commit() is already in the position of
needing to wait for cxl_mbox_sanitize_work() to fire.