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  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, 18 Aug 2016 08:46:38 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Hadar Hen Zion <hadarh@....mellanox.co.il>
Cc:	Hadar Hen Zion <hadarh@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...lanox.com>,
	Amir Vadai <amirva@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net-next V2 1/5] flow_dissector: For stripped vlan, get
 vlan info from skb->vlan_tci

Thu, Aug 18, 2016 at 08:22:49AM CEST, hadarh@....mellanox.co.il wrote:
>On Wed, Aug 17, 2016 at 4:02 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Wed, Aug 17, 2016 at 01:05:33PM CEST, hadarh@....mellanox.co.il wrote:
>>>On Wed, Aug 17, 2016 at 1:46 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>>>> Wed, Aug 17, 2016 at 12:36:10PM CEST, hadarh@...lanox.com wrote:
>>>>>Early in the datapath skb_vlan_untag function is called, stripped
>>>>>the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields.
>>>>>
>>>>>The current dissection doesn't handle stripped vlan packets correctly.
>>>>>In some flows, vlan doesn't exist in skb->data anymore when applying
>>>>>flow dissection on the skb, fix that.
>>>>>
>>>>>In case vlan info wasn't stripped before applying flow_dissector (RPS
>>>>>flow for example), or in case of skb with multiple vlans (e.g. 802.1ad),
>>>>>get the vlan info from skb->data. The flow_dissector correctly skips
>>>>>any number of vlans and stores only the first level vlan.
>>>>>
>>>>>Fixes: 0744dd00c1b1 ('net: introduce skb_flow_dissect()')
>>>>>Signed-off-by: Hadar Hen Zion <hadarh@...lanox.com>
>>>>>---
>>>>> net/core/flow_dissector.c | 34 ++++++++++++++++++++++++++--------
>>>>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>>>>
>>>>>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>>>index 91028ae..362d693 100644
>>>>>--- a/net/core/flow_dissector.c
>>>>>+++ b/net/core/flow_dissector.c
>>>>>@@ -119,12 +119,14 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>>>       struct flow_dissector_key_ports *key_ports;
>>>>>       struct flow_dissector_key_tags *key_tags;
>>>>>       struct flow_dissector_key_keyid *key_keyid;
>>>>>+      bool skip_vlan = false;
>>>>>       u8 ip_proto = 0;
>>>>>       bool ret = false;
>>>>>
>>>>>       if (!data) {
>>>>>               data = skb->data;
>>>>>-              proto = skb->protocol;
>>>>>+              proto = skb_vlan_tag_present(skb) ?
>>>>>+                       skb->vlan_proto : skb->protocol;
>>>>>               nhoff = skb_network_offset(skb);
>>>>>               hlen = skb_headlen(skb);
>>>>>       }
>>>>>@@ -243,23 +245,39 @@ ipv6:
>>>>>       case htons(ETH_P_8021AD):
>>>>>       case htons(ETH_P_8021Q): {
>>>>>               const struct vlan_hdr *vlan;
>>>>>-              struct vlan_hdr _vlan;
>>>>>
>>>>>-              vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), data, hlen, &_vlan);
>>>>>-              if (!vlan)
>>>>>-                      goto out_bad;
>>>>>+              if (skb_vlan_tag_present(skb))
>>>>>+                      proto = skb->protocol;
>>>>>+
>>>>>+              if (!skb_vlan_tag_present(skb) ||
>>>>>+                  proto == cpu_to_be16(ETH_P_8021Q) ||
>>>>>+                  proto == cpu_to_be16(ETH_P_8021AD)) {
>>>>
>>>> How this can happen? Could you give me an example?
>>>>
>>>
>>>This can happen in 2 cases:
>>>
>>>1. vlan wasn't stripped yet from the skb.
>>>In RPS flow for example, get_rps_cpu function is using flow-dissector
>>>before vlan_untag is called by __netif_receive_skb_core.
>>
>> right, sigh...
>>
>>
>>>
>>>2. skb with multiple vlan tags.
>>>Only the first vlan is stripped while the inner vlans are still in skb->data.
>>>In this case skb->vlan_proto is 802.1AD and skb->protocol is 802.1Q
>>>(for example) so I have to take the next header from skb->data.
>>
>> Hmm I think that whoever removes the outermost vlan from skb->vlan_*
>> should strip the next header into skb->vlan_*
>
>The outermost vlan isn't removed from skb->vlan_* it stays there.

No I didn't mean in your code. But other code that removes it should
ensure that next vlan header is stripped so the rest of the code (like
yours) can count with it.

Powered by blists - more mailing lists