netdev - Re: [net-next PATCH 2/2] net: page_pool: don't use page->private to store dma_addr

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:   Mon, 11 Feb 2019 11:31:13 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Netdev <netdev@...r.kernel.org>, linux-mm <linux-mm@...ck.org>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Matthew Wilcox <willy@...radead.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        "David S. Miller" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [net-next PATCH 2/2] net: page_pool: don't use page->private to
 store dma_addr_t

On Mon, Feb 11, 2019 at 8:07 AM Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
>
> From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
>
> As pointed out by David Miller the current page_pool implementation
> stores dma_addr_t in page->private.
> This won't work on 32-bit platforms with 64-bit DMA addresses since the
> page->private is an unsigned long and the dma_addr_t a u64.
>
> A previous patch is adding dma_addr_t on struct page to accommodate this.
> This patch adapts the page_pool related functions to use the newly added
> struct for storing and retrieving DMA addresses from network drivers.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@...aro.org>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  net/core/page_pool.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 43a932cb609b..897a69a1477e 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -136,7 +136,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>         if (!(pool->p.flags & PP_FLAG_DMA_MAP))
>                 goto skip_dma_map;
>
> -       /* Setup DMA mapping: use page->private for DMA-addr
> +       /* Setup DMA mapping: use 'struct page' area for storing DMA-addr
> +        * since dma_addr_t can be either 32 or 64 bits and does not always fit
> +        * into page private data (i.e 32bit cpu with 64bit DMA caps)
>          * This mapping is kept for lifetime of page, until leaving pool.
>          */
>         dma = dma_map_page(pool->p.dev, page, 0,
> @@ -146,7 +148,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>                 put_page(page);
>                 return NULL;
>         }
> -       set_page_private(page, dma); /* page->private = dma; */
> +       page->dma_addr = dma;
>
>  skip_dma_map:
>         /* When page just alloc'ed is should/must have refcnt 1. */
> @@ -175,13 +177,16 @@ EXPORT_SYMBOL(page_pool_alloc_pages);
>  static void __page_pool_clean_page(struct page_pool *pool,
>                                    struct page *page)
>  {
> +       dma_addr_t dma;
> +
>         if (!(pool->p.flags & PP_FLAG_DMA_MAP))
>                 return;
>
> +       dma = page->dma_addr;
>         /* DMA unmap */
> -       dma_unmap_page(pool->p.dev, page_private(page),
> +       dma_unmap_page(pool->p.dev, dma,
>                        PAGE_SIZE << pool->p.order, pool->p.dma_dir);
> -       set_page_private(page, 0);
> +       page->dma_addr = 0;
>  }
>
>  /* Return a page to the page allocator, cleaning up our state */

This comment is unrelated to this patch specifically, but applies more
generally to the page_pool use of dma_unmap_page.

So just looking at this I am pretty sure the use of just
dma_unmap_page isn't correct here. You should probably be using
dma_unmap_page_attrs and specifically be passing the attribute
DMA_ATTR_SKIP_CPU_SYNC so that you can tear down the mapping without
invalidating the contents of the page.

This is something that will work for most cases but if you run into a
case where this is used with SWIOTLB in bounce buffer mode you would
end up potentially corrupting data on the unmap call.