Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 24 Jun 2019 15:50:07 +0200
From:   Jiri Pirko <jiri@resnulli.us>
To:     Stanislav Fomichev <sdf@fomichev.me>
Cc:     Yuehaibing <yuehaibing@huawei.com>, davem@davemloft.net,
        sdf@google.com, jianbol@mellanox.com, jiri@mellanox.com,
        mirq-linux@rere.qmqm.pl, willemb@google.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH] flow_dissector: Fix vlan header offset in
 __skb_flow_dissect
Message-ID: <20190624135007.GA17673@nanopsycho>
References: <20190619160132.38416-1-yuehaibing@huawei.com>
 <20190619183938.GA19111@mini-arch>
 <00a5d09f-a23e-661f-60c0-75fba6227451@huawei.com>
 <20190621003317.GE1383@mini-arch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190621003317.GE1383@mini-arch>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Fri, Jun 21, 2019 at 02:33:17AM CEST, sdf@fomichev.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@huawei.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 :-)