[PATCH] cxl/pmem: Fix reference counting for delayed work

[PATCH] cxl/pmem: Fix reference counting for delayed work

[Dan Williams]

There is a potential race between queue_work() returning and the
queued-work running that could result in put_device() running before
get_device(). Introduce the cxl_nvdimm_bridge_state_work() helper that
takes the reference unconditionally, but drops it if no new work was
queued, to keep the references balanced.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pmem.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index ceb2115981e5..38bcbb4e9409 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -266,14 +266,24 @@ static void cxl_nvb_update_state(struct work_struct *work)
 	put_device(&cxl_nvb->dev);
 }
 
+static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
+{
+	/*
+	 * Take a reference that the workqueue will drop if new work
+	 * gets queued.
+	 */
+	get_device(&cxl_nvb->dev);
+	if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
+		put_device(&cxl_nvb->dev);
+}
+
 static void cxl_nvdimm_bridge_remove(struct device *dev)
 {
 	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
 
 	if (cxl_nvb->state == CXL_NVB_ONLINE)
 		cxl_nvb->state = CXL_NVB_OFFLINE;
-	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
-		get_device(&cxl_nvb->dev);
+	cxl_nvdimm_bridge_state_work(cxl_nvb);
 }
 
 static int cxl_nvdimm_bridge_probe(struct device *dev)
@@ -294,8 +304,7 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
 	}
 
 	cxl_nvb->state = CXL_NVB_ONLINE;
-	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
-		get_device(&cxl_nvb->dev);
+	cxl_nvdimm_bridge_state_work(cxl_nvb);
 
 	return 0;
 }

Re: [PATCH] cxl/pmem: Fix reference counting for delayed work

[Ben Widawsky]

On 21-10-29 12:55:47, Dan Williams wrote:
> There is a potential race between queue_work() returning and the
> queued-work running that could result in put_device() running before
> get_device(). Introduce the cxl_nvdimm_bridge_state_work() helper that
> takes the reference unconditionally, but drops it if no new work was
> queued, to keep the references balanced.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Arguably fixes/stable?
Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

> ---
>  drivers/cxl/pmem.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index ceb2115981e5..38bcbb4e9409 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -266,14 +266,24 @@ static void cxl_nvb_update_state(struct work_struct *work)
>  	put_device(&cxl_nvb->dev);
>  }
>  
> +static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
> +{
> +	/*
> +	 * Take a reference that the workqueue will drop if new work
> +	 * gets queued.
> +	 */
> +	get_device(&cxl_nvb->dev);
> +	if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> +		put_device(&cxl_nvb->dev);
> +}
> +
>  static void cxl_nvdimm_bridge_remove(struct device *dev)
>  {
>  	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
>  
>  	if (cxl_nvb->state == CXL_NVB_ONLINE)
>  		cxl_nvb->state = CXL_NVB_OFFLINE;
> -	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> -		get_device(&cxl_nvb->dev);
> +	cxl_nvdimm_bridge_state_work(cxl_nvb);
>  }
>  
>  static int cxl_nvdimm_bridge_probe(struct device *dev)
> @@ -294,8 +304,7 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
>  	}
>  
>  	cxl_nvb->state = CXL_NVB_ONLINE;
> -	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> -		get_device(&cxl_nvb->dev);
> +	cxl_nvdimm_bridge_state_work(cxl_nvb);
>  
>  	return 0;
>  }
> 

Re: [PATCH] cxl/pmem: Fix reference counting for delayed work

[Dan Williams]

On Sun, Oct 31, 2021 at 11:58 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-10-29 12:55:47, Dan Williams wrote:
> > There is a potential race between queue_work() returning and the
> > queued-work running that could result in put_device() running before
> > get_device(). Introduce the cxl_nvdimm_bridge_state_work() helper that
> > takes the reference unconditionally, but drops it if no new work was
> > queued, to keep the references balanced.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Arguably fixes/stable?

Whoops, yes, definitely needs a Fixes:. A stable cc is debatable, but
if autosel picked up this fix I wouldn't say no, so might as well cc
stable upfront.

> Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

Thanks!

Re: [PATCH] cxl/pmem: Fix reference counting for delayed work

[Jonathan Cameron]

On Fri, 29 Oct 2021 12:55:47 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> There is a potential race between queue_work() returning and the
> queued-work running that could result in put_device() running before
> get_device(). Introduce the cxl_nvdimm_bridge_state_work() helper that
> takes the reference unconditionally, but drops it if no new work was
> queued, to keep the references balanced.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Good spot, I'm guessing this was an inspection thing rather than a problem
you've managed to trigger, but either way.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/pmem.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index ceb2115981e5..38bcbb4e9409 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -266,14 +266,24 @@ static void cxl_nvb_update_state(struct work_struct *work)
>  	put_device(&cxl_nvb->dev);
>  }
>  
> +static void cxl_nvdimm_bridge_state_work(struct cxl_nvdimm_bridge *cxl_nvb)
> +{
> +	/*
> +	 * Take a reference that the workqueue will drop if new work
> +	 * gets queued.
> +	 */
> +	get_device(&cxl_nvb->dev);
> +	if (!queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> +		put_device(&cxl_nvb->dev);
> +}
> +
>  static void cxl_nvdimm_bridge_remove(struct device *dev)
>  {
>  	struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
>  
>  	if (cxl_nvb->state == CXL_NVB_ONLINE)
>  		cxl_nvb->state = CXL_NVB_OFFLINE;
> -	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> -		get_device(&cxl_nvb->dev);
> +	cxl_nvdimm_bridge_state_work(cxl_nvb);
>  }
>  
>  static int cxl_nvdimm_bridge_probe(struct device *dev)
> @@ -294,8 +304,7 @@ static int cxl_nvdimm_bridge_probe(struct device *dev)
>  	}
>  
>  	cxl_nvb->state = CXL_NVB_ONLINE;
> -	if (queue_work(cxl_pmem_wq, &cxl_nvb->state_work))
> -		get_device(&cxl_nvb->dev);
> +	cxl_nvdimm_bridge_state_work(cxl_nvb);
>  
>  	return 0;
>  }
> 

Re: [PATCH] cxl/pmem: Fix reference counting for delayed work

[Dan Williams]

On Mon, Nov 1, 2021 at 4:30 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 29 Oct 2021 12:55:47 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > There is a potential race between queue_work() returning and the
> > queued-work running that could result in put_device() running before
> > get_device(). Introduce the cxl_nvdimm_bridge_state_work() helper that
> > takes the reference unconditionally, but drops it if no new work was
> > queued, to keep the references balanced.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Good spot, I'm guessing this was an inspection thing rather than a problem
> you've managed to trigger, but either way.

Correct, it was by inspection. I expect it would be extremely
difficult to hit this race in practice.

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!