netdev - Re: [net RFC] net/mlx4_en: Use frag stride in crossing page boundary condition

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:   Thu, 14 Jun 2018 18:56:16 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "kafai@...com" <kafai@...com>, Tariq Toukan <tariqt@...lanox.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>
Subject: Re: [net RFC] net/mlx4_en: Use frag stride in crossing page boundary
 condition

On Wed, 2018-06-13 at 19:30 -0700, Eric Dumazet wrote:
> 
> On 06/13/2018 05:53 PM, Saeed Mahameed wrote:
> > When a new rx packet arrives, the rx path will decide whether to
> > reuse
> > the same page or not according to the current rx frag page offset
> > and
> > frag size, i.e:
> > release = frags->page_offset + frag_info->frag_size > PAGE_SIZE;
> > 
> > Martin debugged this and he showed that this can cause mlx4 XDP to
> > reuse
> > buffers when it shouldn't.
> > 
> > Using frag_info->frag_size is wrong since frag size is always the
> > port
> > mtu and the frag stride might be larger, especially in XDP case
> > where
> > frag_stride == PAGE_SIZE.
> 
> Hmmm... why frag_size is not PAGE_SIZE for XDP then ?
> 

I thought about this, but i see some problems with this that i would
like to avoid.

1. definition of frag_size v.s. frag_stride:
frag_size should be equal to the effective mtu, since the descriptor
frag size depends solely on it. and we don't want to end up receiving
packets larger than the stack mtu (think of a vf with smaller mtu than
the wire).
e.g:
rx_desc->data[i].byte_count = 
       cpu_to_be32(priv->frag_info[i].frag_size);

frag_stride defines how much the frag_size can safely grow inside of a
page or how much padding we need for this frag.

so yes frag_size = PAGE_SIZE can work but it also kind of weird and
might cause incorrectness in terms of RX packets sizes.


> This patch is a bit strange, since we do :
> 
> u32 sz_align = ALIGN(frag_size, SMP_CACHE_BYTES);
> 
> frags->page_offset += sz_align;
> release = frags->page_offset + frag_info->frag_size > PAGE_SIZE;
> 
> So the @release condition is really to have enough space from frags-
> >page_offset
> to hold up to frag_info->frag_size bytes.
> 
> (NIC wont push more than frag_info->frag_size bytes, regardless of
> how big is out frag_stride )
> 

yes, but if frag_size > mtu, we might have a problem.

> Your patch would prevent a page being reused if say the amount of
> available bytes is 1536,
> but frag_stride = 2048
> 

Interestingly for this exact frag_stride we don't have an issue :)
since it goes through a different condition branch
(the page flipping thing):

if (frag_info->frag_stride == PAGE_SIZE / 2) {
    frags->page_offset ^= PAGE_SIZE / 2;
	release = page_count(page) != 1 ||
		  page_is_pfmemalloc(page) ||
		  page_to_nid(page) != numa_mem_id();

but yes there is a small price to pay for when 1536 < mtu < 2k

but for other cases there is no change in behavior.

overall i am not against your suggestion i am just laying off the
problems with the two suggestions.

> 
> I would suggest a patch like the following instead.
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index
> 5edd0cf2999cbde37b3404aafd700584696af940..83d6b17b102bc9a22bfd8e68863
> d079f38670501 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -1082,7 +1082,7 @@ void mlx4_en_calc_rx_buf(struct net_device
> *dev)
>          * This only works when num_frags == 1.
>          */
>         if (priv->tx_ring_num[TX_XDP]) {
> -               priv->frag_info[0].frag_size = eff_mtu;
> +               priv->frag_info[0].frag_size = PAGE_SIZE;
>                 /* This will gain efficient xdp frame recycling at
> the
>                  * expense of more costly truesize accounting
>                  */
>