linux-kernel - Re: [PATCH] spi: spi-fsl-dspi: Free DMA memory with matching function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Jun 2020 16:32:49 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] spi: spi-fsl-dspi: Free DMA memory with matching function

On 6/10/20 6:42 PM, Krzysztof Kozlowski wrote:
> 
> Driver allocates DMA memory with dma_alloc_coherent() but frees it with
> dma_unmap_single().
> 
> This causes DMA warning during system shutdown (with DMA debugging) on
> Toradex Colibri VF50 module:
> 
>      WARNING: CPU: 0 PID: 1 at ../kernel/dma/debug.c:1036 check_unmap+0x3fc/0xb04
>      DMA-API: fsl-edma 40098000.dma-controller: device driver frees DMA memory with wrong function
>        [device address=0x0000000087040000] [size=8 bytes] [mapped as coherent] [unmapped as single]
>      Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
>        (unwind_backtrace) from [<8010bb34>] (show_stack+0x10/0x14)
>        (show_stack) from [<8011ced8>] (__warn+0xf0/0x108)
>        (__warn) from [<8011cf64>] (warn_slowpath_fmt+0x74/0xb8)
>        (warn_slowpath_fmt) from [<8017d170>] (check_unmap+0x3fc/0xb04)
>        (check_unmap) from [<8017d900>] (debug_dma_unmap_page+0x88/0x90)
>        (debug_dma_unmap_page) from [<80601d68>] (dspi_release_dma+0x88/0x110)
>        (dspi_release_dma) from [<80601e4c>] (dspi_shutdown+0x5c/0x80)
>        (dspi_shutdown) from [<805845f8>] (device_shutdown+0x17c/0x220)
>        (device_shutdown) from [<80143ef8>] (kernel_restart+0xc/0x50)
>        (kernel_restart) from [<801441cc>] (__do_sys_reboot+0x18c/0x210)
>        (__do_sys_reboot) from [<80100060>] (ret_fast_syscall+0x0/0x28)
>      DMA-API: Mapped at:
>       dma_alloc_attrs+0xa4/0x130
>       dspi_probe+0x568/0x7b4
>       platform_drv_probe+0x6c/0xa4
>       really_probe+0x208/0x348
>       driver_probe_device+0x5c/0xb4
> 
> Fixes: 90ba37033cb9 ("spi: spi-fsl-dspi: Add DMA support for Vybrid")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzk@...nel.org>
> ---

Sounds reasonable. I always wondered why err_slave_config and 
err_rx_dma_buf call something different than dspi_release_dma, but I 
didn't see any problem so I didn't investigate it further.

Acked-by: Vladimir Oltean <vladimir.oltean@....com>

>   drivers/spi/spi-fsl-dspi.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index a35faced0456..58190c94561f 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -588,14 +588,14 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
>                  return;
> 
>          if (dma->chan_tx) {
> -               dma_unmap_single(dma->chan_tx->device->dev, dma->tx_dma_phys,
> -                                dma_bufsize, DMA_TO_DEVICE);
> +               dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
> +                                 dma->tx_dma_buf, dma->tx_dma_phys);
>                  dma_release_channel(dma->chan_tx);
>          }
> 
>          if (dma->chan_rx) {
> -               dma_unmap_single(dma->chan_rx->device->dev, dma->rx_dma_phys,
> -                                dma_bufsize, DMA_FROM_DEVICE);
> +               dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
> +                                 dma->rx_dma_buf, dma->rx_dma_phys);
>                  dma_release_channel(dma->chan_rx);
>          }
>   }
> --
> 2.7.4
> 
> 

Thanks!
-Vladimir