linux-kernel - Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow

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, 24 Jun 2019 15:50:07 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Yuehaibing <yuehaibing@...wei.com>, davem@...emloft.net,
        sdf@...gle.com, jianbol@...lanox.com, jiri@...lanox.com,
        mirq-linux@...e.qmqm.pl, willemb@...gle.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] flow_dissector: Fix vlan header offset in
 __skb_flow_dissect

Fri, Jun 21, 2019 at 02:33:17AM CEST, sdf@...ichev.me wrote:
>On 06/20, Yuehaibing wrote:
>> On 2019/6/20 2:39, Stanislav Fomichev wrote:
>> > On 06/20, YueHaibing wrote:
>> >> We build vlan on top of bonding interface, which vlan offload
>> >> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> >> BOND_XMIT_POLICY_ENCAP34.
>> >>
>> >> __skb_flow_dissect() fails to get information from protocol headers
>> >> encapsulated within vlan, because 'nhoff' is points to IP header,
>> >> so bond hashing is based on layer 2 info, which fails to distribute
>> >> packets across slaves.
>> >>
>> >> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
>> >> Signed-off-by: YueHaibing <yuehaibing@...wei.com>
>> >> ---
>> >>  net/core/flow_dissector.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> >> index 415b95f..2a52abb 100644
>> >> --- a/net/core/flow_dissector.c
>> >> +++ b/net/core/flow_dissector.c
>> >> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >>  		    skb && skb_vlan_tag_present(skb)) {
>> >>  			proto = skb->protocol;
>> >>  		} else {
>> >> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
>> >> +				nhoff -=  sizeof(*vlan);
>> >> +
>> > Should we instead fix the place where the skb is allocated to properly
>> > pull vlan (skb_vlan_untag)? I'm not sure this particular place is
>> > supposed to work with an skb. Having an skb with nhoff pointing to
>> > IP header but missing skb_vlan_tag_present() when with
>> > proto==ETH_P_8021xx seems weird.
>> 
>> The skb is a forwarded vxlan packet, it send through vlan interface like this:
>> 
>>    vlan_dev_hard_start_xmit
>>     --> __vlan_hwaccel_put_tag //vlan_tci and VLAN_TAG_PRESENT is set
>>     --> dev_queue_xmit
>>         --> validate_xmit_skb
>>           --> validate_xmit_vlan // vlan_hw_offload_capable is false
>>              --> __vlan_hwaccel_push_inside //here skb_push vlan_hlen, then clear skb->tci
>> 
>>     --> bond_start_xmit
>>        --> bond_xmit_hash
>>          --> __skb_flow_dissect // nhoff point to IP header
>>             -->  case htons(ETH_P_8021Q)
>>             // skb_vlan_tag_present is false, so
>>               vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), //vlan point to ip header wrongly
>I see, so bonding device propagates hw VLAN support from the slaves.
>If one of the slaves doesn't have it, its disabled for the bond device.
>Any idea why we do that? Why not pass skbs to the slave devices
>instead and let them handle the hw/sw vlan implementation?

Probably due to historical reasons. It is indeed not needed to push the
vlan header. We should rather have the vlan_tci filled all the way down
to the transmitting netdevice. So the bonding/team should have the
NETIF_F_HW_VLAN_CTAG_TX and NETIF_F_HW_VLAN_STAG_TX flags always on.
That seems to be the correct fix to me.


>I see the propagation was added in 278339a42a1b 10 years ago and
>I don't see any rationale in the commit description.
>Somebody with more context should probably chime in :-)