linux-kernel - Re: [RFC] remove the ->mapping_error method from dma_map

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:   Fri, 09 Nov 2018 15:12:34 -0800 (PST)
From:   David Miller <davem@...emloft.net>
To:     hch@....de
Cc:     iommu@...ts.linux-foundation.org, torvalds@...ux-foundation.org,
        linux-alpha@...r.kernel.org, linux-arch@...r.kernel.org,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] remove the ->mapping_error method from dma_map_ops

From: Christoph Hellwig <hch@....de>
Date: Fri,  9 Nov 2018 09:46:30 +0100

> Error reporting for the dma_map_single and dma_map_page operations is
> currently a mess.  Both APIs directly return the dma_addr_t to be used for
> the DMA, with a magic error escape that is specific to the instance and
> checked by another method provided.  This has a few downsides:
> 
>  - the error check is easily forgotten and a __must_check marker doesn't
>    help as the value always is consumed anyway
>  - the error checking requires another indirect call, which have gotten
>    incredibly expensive
>  - a lot of code is wasted on implementing these methods
> 
> The historical reason for this is that people thought DMA mappings would
> not fail when the API was created, which sounds like a really bad
> assumption in retrospective, and then we tried to cram error handling
> onto it later on.
> 
> There basically are two variants:  the error code is 0 because the
> implementation will never return 0 as a valid DMA address, or the error
> code is all-F as the implementation won't ever return an address that
> high.  The old AMD GART is the only one not falling into these two camps
> as it picks sort of a relative zero relative to where it is mapped.
> 
> The 0 return doesn't work for a lot of IOMMUs that start allocating
> bus space from address zero, so we can't consolidate on that, but I think
> we can move everyone to all-Fs, which the patch here does.  The reason
> for that is that there is only one way to ever get this address: by
> doing a 1-byte long, 1-byte aligned mapping, but all our mappings
> are not only longer but generally aligned, and the mappings have to
> keep at least the basic alignment.  Please try to poke holes into this
> idea as it would simplify our logic a lot, and also allow to change
> at least the not so commonly used yet dma_map_single_attrs and
> dma_map_page_attrs to actually return an error code and further improve
> the error handling flow in the drivers.

I have no problem with patch #1, it looks great.

But patch #2 on the other hand, not so much.

I hate seeing values returned by reference, it adds cost especially
on cpus where all argments and return values fit in registers (we end
up forcing a stack slot and memory references).

And we don't need it here.

DMA addresses are like pointers, and therefore we can return errors and
valid success values in the same dma_addr_t just fine.  PTR_ERR() --> DMA_ERR(),
IS_PTR_ERR() --> IS_DMA_ERR, etc.