linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] dmaengine: idxd: Error path fixes
@ 2022-12-02 18:25 Reinette Chatre
  2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 18:25 UTC (permalink / raw)
  To: fenghua.yu, dave.jiang, vkoul, dmaengine; +Cc: linux-kernel

Dear Maintainers,

I have been using the IDXD driver to experiment with the upcoming core
changes in support of IMS ([1], [2], [3]). As part of this work I
happened to exercise the error paths within IDXD and encountered
a few issues that are addressed in this series. These changes are
independent from IMS and just aims to make the IDXD driver more
robust against errors.

It is not clear to me if these are appropriate for stable so I am
not including the stable team. Please let me know if you think
otherwise and I can add the necessary Cc. With the refactoring
through the history of the driver I was not able to
identify a Fixes: candidate for all. Patch #3 does look to be a
potentially complicated backport.

Your feedback is greatly appreciated.

Reinette

[1] https://lore.kernel.org/lkml/20221111132706.104870257@linutronix.de
[2] https://lore.kernel.org/lkml/20221111131813.914374272@linutronix.dexo
[3] https://lore.kernel.org/lkml/20221111133158.196269823@linutronix.de

Reinette Chatre (3):
  dmaengine: idxd: Let probe fail when workqueue cannot be enabled
  dmaengine: idxd: Prevent use after free on completion memory
  dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable

 drivers/dma/idxd/device.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)


base-commit: a4412fdd49dc011bcc2c0d81ac4cab7457092650
-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled
  2022-12-02 18:25 [PATCH 0/3] dmaengine: idxd: Error path fixes Reinette Chatre
@ 2022-12-02 18:25 ` Reinette Chatre
  2022-12-02 18:44   ` Dave Jiang
  2022-12-02 18:52   ` Yu, Fenghua
  2022-12-02 18:25 ` [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory Reinette Chatre
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 18:25 UTC (permalink / raw)
  To: fenghua.yu, dave.jiang, vkoul, dmaengine; +Cc: linux-kernel

The workqueue is enabled when the appropriate driver is loaded and
disabled when the driver is removed. When the driver is removed it
assumes that the workqueue was enabled successfully and proceeds to
free allocations made during workqueue enabling.

Failure during workqueue enabling does not prevent the driver from
being loaded. This is because the error path within drv_enable_wq()
returns success unless a second failure is encountered
during the error path. By returning success it is possible to load
the driver even if the workqueue cannot be enabled and
allocations that do not exist are attempted to be freed during
driver remove.

Some examples of problematic flows:
(a)

 idxd_dmaengine_drv_probe() -> drv_enable_wq() -> idxd_wq_request_irq():
 In above flow, if idxd_wq_request_irq() fails then
 idxd_wq_unmap_portal() is called on error exit path, but
 drv_enable_wq() returns 0 because idxd_wq_disable() succeeds. The
 driver is thus loaded successfully.

 idxd_dmaengine_drv_remove()->drv_disable_wq()->idxd_wq_unmap_portal()
 Above flow on driver unload triggers the WARN in devm_iounmap() because
 the device resource has already been removed during error path of
 drv_enable_wq().

(b)

 idxd_dmaengine_drv_probe() -> drv_enable_wq() -> idxd_wq_request_irq():
 In above flow, if idxd_wq_request_irq() fails then
 idxd_wq_init_percpu_ref() is never called to initialize the percpu
 counter, yet the driver loads successfully because drv_enable_wq()
 returns 0.

 idxd_dmaengine_drv_remove()->__idxd_wq_quiesce()->percpu_ref_kill():
 Above flow on driver unload triggers a BUG when attempting to drop the
 initial ref of the uninitialized percpu ref:
 BUG: kernel NULL pointer dereference, address: 0000000000000010

Fix the drv_enable_wq() error path by returning the original error that
indicates failure of workqueue enabling. This ensures that the probe
fails when an error is encountered and the driver remove paths are only
attempted when the workqueue was enabled successfully.

Fixes: 1f2bb40337f0 ("dmaengine: idxd: move wq_enable() to device.c")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/dma/idxd/device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 6f44fa8f78a5..fcd03d29a941 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1391,8 +1391,7 @@ int drv_enable_wq(struct idxd_wq *wq)
 err_irq:
 	idxd_wq_unmap_portal(wq);
 err_map_portal:
-	rc = idxd_wq_disable(wq, false);
-	if (rc < 0)
+	if (idxd_wq_disable(wq, false))
 		dev_dbg(dev, "wq %s disable failed\n", dev_name(wq_confdev(wq)));
 err:
 	return rc;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory
  2022-12-02 18:25 [PATCH 0/3] dmaengine: idxd: Error path fixes Reinette Chatre
  2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
@ 2022-12-02 18:25 ` Reinette Chatre
  2022-12-02 18:44   ` Dave Jiang
  2022-12-02 19:45   ` Yu, Fenghua
  2022-12-02 18:25 ` [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable Reinette Chatre
  2022-12-02 21:32 ` [PATCH 0/3] dmaengine: idxd: Error path fixes Yu, Fenghua
  3 siblings, 2 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 18:25 UTC (permalink / raw)
  To: fenghua.yu, dave.jiang, vkoul, dmaengine; +Cc: linux-kernel

On driver unload any pending descriptors are flushed at the
time the interrupt is freed:
idxd_dmaengine_drv_remove() ->
	drv_disable_wq() ->
		idxd_wq_free_irq() ->
			idxd_flush_pending_descs().

If there are any descriptors present that need to be flushed this
flow triggers a "not present" page fault as below:

 BUG: unable to handle page fault for address: ff391c97c70c9040
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page

The address that triggers the fault is the address of the
descriptor that was freed moments earlier via:
drv_disable_wq()->idxd_wq_free_resources()

Fix the use after free by freeing the descriptors after any possible
usage. This is done after idxd_wq_reset() to ensure that the memory
remains accessible during possible completion writes by the device.

Fixes: 63c14ae6c161 ("dmaengine: idxd: refactor wq driver enable/disable operations")
Suggested-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/dma/idxd/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index fcd03d29a941..b4d7bb923a40 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1408,11 +1408,11 @@ void drv_disable_wq(struct idxd_wq *wq)
 		dev_warn(dev, "Clients has claim on wq %d: %d\n",
 			 wq->id, idxd_wq_refcount(wq));
 
-	idxd_wq_free_resources(wq);
 	idxd_wq_unmap_portal(wq);
 	idxd_wq_drain(wq);
 	idxd_wq_free_irq(wq);
 	idxd_wq_reset(wq);
+	idxd_wq_free_resources(wq);
 	percpu_ref_exit(&wq->wq_active);
 	wq->type = IDXD_WQT_NONE;
 	wq->client_count = 0;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable
  2022-12-02 18:25 [PATCH 0/3] dmaengine: idxd: Error path fixes Reinette Chatre
  2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
  2022-12-02 18:25 ` [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory Reinette Chatre
@ 2022-12-02 18:25 ` Reinette Chatre
  2022-12-02 18:45   ` Dave Jiang
  2022-12-02 21:12   ` Yu, Fenghua
  2022-12-02 21:32 ` [PATCH 0/3] dmaengine: idxd: Error path fixes Yu, Fenghua
  3 siblings, 2 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 18:25 UTC (permalink / raw)
  To: fenghua.yu, dave.jiang, vkoul, dmaengine; +Cc: linux-kernel

On driver unload any pending descriptors are flushed and pending
DMA descriptors are explicitly completed:
idxd_dmaengine_drv_remove() ->
	drv_disable_wq() ->
		idxd_wq_free_irq() ->
			idxd_flush_pending_descs() ->
				idxd_dma_complete_txd()

With this done during driver unload any remaining descriptor is
likely stuck and can be dropped. Even so, the descriptor may still
have a callback set that could no longer be accessible. An
example of such a problem is when the dmatest fails and the dmatest
module is unloaded. The failure of dmatest leaves descriptors with
dma_async_tx_descriptor::callback pointing to code that no longer
exist. This causes a page fault as below at the time the IDXD driver
is unloaded when it attempts to run the callback:
 BUG: unable to handle page fault for address: ffffffffc0665190
 #PF: supervisor instruction fetch in kernel mode
 #PF: error_code(0x0010) - not-present page

Fix this by clearing the callback pointers on the transmit
descriptors only when workqueue is disabled.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---

History of refactoring made the Fixes: hard to identify by me.

 drivers/dma/idxd/device.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index b4d7bb923a40..2ac71a34fa34 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1156,6 +1156,7 @@ int idxd_device_load_config(struct idxd_device *idxd)
 
 static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
 {
+	struct dma_async_tx_descriptor *tx;
 	struct idxd_desc *desc, *itr;
 	struct llist_node *head;
 	LIST_HEAD(flist);
@@ -1175,6 +1176,15 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
 	list_for_each_entry_safe(desc, itr, &flist, list) {
 		list_del(&desc->list);
 		ctype = desc->completion->status ? IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
+		/*
+		 * wq is being disabled. Any remaining descriptors are
+		 * likely to be stuck and can be dropped. callback could
+		 * point to code that is no longer accessible, for example
+		 * if dmatest module has been unloaded.
+		 */
+		tx = &desc->txd;
+		tx->callback = NULL;
+		tx->callback_result = NULL;
 		idxd_dma_complete_txd(desc, ctype, true);
 	}
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled
  2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
@ 2022-12-02 18:44   ` Dave Jiang
  2022-12-02 18:52   ` Yu, Fenghua
  1 sibling, 0 replies; 13+ messages in thread
From: Dave Jiang @ 2022-12-02 18:44 UTC (permalink / raw)
  To: Reinette Chatre, fenghua.yu, vkoul, dmaengine; +Cc: linux-kernel



On 12/2/2022 11:25 AM, Reinette Chatre wrote:
> The workqueue is enabled when the appropriate driver is loaded and
> disabled when the driver is removed. When the driver is removed it
> assumes that the workqueue was enabled successfully and proceeds to
> free allocations made during workqueue enabling.
> 
> Failure during workqueue enabling does not prevent the driver from
> being loaded. This is because the error path within drv_enable_wq()
> returns success unless a second failure is encountered
> during the error path. By returning success it is possible to load
> the driver even if the workqueue cannot be enabled and
> allocations that do not exist are attempted to be freed during
> driver remove.
> 
> Some examples of problematic flows:
> (a)
> 
>   idxd_dmaengine_drv_probe() -> drv_enable_wq() -> idxd_wq_request_irq():
>   In above flow, if idxd_wq_request_irq() fails then
>   idxd_wq_unmap_portal() is called on error exit path, but
>   drv_enable_wq() returns 0 because idxd_wq_disable() succeeds. The
>   driver is thus loaded successfully.
> 
>   idxd_dmaengine_drv_remove()->drv_disable_wq()->idxd_wq_unmap_portal()
>   Above flow on driver unload triggers the WARN in devm_iounmap() because
>   the device resource has already been removed during error path of
>   drv_enable_wq().
> 
> (b)
> 
>   idxd_dmaengine_drv_probe() -> drv_enable_wq() -> idxd_wq_request_irq():
>   In above flow, if idxd_wq_request_irq() fails then
>   idxd_wq_init_percpu_ref() is never called to initialize the percpu
>   counter, yet the driver loads successfully because drv_enable_wq()
>   returns 0.
> 
>   idxd_dmaengine_drv_remove()->__idxd_wq_quiesce()->percpu_ref_kill():
>   Above flow on driver unload triggers a BUG when attempting to drop the
>   initial ref of the uninitialized percpu ref:
>   BUG: kernel NULL pointer dereference, address: 0000000000000010
> 
> Fix the drv_enable_wq() error path by returning the original error that
> indicates failure of workqueue enabling. This ensures that the probe
> fails when an error is encountered and the driver remove paths are only
> attempted when the workqueue was enabled successfully.
> 
> Fixes: 1f2bb40337f0 ("dmaengine: idxd: move wq_enable() to device.c")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/dma/idxd/device.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index 6f44fa8f78a5..fcd03d29a941 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1391,8 +1391,7 @@ int drv_enable_wq(struct idxd_wq *wq)
>   err_irq:
>   	idxd_wq_unmap_portal(wq);
>   err_map_portal:
> -	rc = idxd_wq_disable(wq, false);
> -	if (rc < 0)
> +	if (idxd_wq_disable(wq, false))
>   		dev_dbg(dev, "wq %s disable failed\n", dev_name(wq_confdev(wq)));
>   err:
>   	return rc;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory
  2022-12-02 18:25 ` [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory Reinette Chatre
@ 2022-12-02 18:44   ` Dave Jiang
  2022-12-02 19:45   ` Yu, Fenghua
  1 sibling, 0 replies; 13+ messages in thread
From: Dave Jiang @ 2022-12-02 18:44 UTC (permalink / raw)
  To: Reinette Chatre, fenghua.yu, vkoul, dmaengine; +Cc: linux-kernel



On 12/2/2022 11:25 AM, Reinette Chatre wrote:
> On driver unload any pending descriptors are flushed at the
> time the interrupt is freed:
> idxd_dmaengine_drv_remove() ->
> 	drv_disable_wq() ->
> 		idxd_wq_free_irq() ->
> 			idxd_flush_pending_descs().
> 
> If there are any descriptors present that need to be flushed this
> flow triggers a "not present" page fault as below:
> 
>   BUG: unable to handle page fault for address: ff391c97c70c9040
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0000) - not-present page
> 
> The address that triggers the fault is the address of the
> descriptor that was freed moments earlier via:
> drv_disable_wq()->idxd_wq_free_resources()
> 
> Fix the use after free by freeing the descriptors after any possible
> usage. This is done after idxd_wq_reset() to ensure that the memory
> remains accessible during possible completion writes by the device.
> 
> Fixes: 63c14ae6c161 ("dmaengine: idxd: refactor wq driver enable/disable operations")
> Suggested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/dma/idxd/device.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index fcd03d29a941..b4d7bb923a40 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1408,11 +1408,11 @@ void drv_disable_wq(struct idxd_wq *wq)
>   		dev_warn(dev, "Clients has claim on wq %d: %d\n",
>   			 wq->id, idxd_wq_refcount(wq));
>   
> -	idxd_wq_free_resources(wq);
>   	idxd_wq_unmap_portal(wq);
>   	idxd_wq_drain(wq);
>   	idxd_wq_free_irq(wq);
>   	idxd_wq_reset(wq);
> +	idxd_wq_free_resources(wq);
>   	percpu_ref_exit(&wq->wq_active);
>   	wq->type = IDXD_WQT_NONE;
>   	wq->client_count = 0;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable
  2022-12-02 18:25 ` [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable Reinette Chatre
@ 2022-12-02 18:45   ` Dave Jiang
  2022-12-02 21:12   ` Yu, Fenghua
  1 sibling, 0 replies; 13+ messages in thread
From: Dave Jiang @ 2022-12-02 18:45 UTC (permalink / raw)
  To: Reinette Chatre, fenghua.yu, vkoul, dmaengine; +Cc: linux-kernel



On 12/2/2022 11:25 AM, Reinette Chatre wrote:
> On driver unload any pending descriptors are flushed and pending
> DMA descriptors are explicitly completed:
> idxd_dmaengine_drv_remove() ->
> 	drv_disable_wq() ->
> 		idxd_wq_free_irq() ->
> 			idxd_flush_pending_descs() ->
> 				idxd_dma_complete_txd()
> 
> With this done during driver unload any remaining descriptor is
> likely stuck and can be dropped. Even so, the descriptor may still
> have a callback set that could no longer be accessible. An
> example of such a problem is when the dmatest fails and the dmatest
> module is unloaded. The failure of dmatest leaves descriptors with
> dma_async_tx_descriptor::callback pointing to code that no longer
> exist. This causes a page fault as below at the time the IDXD driver
> is unloaded when it attempts to run the callback:
>   BUG: unable to handle page fault for address: ffffffffc0665190
>   #PF: supervisor instruction fetch in kernel mode
>   #PF: error_code(0x0010) - not-present page
> 
> Fix this by clearing the callback pointers on the transmit
> descriptors only when workqueue is disabled.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
> 
> History of refactoring made the Fixes: hard to identify by me.
> 
>   drivers/dma/idxd/device.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index b4d7bb923a40..2ac71a34fa34 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1156,6 +1156,7 @@ int idxd_device_load_config(struct idxd_device *idxd)
>   
>   static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
>   {
> +	struct dma_async_tx_descriptor *tx;
>   	struct idxd_desc *desc, *itr;
>   	struct llist_node *head;
>   	LIST_HEAD(flist);
> @@ -1175,6 +1176,15 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
>   	list_for_each_entry_safe(desc, itr, &flist, list) {
>   		list_del(&desc->list);
>   		ctype = desc->completion->status ? IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
> +		/*
> +		 * wq is being disabled. Any remaining descriptors are
> +		 * likely to be stuck and can be dropped. callback could
> +		 * point to code that is no longer accessible, for example
> +		 * if dmatest module has been unloaded.
> +		 */
> +		tx = &desc->txd;
> +		tx->callback = NULL;
> +		tx->callback_result = NULL;
>   		idxd_dma_complete_txd(desc, ctype, true);
>   	}
>   }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled
  2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
  2022-12-02 18:44   ` Dave Jiang
@ 2022-12-02 18:52   ` Yu, Fenghua
  1 sibling, 0 replies; 13+ messages in thread
From: Yu, Fenghua @ 2022-12-02 18:52 UTC (permalink / raw)
  To: Chatre, Reinette, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

> The workqueue is enabled when the appropriate driver is loaded and disabled
> when the driver is removed. When the driver is removed it assumes that the
> workqueue was enabled successfully and proceeds to free allocations made
> during workqueue enabling.
...
> Fixes: 1f2bb40337f0 ("dmaengine: idxd: move wq_enable() to device.c")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory
  2022-12-02 18:25 ` [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory Reinette Chatre
  2022-12-02 18:44   ` Dave Jiang
@ 2022-12-02 19:45   ` Yu, Fenghua
  1 sibling, 0 replies; 13+ messages in thread
From: Yu, Fenghua @ 2022-12-02 19:45 UTC (permalink / raw)
  To: Chatre, Reinette, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

> On driver unload any pending descriptors are flushed at the time the interrupt is
> freed:
> idxd_dmaengine_drv_remove() ->
> 	drv_disable_wq() ->
> 		idxd_wq_free_irq() ->
> 			idxd_flush_pending_descs().
> 
> If there are any descriptors present that need to be flushed this flow triggers a
> "not present" page fault as below:
> 
>  BUG: unable to handle page fault for address: ff391c97c70c9040
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
> 
> The address that triggers the fault is the address of the descriptor that was freed
> moments earlier via:
> drv_disable_wq()->idxd_wq_free_resources()
> 
> Fix the use after free by freeing the descriptors after any possible usage. This is
...
> Fixes: 63c14ae6c161 ("dmaengine: idxd: refactor wq driver enable/disable
> operations")
> Suggested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable
  2022-12-02 18:25 ` [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable Reinette Chatre
  2022-12-02 18:45   ` Dave Jiang
@ 2022-12-02 21:12   ` Yu, Fenghua
  2022-12-02 21:51     ` Reinette Chatre
  1 sibling, 1 reply; 13+ messages in thread
From: Yu, Fenghua @ 2022-12-02 21:12 UTC (permalink / raw)
  To: Chatre, Reinette, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

Hi, Reinette,

> On driver unload any pending descriptors are flushed and pending DMA
> descriptors are explicitly completed:
> idxd_dmaengine_drv_remove() ->
> 	drv_disable_wq() ->
> 		idxd_wq_free_irq() ->
> 			idxd_flush_pending_descs() ->
> 				idxd_dma_complete_txd()
> 
> With this done during driver unload any remaining descriptor is likely stuck and
> can be dropped. Even so, the descriptor may still have a callback set that could
> no longer be accessible. An example of such a problem is when the dmatest fails
> and the dmatest module is unloaded. The failure of dmatest leaves descriptors
> with dma_async_tx_descriptor::callback pointing to code that no longer exist.
> This causes a page fault as below at the time the IDXD driver is unloaded when it
> attempts to run the callback:
>  BUG: unable to handle page fault for address: ffffffffc0665190
>  #PF: supervisor instruction fetch in kernel mode
>  #PF: error_code(0x0010) - not-present page
> 
> Fix this by clearing the callback pointers on the transmit descriptors only when
> workqueue is disabled.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> 
> History of refactoring made the Fixes: hard to identify by me.
> 
>  drivers/dma/idxd/device.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c index
> b4d7bb923a40..2ac71a34fa34 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1156,6 +1156,7 @@ int idxd_device_load_config(struct idxd_device *idxd)
> 
>  static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)  {
> +	struct dma_async_tx_descriptor *tx;

Nitpicking. It's better to move this line to below:

>  	struct idxd_desc *desc, *itr;
>  	struct llist_node *head;
>  	LIST_HEAD(flist);
> @@ -1175,6 +1176,15 @@ static void idxd_flush_pending_descs(struct
> idxd_irq_entry *ie)
>  	list_for_each_entry_safe(desc, itr, &flist, list) {

here?
+	struct dma_async_tx_descriptor *tx;

>  		list_del(&desc->list);
>  		ctype = desc->completion->status ?
> IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
> +		/*
> +		 * wq is being disabled. Any remaining descriptors are
> +		 * likely to be stuck and can be dropped. callback could
> +		 * point to code that is no longer accessible, for example
> +		 * if dmatest module has been unloaded.
> +		 */
> +		tx = &desc->txd;
> +		tx->callback = NULL;
> +		tx->callback_result = NULL;
>  		idxd_dma_complete_txd(desc, ctype, true);
>  	}
>  }
> --
> 2.34.1

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 0/3] dmaengine: idxd: Error path fixes
  2022-12-02 18:25 [PATCH 0/3] dmaengine: idxd: Error path fixes Reinette Chatre
                   ` (2 preceding siblings ...)
  2022-12-02 18:25 ` [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable Reinette Chatre
@ 2022-12-02 21:32 ` Yu, Fenghua
  2022-12-02 21:53   ` Reinette Chatre
  3 siblings, 1 reply; 13+ messages in thread
From: Yu, Fenghua @ 2022-12-02 21:32 UTC (permalink / raw)
  To: Chatre, Reinette, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

Hi, Reinette,

> It is not clear to me if these are appropriate for stable so I am not including the
> stable team. Please let me know if you think otherwise and I can add the
> necessary Cc. With the refactoring through the history of the driver I was not
> able to identify a Fixes: candidate for all. Patch #3 does look to be a potentially
> complicated backport.

It would be better to add Cc: stable because they cause series issues.

In patch 3, you may add Fixes: 403a2e236538. This is in 5.16. 5.17 and later support
SVA. We don't need to backport to too early versions.

Thanks.

-Fenghua

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable
  2022-12-02 21:12   ` Yu, Fenghua
@ 2022-12-02 21:51     ` Reinette Chatre
  0 siblings, 0 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 21:51 UTC (permalink / raw)
  To: Yu, Fenghua, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

Hi Fenghua,

On 12/2/2022 1:12 PM, Yu, Fenghua wrote:

...

>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c index
>> b4d7bb923a40..2ac71a34fa34 100644
>> --- a/drivers/dma/idxd/device.c
>> +++ b/drivers/dma/idxd/device.c
>> @@ -1156,6 +1156,7 @@ int idxd_device_load_config(struct idxd_device *idxd)
>>
>>  static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)  {
>> +	struct dma_async_tx_descriptor *tx;
> 
> Nitpicking. It's better to move this line to below:
> 
>>  	struct idxd_desc *desc, *itr;
>>  	struct llist_node *head;
>>  	LIST_HEAD(flist);
>> @@ -1175,6 +1176,15 @@ static void idxd_flush_pending_descs(struct
>> idxd_irq_entry *ie)
>>  	list_for_each_entry_safe(desc, itr, &flist, list) {
> 
> here?
> +	struct dma_async_tx_descriptor *tx;
> 

Will do.

>>  		list_del(&desc->list);
>>  		ctype = desc->completion->status ?
>> IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
>> +		/*
>> +		 * wq is being disabled. Any remaining descriptors are
>> +		 * likely to be stuck and can be dropped. callback could
>> +		 * point to code that is no longer accessible, for example
>> +		 * if dmatest module has been unloaded.
>> +		 */
>> +		tx = &desc->txd;
>> +		tx->callback = NULL;
>> +		tx->callback_result = NULL;
>>  		idxd_dma_complete_txd(desc, ctype, true);
>>  	}
>>  }
>> --
>> 2.34.1
> 
> Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thank you very much.

Reinette


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] dmaengine: idxd: Error path fixes
  2022-12-02 21:32 ` [PATCH 0/3] dmaengine: idxd: Error path fixes Yu, Fenghua
@ 2022-12-02 21:53   ` Reinette Chatre
  0 siblings, 0 replies; 13+ messages in thread
From: Reinette Chatre @ 2022-12-02 21:53 UTC (permalink / raw)
  To: Yu, Fenghua, Jiang, Dave, vkoul, dmaengine; +Cc: linux-kernel

Hi Fenghua,

On 12/2/2022 1:32 PM, Yu, Fenghua wrote:
> Hi, Reinette,
> 
>> It is not clear to me if these are appropriate for stable so I am not including the
>> stable team. Please let me know if you think otherwise and I can add the
>> necessary Cc. With the refactoring through the history of the driver I was not
>> able to identify a Fixes: candidate for all. Patch #3 does look to be a potentially
>> complicated backport.
> 
> It would be better to add Cc: stable because they cause series issues.

Thank you. Will do.

> 
> In patch 3, you may add Fixes: 403a2e236538. This is in 5.16. 5.17 and later support
> SVA. We don't need to backport to too early versions.

ok. Thank you very much.

Reinette

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-12-02 21:53 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 18:25 [PATCH 0/3] dmaengine: idxd: Error path fixes Reinette Chatre
2022-12-02 18:25 ` [PATCH 1/3] dmaengine: idxd: Let probe fail when workqueue cannot be enabled Reinette Chatre
2022-12-02 18:44   ` Dave Jiang
2022-12-02 18:52   ` Yu, Fenghua
2022-12-02 18:25 ` [PATCH 2/3] dmaengine: idxd: Prevent use after free on completion memory Reinette Chatre
2022-12-02 18:44   ` Dave Jiang
2022-12-02 19:45   ` Yu, Fenghua
2022-12-02 18:25 ` [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during workqueue disable Reinette Chatre
2022-12-02 18:45   ` Dave Jiang
2022-12-02 21:12   ` Yu, Fenghua
2022-12-02 21:51     ` Reinette Chatre
2022-12-02 21:32 ` [PATCH 0/3] dmaengine: idxd: Error path fixes Yu, Fenghua
2022-12-02 21:53   ` Reinette Chatre

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).