netdev - Re: [PATCH net-next 1/2] net: Header length compution function

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: <1406703644.3178.30.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Wed, 30 Jul 2014 09:00:44 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	amirv@...lanox.com, alexander.h.duyck@...el.com,
	netdev@...r.kernel.org, ogerlitz@...lanox.com,
	yevgenyp@...lanox.com, idos@...lanox.com
Subject: Re: [PATCH net-next 1/2] net: Header length compution function

On Tue, 2014-07-29 at 21:58 -0700, David Miller wrote:
> From: Amir Vadai <amirv@...lanox.com>
> Date: Mon, 28 Jul 2014 13:14:10 +0300
> 
> > From: Eric Dumazet <eric.dumazet@...il.com>
> > 
> > This commit is based on Eric Dumazet suggestion.
> > Use flow dissector to calculate header length.
> > Tested the following with a mlx4, and it indeed speeds up GRE traffic,
> > as GRO packets can now contain 17 MSS instead of 8.
> > (Pulling payload means GRO had to use 2 'frags' per MSS)
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> > Signed-off-by: Amir Vadai <amirv@...lanox.com>
> 
> So I decided to see how bad it would be if we tried to avoid making
> that funky on-stack SKB and came up with the patch below.
> 
> With this you can make __skb_get_poff() take "data" and "hlen" too,
> pass it onward to __skb_flow_dissect(), and then your new function is
> just:
> 
> u32 eth_frame_headlen(void *data, unsigned int len)
> {
> 	if (unlikely(len < ETH_HLEN))
> 		return len;
> 	return __skb_get_poff(NULL, data, hlen) + ETH_HLEN;
> }

Yep, this was basically what I got when I tried it as well, and I was
not convinced it was the way to go.

This adds quite large number of conditional jumps, as
skb_header_pointer() is heavily used in the stack.

If we want to restrict flow dissection to a 'fake linear skb',
needed fields from this fake skb are well known :

skb->data,		(data)
skb->len,	        (len)
skb->data_len		(0)   // this is a fake linear skb ...

Then skb_flow_dissect() needs 2 additional parts, because it assumed
ethernet header was already pulled (from eth_type_trans())

skb->network_header     (ETH_HLEN)
skb->protocol		(eth->h_proto)

This solution, admittedly a bit hacky, do not add more complexity into
fast path.

The last 2 fields (network_header and protocol) could even be passed as
__skb_flow_dissect() parameters.

By nature, skb_flow_dissect() should not access any information outside
of the frame itself.

Apparently, Alexander never trusted this core function and decided to
implement its own limited flow dissector in igb/ixgbe...

skb layout regarding how linear data is attached wont change anytime
soon.

We certainly can add a fat comment, but then any code using skb->data &
skb->len should get a fat comment as well.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html