FAILED: patch "[PATCH] dmaengine: qcom: bam_dma: Fix resource leak" failed to apply to 4.14-stable tree

PATCH [email protected] 3 participants 5 messages Nov 4, 2019

Subsystems (3)

Stable branch Arm Qualcomm mailing list dmaengine

lore

Share & tools

lore

[email protected]

author maintainer

2019-11-04 09:35 UTC #0 lore raw

The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 7667819385457b4aeb5fac94f67f52ab52cc10d5 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <[email protected]>
Date: Thu, 17 Oct 2019 08:26:06 -0700
Subject: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak

bam_dma_terminate_all() will leak resources if any of the transactions are committed to the hardware (present in the desc fifo), and not complete. Since bam_dma_terminate_all() does not cause the hardware to be updated, the hardware will still operate on any previously committed transactions. This can cause memory corruption if the memory for the transaction has been reassigned, and will cause a sync issue between the BAM and its client(s).

Fix this by properly updating the hardware in bam_dma_terminate_all().

Fixes e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")

Signed-off-by Jeffrey Hugo <[email protected]>

Cc [email protected]

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Vinod Koul <[email protected]>

drivers/dma/qcom/bam_dma.c +19 -0

index 8e90a405939d..ef73f65224b1 100644
@@ -694,6 +694,25 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
 
 	/* remove all transactions, including active transaction */
 	spin_lock_irqsave(&bchan->vc.lock, flag);
+	/*
+	 * If we have transactions queued, then some might be committed to the
+	 * hardware in the desc fifo.  The only way to reset the desc fifo is
+	 * to do a hardware reset (either by pipe or the entire block).
+	 * bam_chan_init_hw() will trigger a pipe reset, and also reinit the
+	 * pipe.  If the pipe is left disabled (default state after pipe reset)
+	 * and is accessed by a connected hardware engine, a fatal error in
+	 * the BAM will occur.  There is a small window where this could happen
+	 * with bam_chan_init_hw(), but it is assumed that the caller has
+	 * stopped activity on any attached hardware engine.  Make sure to do
+	 * this first so that the BAM hardware doesn't cause memory corruption
+	 * by accessing freed resources.
+	 */
+	if (!list_empty(&bchan->desc_list)) {
+		async_desc = list_first_entry(&bchan->desc_list,
+					      struct bam_async_desc, desc_node);
+		bam_chan_init_hw(bchan, async_desc->dir);
+	}
+
 	list_for_each_entry_safe(async_desc, tmp,
 				 &bchan->desc_list, desc_node) {
 		list_add(&async_desc->vd.node, &bchan->vc.desc_issued);

Sasha Levin

maintainer

2019-11-04 14:07 UTC #1 lore raw

Quoted 24 lines · On Mon, Nov 04, 2019 at 10:35:26AM +0100, [email protected] wrote:

The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7667819385457b4aeb5fac94f67f52ab52cc10d5 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <[email protected]>
Date: Thu, 17 Oct 2019 08:26:06 -0700
Subject: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
bam_dma_terminate_all() will leak resources if any of the transactions are committed to the hardware (present in the desc fifo), and not complete. Since bam_dma_terminate_all() does not cause the hardware to be updated, the hardware will still operate on any previously committed transactions. This can cause memory corruption if the memory for the transaction has been reassigned, and will cause a sync issue between the BAM and its client(s).

Quoted 7 lines

Fix this by properly updating the hardware in bam_dma_terminate_all().
Fixes e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Signed-off-by Jeffrey Hugo <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Vinod Koul <[email protected]>

Is the "Fixes:" tag correct here? Is it an issue without 6b4faeac05bc ("dmaengine: qcom-bam: Process multiple pending descriptors")?

Signature · 2 lines

Thanks,
Sasha

Jeffrey Hugo

2019-11-04 14:40 UTC #2 lore raw

Quoted 28 lines · Sasha Levin

Quoted 26 lines · On Mon, Nov 04, 2019 at 10:35:26AM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7667819385457b4aeb5fac94f67f52ab52cc10d5 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <[email protected]>
Date: Thu, 17 Oct 2019 08:26:06 -0700
Subject: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
bam_dma_terminate_all() will leak resources if any of the transactions are committed to the hardware (present in the desc fifo), and not complete. Since bam_dma_terminate_all() does not cause the hardware to be updated, the hardware will still operate on any previously committed transactions. This can cause memory corruption if the memory for the transaction has been reassigned, and will cause a sync issue between the BAM and its client(s).
Fix this by properly updating the hardware in bam_dma_terminate_all().

Quoted 8 lines

Quoted 5 lines
Fixes e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Signed-off-by Jeffrey Hugo <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Vinod Koul <[email protected]>
Is the "Fixes:" tag correct here? Is it an issue without 6b4faeac05bc ("dmaengine: qcom-bam: Process multiple pending descriptors")?

Yes. The issue will occur, even if you submit only one descriptor. The uart_dm driver which exposed this issue (msm_serial), only uses one descriptor at a time, despite the hardware and some versions of the bam driver allowing more than that.

A trivial way to trigger this would be to queue a descriptor to receive data from some peripheral that is attached to the BAM dma engine, but the peripheral never sends that data - ie if you had a NIC and you wanted to prequeue a receive buffer to accept an incoming packet. If you then invoke terminate_all(), perhaps you need to renegotiate the link speed of the NIC, you'll hit the same issue - with or without "Process multiple pending descriptors".

Quoted 4 lines

--
Thanks,
Sasha

Sasha Levin

maintainer

2019-11-04 17:12 UTC #3 lore raw

Quoted 43 lines · Jeffrey Hugo

Quoted 28 lines · Sasha Levin
Quoted 26 lines · On Mon, Nov 04, 2019 at 10:35:26AM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7667819385457b4aeb5fac94f67f52ab52cc10d5 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <[email protected]>
Date: Thu, 17 Oct 2019 08:26:06 -0700
Subject: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
bam_dma_terminate_all() will leak resources if any of the transactions are committed to the hardware (present in the desc fifo), and not complete. Since bam_dma_terminate_all() does not cause the hardware to be updated, the hardware will still operate on any previously committed transactions. This can cause memory corruption if the memory for the transaction has been reassigned, and will cause a sync issue between the BAM and its client(s).
Fix this by properly updating the hardware in bam_dma_terminate_all().
Quoted 8 lines
Quoted 5 lines
Fixes e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Signed-off-by Jeffrey Hugo <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Vinod Koul <[email protected]>
Is the "Fixes:" tag correct here? Is it an issue without 6b4faeac05bc ("dmaengine: qcom-bam: Process multiple pending descriptors")?
Yes. The issue will occur, even if you submit only one descriptor. The uart_dm driver which exposed this issue (msm_serial), only uses one descriptor at a time, despite the hardware and some versions of the bam driver allowing more than that.

Quoted 7 lines

A trivial way to trigger this would be to queue a descriptor to receive data from some peripheral that is attached to the BAM dma engine, but the peripheral never sends that data - ie if you had a NIC and you wanted to prequeue a receive buffer to accept an incoming packet. If you then invoke terminate_all(), perhaps you need to renegotiate the link speed of the NIC, you'll hit the same issue - with or without "Process multiple pending descriptors".

In this case I'll happily take a backport of this patch :)

Signature · 2 lines

Thanks,
Sasha

Jeffrey Hugo

2019-11-04 17:16 UTC #4 lore raw

Quoted 53 lines · Sasha Levin

Quoted 43 lines · Jeffrey Hugo
Quoted 28 lines · Sasha Levin
Quoted 26 lines · On Mon, Nov 04, 2019 at 10:35:26AM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7667819385457b4aeb5fac94f67f52ab52cc10d5 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <[email protected]>
Date: Thu, 17 Oct 2019 08:26:06 -0700
Subject: [PATCH] dmaengine: qcom: bam_dma: Fix resource leak
bam_dma_terminate_all() will leak resources if any of the transactions are committed to the hardware (present in the desc fifo), and not complete. Since bam_dma_terminate_all() does not cause the hardware to be updated, the hardware will still operate on any previously committed transactions. This can cause memory corruption if the memory for the transaction has been reassigned, and will cause a sync issue between the BAM and its client(s).
Fix this by properly updating the hardware in bam_dma_terminate_all().
Quoted 8 lines
Quoted 5 lines
Fixes e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Signed-off-by Jeffrey Hugo <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Vinod Koul <[email protected]>
Is the "Fixes:" tag correct here? Is it an issue without 6b4faeac05bc ("dmaengine: qcom-bam: Process multiple pending descriptors")?
Yes. The issue will occur, even if you submit only one descriptor. The uart_dm driver which exposed this issue (msm_serial), only uses one descriptor at a time, despite the hardware and some versions of the bam driver allowing more than that.
Quoted 7 lines
A trivial way to trigger this would be to queue a descriptor to receive data from some peripheral that is attached to the BAM dma engine, but the peripheral never sends that data - ie if you had a NIC and you wanted to prequeue a receive buffer to accept an incoming packet. If you then invoke terminate_all(), perhaps you need to renegotiate the link speed of the NIC, you'll hit the same issue - with or without "Process multiple pending descriptors".

Quoted 1 line

In this case I'll happily take a backport of this patch :)

I'll see what I can do in the next few days.