[PATCH] cxl/port: Fix missing target list lock

[PATCH] cxl/port: Fix missing target list lock

[Dan Williams]

cxl_port_setup_targets() modifies the ->targets[] array of a switch
decoder. target_list_show() expects to be able to emit a coherent
snapshot of that array by holding by holding ->target_lock. The
target_lock is held for write during initialization of the ->targets[]
array, but it is not held for write during cxl_port_setup_targets().

The ->target_lock() predates the introduction of @cxl_region_rwsem. That
semaphore protects changes to host-physical-address (HPA) decode which
is precisely what writes to a switch decoder's target list affects.

Replace ->target_lock with @cxl_region_rwsem.

Now the side-effect of snapshotting a unstable view of a decoder's
target list is likely benign so the Fixes: tag is presumptive.

Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c |   22 +++++++---------------
 drivers/cxl/cxl.h       |    2 --
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 57495cdc181f..6b386771cc25 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -172,14 +172,10 @@ static ssize_t target_list_show(struct device *dev,
 {
 	struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(dev);
 	ssize_t offset;
-	unsigned int seq;
 	int rc;
 
-	do {
-		seq = read_seqbegin(&cxlsd->target_lock);
-		rc = emit_target_list(cxlsd, buf);
-	} while (read_seqretry(&cxlsd->target_lock, seq));
-
+	guard(rwsem_read)(&cxl_region_rwsem);
+	rc = emit_target_list(cxlsd, buf);
 	if (rc < 0)
 		return rc;
 	offset = rc;
@@ -1633,7 +1629,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
 static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 				    struct cxl_port *port, int *target_map)
 {
-	int i, rc = 0;
+	int i;
 
 	if (!target_map)
 		return 0;
@@ -1643,19 +1639,16 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 	if (xa_empty(&port->dports))
 		return -EINVAL;
 
-	write_seqlock(&cxlsd->target_lock);
+	guard(rwsem_write)(&cxl_region_rwsem);
 	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
 		struct cxl_dport *dport = find_dport(port, target_map[i]);
 
-		if (!dport) {
-			rc = -ENXIO;
-			break;
-		}
+		if (!dport)
+			return -ENXIO;
 		cxlsd->target[i] = dport;
 	}
-	write_sequnlock(&cxlsd->target_lock);
 
-	return rc;
+	return 0;
 }
 
 struct cxl_dport *cxl_hb_modulo(struct cxl_root_decoder *cxlrd, int pos)
@@ -1725,7 +1718,6 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
 		return -EINVAL;
 
 	cxlsd->nr_targets = nr_targets;
-	seqlock_init(&cxlsd->target_lock);
 	return cxl_decoder_init(port, &cxlsd->cxld);
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 687043ece101..62fa96f8567e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -412,7 +412,6 @@ struct cxl_endpoint_decoder {
 /**
  * struct cxl_switch_decoder - Switch specific CXL HDM Decoder
  * @cxld: base cxl_decoder object
- * @target_lock: coordinate coherent reads of the target list
  * @nr_targets: number of elements in @target
  * @target: active ordered target list in current decoder configuration
  *
@@ -424,7 +423,6 @@ struct cxl_endpoint_decoder {
  */
 struct cxl_switch_decoder {
 	struct cxl_decoder cxld;
-	seqlock_t target_lock;
 	int nr_targets;
 	struct cxl_dport *target[];
 };

Re: [PATCH] cxl/port: Fix missing target list lock

[Dave Jiang]

On 12/21/23 23:12, Dan Williams wrote:
> cxl_port_setup_targets() modifies the ->targets[] array of a switch
> decoder. target_list_show() expects to be able to emit a coherent
> snapshot of that array by holding by holding ->target_lock. The
> target_lock is held for write during initialization of the ->targets[]
> array, but it is not held for write during cxl_port_setup_targets().
> 
> The ->target_lock() predates the introduction of @cxl_region_rwsem. That
> semaphore protects changes to host-physical-address (HPA) decode which
> is precisely what writes to a switch decoder's target list affects.
> 
> Replace ->target_lock with @cxl_region_rwsem.
> 
> Now the side-effect of snapshotting a unstable view of a decoder's
> target list is likely benign so the Fixes: tag is presumptive.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/port.c |   22 +++++++---------------
>  drivers/cxl/cxl.h       |    2 --
>  2 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 57495cdc181f..6b386771cc25 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -172,14 +172,10 @@ static ssize_t target_list_show(struct device *dev,
>  {
>  	struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(dev);
>  	ssize_t offset;
> -	unsigned int seq;
>  	int rc;
>  
> -	do {
> -		seq = read_seqbegin(&cxlsd->target_lock);
> -		rc = emit_target_list(cxlsd, buf);
> -	} while (read_seqretry(&cxlsd->target_lock, seq));
> -
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	rc = emit_target_list(cxlsd, buf);
>  	if (rc < 0)
>  		return rc;
>  	offset = rc;
> @@ -1633,7 +1629,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>  				    struct cxl_port *port, int *target_map)
>  {
> -	int i, rc = 0;
> +	int i;
>  
>  	if (!target_map)
>  		return 0;
> @@ -1643,19 +1639,16 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>  	if (xa_empty(&port->dports))
>  		return -EINVAL;
>  
> -	write_seqlock(&cxlsd->target_lock);
> +	guard(rwsem_write)(&cxl_region_rwsem);
>  	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
>  		struct cxl_dport *dport = find_dport(port, target_map[i]);
>  
> -		if (!dport) {
> -			rc = -ENXIO;
> -			break;
> -		}
> +		if (!dport)
> +			return -ENXIO;
>  		cxlsd->target[i] = dport;
>  	}
> -	write_sequnlock(&cxlsd->target_lock);
>  
> -	return rc;
> +	return 0;
>  }
>  
>  struct cxl_dport *cxl_hb_modulo(struct cxl_root_decoder *cxlrd, int pos)
> @@ -1725,7 +1718,6 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
>  		return -EINVAL;
>  
>  	cxlsd->nr_targets = nr_targets;
> -	seqlock_init(&cxlsd->target_lock);
>  	return cxl_decoder_init(port, &cxlsd->cxld);
>  }
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 687043ece101..62fa96f8567e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -412,7 +412,6 @@ struct cxl_endpoint_decoder {
>  /**
>   * struct cxl_switch_decoder - Switch specific CXL HDM Decoder
>   * @cxld: base cxl_decoder object
> - * @target_lock: coordinate coherent reads of the target list
>   * @nr_targets: number of elements in @target
>   * @target: active ordered target list in current decoder configuration
>   *
> @@ -424,7 +423,6 @@ struct cxl_endpoint_decoder {
>   */
>  struct cxl_switch_decoder {
>  	struct cxl_decoder cxld;
> -	seqlock_t target_lock;
>  	int nr_targets;
>  	struct cxl_dport *target[];
>  };
> 
> 

Re: [PATCH] cxl/port: Fix missing target list lock

[Alison Schofield]

On Thu, Dec 21, 2023 at 10:12:19PM -0800, Dan Williams wrote:
> cxl_port_setup_targets() modifies the ->targets[] array of a switch
> decoder. target_list_show() expects to be able to emit a coherent
> snapshot of that array by holding by holding ->target_lock. The
> target_lock is held for write during initialization of the ->targets[]
> array, but it is not held for write during cxl_port_setup_targets().
> 
> The ->target_lock() predates the introduction of @cxl_region_rwsem. That
> semaphore protects changes to host-physical-address (HPA) decode which
> is precisely what writes to a switch decoder's target list affects.
> 
> Replace ->target_lock with @cxl_region_rwsem.
> 
> Now the side-effect of snapshotting a unstable view of a decoder's
> target list is likely benign so the Fixes: tag is presumptive.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>