netdev - Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device

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]

Message-ID: <20191119113336.GA25152@apalos.home>
Date:   Tue, 19 Nov 2019 13:33:36 +0200
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, lorenzo.bianconi@...hat.com,
        mcroce@...hat.com, jonathan.lemon@...il.com
Subject: Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to
 sync DMA memory for device

> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index dfc2501c35d9..4f9aed7bce5a 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -47,6 +47,13 @@ static int page_pool_init(struct page_pool *pool,
> >  	    (pool->p.dma_dir != DMA_BIDIRECTIONAL))
> >  		return -EINVAL;
> >  
> > +	/* In order to request DMA-sync-for-device the page needs to
> > +	 * be mapped
> > +	 */
> > +	if ((pool->p.flags & PP_FLAG_DMA_SYNC_DEV) &&
> > +	    !(pool->p.flags & PP_FLAG_DMA_MAP))
> > +		return -EINVAL;
> > +
> 
> I like that you have moved this check to setup time.
> 
> There are two other parameters the DMA_SYNC_DEV depend on:
> 
>  	struct page_pool_params pp_params = {
>  		.order = 0,
> -		.flags = PP_FLAG_DMA_MAP,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
>  		.pool_size = size,
>  		.nid = cpu_to_node(0),
>  		.dev = pp->dev->dev.parent,
>  		.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE,
> +		.offset = pp->rx_offset_correction,
> +		.max_len = MVNETA_MAX_RX_BUF_SIZE,
>  	};
> 
> Can you add a check, that .max_len must not be zero.  The reason is
> that I can easily see people misconfiguring this.  And the effect is
> that the DMA-sync-for-device is essentially disabled, without user
> realizing this. The not-realizing part is really bad, especially
> because bugs that can occur from this are very rare and hard to catch.

+1 we sync based on the min() value of those 

> 
> I'm up for discussing if there should be a similar check for .offset.
> IMHO we should also check .offset is configured, and then be open to
> remove this check once a driver user want to use offset=0.  Does the
> mvneta driver already have a use-case for this (in non-XDP mode)?

Not sure about this, since it does not break anything apart from some
performance hit

Cheers
/Ilias
> 
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>