Skip to contentSkip to conversation

dmaengine: mv_xor: protect MBUS window access with a spinlock

PATCH Superseded Rosen Penev 2 participants 4 messages Sep 16 dmaengine

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dmaengine: mv_xor: protect MBUS window access with a spinlock

mv_xor_add_io_win() reads and writes shared MBUS window registers and updates the shared win_start/win_end arrays in mv_xor_device. Multiple DMA channels can call this function concurrently via mv_xor_prep_dma_xor(), leading to races where two threads can select the same free window slot, corrupt the registers, or produce inconsistent cached state.

Add a spinlock to struct mv_xor_device and hold it across the register read-modify-write and cache update in mv_xor_add_io_win().

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by LLM
---
 drivers/dma/mv_xor.c | 15 +++++++++++++--
 drivers/dma/mv_xor.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/dma/mv_xor.c +13 -2
index 0de2b1ad5c30..e7c6ec54ce25 100644
@@ -497,11 +497,14 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 	u8 target, attr;
 	int ret;
 	int i;
+	unsigned long flags;
 
 	/* Nothing needs to get done for the Armada 3700 */
 	if (xordev->xor_type == XOR_ARMADA_37XX)
 		return 0;
 
+	spin_lock_irqsave(&xordev->win_lock, flags);
+
 	/*
 	 * Loop over the cached windows to check, if the requested area
 	 * is already mapped. If this the case, nothing needs to be done
@@ -511,6 +514,7 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 		if (addr >= xordev->win_start[i] &&
 		    addr <= xordev->win_end[i]) {
 			/* Window is already mapped */
+			spin_unlock_irqrestore(&xordev->win_lock, flags);
 			return 0;
 		}
 	}
@@ -521,8 +525,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
 	/* If no IO window is found that addr has to be located in SDRAM */
 	ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
-	if (ret < 0)
+	if (ret < 0) {
+		spin_unlock_irqrestore(&xordev->win_lock, flags);
 		return 0;
+	}
 
 	/*
 	 * Mask the base addr 'addr' according to 'size' read back from the
@@ -540,8 +546,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
 	/* Set 'i' to the first free window to write the new values to */
 	i = ffs(~win_enable) - 1;
-	if (i >= WINDOW_COUNT)
+	if (i >= WINDOW_COUNT) {
+		spin_unlock_irqrestore(&xordev->win_lock, flags);
 		return -ENOMEM;
+	}
 
 	writel((addr & 0xffff0000) | (attr << 8) | target,
 	       base + WINDOW_BASE(i));
@@ -556,6 +564,8 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 	writel(win_enable, base + WINDOW_BAR_ENABLE(0));
 	writel(win_enable, base + WINDOW_BAR_ENABLE(1));
 
+	spin_unlock_irqrestore(&xordev->win_lock, flags);
+
 	return 0;
 }
 
@@ -1353,6 +1363,7 @@ static int mv_xor_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, xordev);
 
+	spin_lock_init(&xordev->win_lock);
 
 	/*
 	 * We need to know which type of XOR device we use before
drivers/dma/mv_xor.h +1 -0
index c87cefd38a07..034db4d0bfb1 100644
@@ -80,6 +80,7 @@ struct mv_xor_device {
 	struct clk	     *clk;
 	struct mv_xor_chan   *channels[MV_XOR_MAX_CHANNELS];
 	int		     xor_type;
+	spinlock_t	     win_lock;
 
 	u32                  win_start[WINDOW_COUNT];
 	u32                  win_end[WINDOW_COUNT];
Signature · 1 line
2.55.0
#1 lore raw
Quoted 24 lines · Rosen Penev

mv_xor_add_io_win() reads and writes shared MBUS window registers and updates the shared win_start/win_end arrays in mv_xor_device. Multiple DMA channels can call this function concurrently via mv_xor_prep_dma_xor(), leading to races where two threads can select the same free window slot, corrupt the registers, or produce inconsistent cached state.

Add a spinlock to struct mv_xor_device and hold it across the register read-modify-write and cache update in mv_xor_add_io_win().

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by LLM
---
 drivers/dma/mv_xor.c | 15 +++++++++++++--
 drivers/dma/mv_xor.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/dma/mv_xor.c +0 -0
index 0de2b1ad5c30..e7c6ec54ce25 100644
@@ -497,11 +497,14 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 	u8 target, attr;

drivers/dma/mv_xor.c static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr) · line 507

Quoted 10 lines
drivers/dma/mv_xor.c +3 -0
 	int ret;
 	int i;
+	unsigned long flags;
 
 	/* Nothing needs to get done for the Armada 3700 */
 	if (xordev->xor_type == XOR_ARMADA_37XX)
 		return 0;
 
+	spin_lock_irqsave(&xordev->win_lock, flags);
+

use auto clean up guard()

Frank

Quoted 67 lines
drivers/dma/mv_xor.c +10 -2
 	/*
 	 * Loop over the cached windows to check, if the requested area
 	 * is already mapped. If this the case, nothing needs to be done
@@ -511,6 +514,7 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 		if (addr >= xordev->win_start[i] &&
 		    addr <= xordev->win_end[i]) {
 			/* Window is already mapped */
+			spin_unlock_irqrestore(&xordev->win_lock, flags);
 			return 0;
 		}
 	}
@@ -521,8 +525,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
 	/* If no IO window is found that addr has to be located in SDRAM */
 	ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
-	if (ret < 0)
+	if (ret < 0) {
+		spin_unlock_irqrestore(&xordev->win_lock, flags);
 		return 0;
+	}
 
 	/*
 	 * Mask the base addr 'addr' according to 'size' read back from the
@@ -540,8 +546,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
 	/* Set 'i' to the first free window to write the new values to */
 	i = ffs(~win_enable) - 1;
-	if (i >= WINDOW_COUNT)
+	if (i >= WINDOW_COUNT) {
+		spin_unlock_irqrestore(&xordev->win_lock, flags);
 		return -ENOMEM;
+	}
 
 	writel((addr & 0xffff0000) | (attr << 8) | target,
 	       base + WINDOW_BASE(i));
@@ -556,6 +564,8 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 	writel(win_enable, base + WINDOW_BAR_ENABLE(0));
 	writel(win_enable, base + WINDOW_BAR_ENABLE(1));
 
+	spin_unlock_irqrestore(&xordev->win_lock, flags);
+
 	return 0;
 }
 
@@ -1353,6 +1363,7 @@ static int mv_xor_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, xordev);
 
+	spin_lock_init(&xordev->win_lock);
 
 	/*
 	 * We need to know which type of XOR device we use before
drivers/dma/mv_xor.h +1 -1
index c87cefd38a07..034db4d0bfb1 100644
@@ -80,6 +80,7 @@ struct mv_xor_device {
 	struct clk	     *clk;
 	struct mv_xor_chan   *channels[MV_XOR_MAX_CHANNELS];
 	int		     xor_type;
+	spinlock_t	     win_lock;
 
 	u32                  win_start[WINDOW_COUNT];
 	u32                  win_end[WINDOW_COUNT];
--

2.55.0

#2 lore raw
Quoted 30 lines · Frank Li
Quoted 28 lines · Rosen Penev

mv_xor_add_io_win() reads and writes shared MBUS window registers and updates the shared win_start/win_end arrays in mv_xor_device. Multiple DMA channels can call this function concurrently via mv_xor_prep_dma_xor(), leading to races where two threads can select the same free window slot, corrupt the registers, or produce inconsistent cached state.

Add a spinlock to struct mv_xor_device and hold it across the register read-modify-write and cache update in mv_xor_add_io_win().

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by LLM
---
 drivers/dma/mv_xor.c | 15 +++++++++++++--
 drivers/dma/mv_xor.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/dma/mv_xor.c +1 -0
index 0de2b1ad5c30..e7c6ec54ce25 100644
@@ -497,11 +497,14 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
      u8 target, attr;
      int ret;
      int i;
+     unsigned long flags;
 
Quoted 8 lines

drivers/dma/mv_xor.c static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr) · line 507

Quoted 6 lines
drivers/dma/mv_xor.c +2 -0
      /* Nothing needs to get done for the Armada 3700 */
      if (xordev->xor_type == XOR_ARMADA_37XX)
              return 0;
 
+     spin_lock_irqsave(&xordev->win_lock, flags);
+

use auto clean up guard()

Could that be backported though?

Quoted 69 lines

Frank

Quoted 67 lines
drivers/dma/mv_xor.c +10 -2
      /*
       * Loop over the cached windows to check, if the requested area
       * is already mapped. If this the case, nothing needs to be done
@@ -511,6 +514,7 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
              if (addr >= xordev->win_start[i] &&
                  addr <= xordev->win_end[i]) {
                      /* Window is already mapped */
+                     spin_unlock_irqrestore(&xordev->win_lock, flags);
                      return 0;
              }
      }
@@ -521,8 +525,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
      /* If no IO window is found that addr has to be located in SDRAM */
      ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
-     if (ret < 0)
+     if (ret < 0) {
+             spin_unlock_irqrestore(&xordev->win_lock, flags);
              return 0;
+     }
 
      /*
       * Mask the base addr 'addr' according to 'size' read back from the
@@ -540,8 +546,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
      /* Set 'i' to the first free window to write the new values to */
      i = ffs(~win_enable) - 1;
-     if (i >= WINDOW_COUNT)
+     if (i >= WINDOW_COUNT) {
+             spin_unlock_irqrestore(&xordev->win_lock, flags);
              return -ENOMEM;
+     }
 
      writel((addr & 0xffff0000) | (attr << 8) | target,
             base + WINDOW_BASE(i));
@@ -556,6 +564,8 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
      writel(win_enable, base + WINDOW_BAR_ENABLE(0));
      writel(win_enable, base + WINDOW_BAR_ENABLE(1));
 
+     spin_unlock_irqrestore(&xordev->win_lock, flags);
+
      return 0;
 }
 
@@ -1353,6 +1363,7 @@ static int mv_xor_probe(struct platform_device *pdev)
 
      platform_set_drvdata(pdev, xordev);
 
+     spin_lock_init(&xordev->win_lock);
 
      /*
       * We need to know which type of XOR device we use before
drivers/dma/mv_xor.h +1 -1
index c87cefd38a07..034db4d0bfb1 100644
@@ -80,6 +80,7 @@ struct mv_xor_device {
      struct clk           *clk;
      struct mv_xor_chan   *channels[MV_XOR_MAX_CHANNELS];
      int                  xor_type;
+     spinlock_t           win_lock;
 
      u32                  win_start[WINDOW_COUNT];
      u32                  win_end[WINDOW_COUNT];
--

2.55.0

#3 lore raw
Quoted 35 lines · Rosen Penev
Quoted 34 lines · Frank Li
Quoted 32 lines · Rosen Penev

mv_xor_add_io_win() reads and writes shared MBUS window registers and updates the shared win_start/win_end arrays in mv_xor_device. Multiple DMA channels can call this function concurrently via mv_xor_prep_dma_xor(), leading to races where two threads can select the same free window slot, corrupt the registers, or produce inconsistent cached state.

Add a spinlock to struct mv_xor_device and hold it across the register read-modify-write and cache update in mv_xor_add_io_win().

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by LLM
---
 drivers/dma/mv_xor.c | 15 +++++++++++++--
 drivers/dma/mv_xor.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/dma/mv_xor.c +1 -0
index 0de2b1ad5c30..e7c6ec54ce25 100644
@@ -497,11 +497,14 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
      u8 target, attr;
      int ret;
      int i;
+     unsigned long flags;
 
      /* Nothing needs to get done for the Armada 3700 */
      if (xordev->xor_type == XOR_ARMADA_37XX)
              return 0;
 
Quoted 5 lines
Quoted 4 lines

drivers/dma/mv_xor.c static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr) · line 507

Quoted 2 lines
drivers/dma/mv_xor.c +2 -0
+     spin_lock_irqsave(&xordev->win_lock, flags);
+

use auto clean up guard()

Could that be backported though?

I think it should be fine now. guard() already use quick popular.

Frank

Quoted 69 lines
Quoted 69 lines

Frank

Quoted 67 lines
drivers/dma/mv_xor.c +10 -2
      /*
       * Loop over the cached windows to check, if the requested area
       * is already mapped. If this the case, nothing needs to be done
@@ -511,6 +514,7 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
              if (addr >= xordev->win_start[i] &&
                  addr <= xordev->win_end[i]) {
                      /* Window is already mapped */
+                     spin_unlock_irqrestore(&xordev->win_lock, flags);
                      return 0;
              }
      }
@@ -521,8 +525,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
      /* If no IO window is found that addr has to be located in SDRAM */
      ret = mvebu_mbus_get_io_win_info(addr, &size, &target, &attr);
-     if (ret < 0)
+     if (ret < 0) {
+             spin_unlock_irqrestore(&xordev->win_lock, flags);
              return 0;
+     }
 
      /*
       * Mask the base addr 'addr' according to 'size' read back from the
@@ -540,8 +546,10 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
 
      /* Set 'i' to the first free window to write the new values to */
      i = ffs(~win_enable) - 1;
-     if (i >= WINDOW_COUNT)
+     if (i >= WINDOW_COUNT) {
+             spin_unlock_irqrestore(&xordev->win_lock, flags);
              return -ENOMEM;
+     }
 
      writel((addr & 0xffff0000) | (attr << 8) | target,
             base + WINDOW_BASE(i));
@@ -556,6 +564,8 @@ static int mv_xor_add_io_win(struct mv_xor_chan *mv_chan, u32 addr)
      writel(win_enable, base + WINDOW_BAR_ENABLE(0));
      writel(win_enable, base + WINDOW_BAR_ENABLE(1));
 
+     spin_unlock_irqrestore(&xordev->win_lock, flags);
+
      return 0;
 }
 
@@ -1353,6 +1363,7 @@ static int mv_xor_probe(struct platform_device *pdev)
 
      platform_set_drvdata(pdev, xordev);
 
+     spin_lock_init(&xordev->win_lock);
 
      /*
       * We need to know which type of XOR device we use before
drivers/dma/mv_xor.h +1 -1
index c87cefd38a07..034db4d0bfb1 100644
@@ -80,6 +80,7 @@ struct mv_xor_device {
      struct clk           *clk;
      struct mv_xor_chan   *channels[MV_XOR_MAX_CHANNELS];
      int                  xor_type;
+     spinlock_t           win_lock;
 
      u32                  win_start[WINDOW_COUNT];
      u32                  win_end[WINDOW_COUNT];
--

2.55.0