linux-kernel - Re: [PATCH] dma-debug: Remove warning in dma_debug

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 1 Aug 2022 13:07:10 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     yf.wang@...iatek.com, Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        "open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>,
        open list <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>
Cc:     wsd_upstream@...iatek.com, Libo Kang <Libo.Kang@...iatek.com>,
        Ning Li <Ning.Li@...iatek.com>, Yong Wu <Yong.Wu@...iatek.com>,
        Miles Chen <miles.chen@...iatek.com>
Subject: Re: [PATCH] dma-debug: Remove warning in dma_debug_entry

On 2022-07-30 09:56, yf.wang@...iatek.com wrote:
> From: Yunfei Wang <yf.wang@...iatek.com>
> 
> The same physical address can be mapped multiple times, and it is not
> required to have attributes such as DMA_ATTR_SKIP_CPU_SYNC, but
> dma_debug_entry will report an warning if active_cacheline_insert
> returns -EEXIST, which can cause a lot of trouble.
> 
> For example, if two dma engine share the same dma-buf, the two dma
> engine drivers will call the DMA API to map their own dma address
> respectively, which is normal follow, but dma_debug_entry will
> report an warning.
> 
> In addition, if active_cacheline_insert returns -EEXIST, the
> active_cacheline_inc_overlap function already has the logic to
> check overlapping mappings counter, if it exceeded
> ACTIVE_CACHELINE_MAX_OVERLAP overlapping mappings, it already
> has a warning.
> And also, if warning as panic is turned on, it will cause KE.
> 
> So, dma_debug_entry does not need report warning.

I'm sure this has come up before, but I can't remember enough of the 
context to easily search for the previous thread.

In short, this check is not entirely correct, but removing it isn't 
right either, since that would be too much of a step backwards in terms 
of debug coverage IMO. What we need here is to do a bit more work to 
differentiate between mappings of the exact same buffer, which are 
allowed, vs. any other kind of partial overlaps which are still illegal 
and definitely deserve a warning.

Furthermore, even for the valid dma-buf import case I'm not convinced 
that the SKIP_CPU_SYNC check can be dropped either. If one device can 
import a buffer while another device is already accessing it, there are 
definitely combinations of parameters which could lead to potential data 
loss (e.g. a non-coherent DMA_TO_DEVICE mapping during a non-coherent 
DMA_FROM_DEVICE access, a or non-coherent DMA_FROM_DEVICE mapping during 
a coherent DMA_TO_DEVICE access).

Thanks,
Robin.

> Signed-off-by: Yunfei Wang <yf.wang@...iatek.com>
> ---
>   kernel/dma/debug.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 2caafd13f8aa..ad731f7858c9 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -566,9 +566,6 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)
>   	if (rc == -ENOMEM) {
>   		pr_err_once("cacheline tracking ENOMEM, dma-debug disabled\n");
>   		global_disable = true;
> -	} else if (rc == -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
> -		err_printk(entry->dev, entry,
> -			"cacheline tracking EEXIST, overlapping mappings aren't supported\n");
>   	}
>   }
>