| 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
| ||
|
Message-ID: <55B4EFA4.4070804@redhat.com>
Date: Sun, 26 Jul 2015 10:33:08 -0400
From: Thomas F Herbert <therbert@...hat.com>
To: ravulakollu.kumar@...ro.com, thomasfherbert@...il.com,
pshelar@...ira.com
CC: dev@...nvswitch.org, netdev@...r.kernel.org
Subject: Re: [ovs-dev] [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling,
actions, and vlan parsing
On 7/26/15 9:57 AM, ravulakollu.kumar@...ro.com wrote:
> Hi Thomas,
>
> I am currently using the beta version of ovs(2.3.90). In my phy-phy scenario , I am configuring the two physical ports(eth0, eth1) attached to ovs bridge
> as trunk ports using the below commands.
>
> ovs-vsctl --no-wait add-port br0 eth0 vlan_mode=trunk
> ovs-vsctl --no-wait add-port br0 eth1 vlan_mode=trunk
>
> I configured the bridge to work in legacy mode as shown below
>
> $ ovs-ofctl dump-flows br0
> NXST_FLOW reply (xid=0x4):
> cookie=0x0, duration=15.458s, table=0, n_packets=0, n_bytes=0, idle_age=15, priority=0 actions=NORMAL
>
> I tried sending 802.1ad tagged(outer tag tpid=0x88a8, inter tag tpid=ox8100) packet from packetgen to phyport1 (eth0), and receiving back on phyport2(eth2).
> When I captured the received packet on eth1 , the received packet is not same as sent packet, means outer vlan TPID is not 0x88a8(instead 0x8100).
>
This patch supports pushing and popping of double tagged vlans. It
shouldn't affect double tagged traffic flowing through the switch unless
your ovs bridge has flows to pop or push a vlan.
Are you using tcpdump to look at the packets? There is a bug in libpcap
where the outer TPID of outgoing packets is miss-reported as 0x8100
whether or not the tpid is actually 0x88a8
> Could you please let me know , whether the below mentioned patch helps here.
>
> Thanks in Advance,
> Uday
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@...nvswitch.org] On Behalf Of Thomas F Herbert
> Sent: Sunday, July 26, 2015 6:03 AM
> To: Pravin Shelar
> Cc: dev@...nvswitch.org; netdev; therbert@...hat.com
> Subject: Re: [ovs-dev] [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling, actions, and vlan parsing
>
> On 6/30/15 12:16 AM, Pravin Shelar wrote:
>> On Tue, Jun 23, 2015 at 11:26 AM, Thomas F Herbert
> Pravin, I apologize because I realize now that I am finishing V12 of this patch that I never responded to your comments in this email.
>> <thomasfherbert@...il.com> wrote:
>>> Add support for 802.1ad including the ability to push and pop double
>>> tagged vlans. Add support for 802.1ad to netlink parsing and flow
>>> conversion. Uses double nested encap attributes to represent double
>>> tagged vlan. Inner TPID encoded along with ctci in nested attributes.
>>>
>>> Signed-off-by: Thomas F Herbert <thomasfherbert@...il.com>
>>> ---
>>> net/openvswitch/flow.c | 84 +++++++++++++++---
>>> net/openvswitch/flow.h | 5 ++
>>> net/openvswitch/flow_netlink.c | 195 ++++++++++++++++++++++++++++++++++-------
>>> 3 files changed, 242 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index
>>> 2dacc7b..e268865 100644
>>> --- a/net/openvswitch/flow.c
>>> +++ b/net/openvswitch/flow.c
>>> @@ -298,21 +298,80 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>> static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> {
>>> struct qtag_prefix {
>>> - __be16 eth_type; /* ETH_P_8021Q */
>>> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */
>>> __be16 tci;
>>> };
>>> - struct qtag_prefix *qp;
>>> + struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
>>>
>>> - if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
>>> + struct qinqtag_prefix {
>>> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */
>>> + __be16 tci;
>>> + __be16 inner_tpid; /* ETH_P_8021Q */
>>> + __be16 ctci;
>>> + };
>>> +
>>> + if (likely(skb_vlan_tag_present(skb))) {
>>> + key->eth.tci = htons(skb->vlan_tci);
>>> +
>>> + /* Case where upstream
>>> + * processing has already stripped the outer vlan tag.
>>> + */
>>> + if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
>>> + if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> + sizeof(__be16))) {
>>> + key->eth.tci = 0;
>>> + return 0;
>>> + }
>>> +
>>> + if (unlikely(!pskb_may_pull(skb,
>>> + sizeof(struct qtag_prefix) +
>>> + sizeof(__be16)))) {
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
>>> + key->eth.cvlan.ctci =
>>> + qp->tci | htons(VLAN_TAG_PRESENT);
>>> + key->eth.cvlan.c_tpid =
>>> + skb->vlan_proto;
>>
>> We should directly copy qp->inner_tpid here. As you have done it for
>> non offloaded case below.
> Thanks! It is copied but it is set to the wrong tpid. The c_tpid field in the key should be set to the ethertype in the packet itself which is the inner tpid, not the offloaded skb-vlan_proto which is the outer tpid. Fixed in V12.
>>
>>> + __skb_pull(skb, sizeof(struct qtag_prefix));
>>> + }
>>> + }
>>> return 0;
>>> + }
>>>
>>> - if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>> - sizeof(__be16))))
>>> - return -ENOMEM;
>>>
>>> - qp = (struct qtag_prefix *) skb->data;
>>> - key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>> - __skb_pull(skb, sizeof(struct qtag_prefix));
>>> + if (qp->eth_type == htons(ETH_P_8021AD)) {
>>> + struct qinqtag_prefix *qinqp =
>>> + (struct qinqtag_prefix
>>> + *)skb->data;
>>> +
>>> + if (unlikely(skb->len < sizeof(struct qinqtag_prefix) +
>>> + sizeof(__be16)))
>>> + return 0;
>>> +
>>> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qinqtag_prefix) +
>>> + sizeof(__be16)))) {
>>> + return -ENOMEM;
>>> + }
>>> + key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT);
>>> + key->eth.cvlan.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
>>> + key->eth.cvlan.c_tpid = qinqp->inner_tpid;
>>> +
>>> + __skb_pull(skb, sizeof(struct qinqtag_prefix));
>>> +
>>> + return 0;
>>> + }
>>> + if (qp->eth_type == htons(ETH_P_8021Q)) {
>>> + if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> + sizeof(__be16)))
>>> + return -ENOMEM;
>>> +
>>> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>> + sizeof(__be16))))
>>> + return 0;
>>> + key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>> +
>>> + __skb_pull(skb, sizeof(struct qtag_prefix));
>>> + }
>>>
>>> return 0;
>>> }
>>> @@ -474,9 +533,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>>> */
>>>
>>> key->eth.tci = 0;
>>> - if (skb_vlan_tag_present(skb))
>>> - key->eth.tci = htons(skb->vlan_tci);
>>> - else if (eth->h_proto == htons(ETH_P_8021Q))
>>> + key->eth.cvlan.ctci = 0;
>> cvlan.c_tpid is not initialized for all cases.
> Fixed in V12
>>
>>> + if ((skb_vlan_tag_present(skb)) ||
>>> + (eth->h_proto == htons(ETH_P_8021Q)) ||
>>> + (eth->h_proto == htons(ETH_P_8021AD)))
>> These are redundant check. so we can directly call this function.
> I don't agree that these 3 checks are redundant. parse_vlan parses both the offloaded and non-offloaded cases. In V12, I changed it to call
> eth_type_vlan() to do the checks for the non-offloaded ethertypes.
>
> Hmmm ... maybe I should add another function to if_vlan.h to check if a packet is a vlan regardless of whether it is offloaded or not?
>>
>>> if (unlikely(parse_vlan(skb, key)))
>>> return -ENOMEM;
>>>
>>
>> ...
>>>
>>> +static int cust_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>> + const struct nlattr **a, bool is_mask,
>>> + bool log) {
>>> + /* This should be nested inner or "customer" tci" */
>>> + if (attrs & (1 << OVS_KEY_ATTR_VLAN)) {
>>> + __be16 ctci;
>>> +
>>> + ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>>> + if (!(ctci & htons(VLAN_TAG_PRESENT))) {
>>> + if (is_mask)
>>> + OVS_NLERR(log, "VLAN CTCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
>>> + else
>>> + OVS_NLERR(log, "VLAN CTCI does not
>>> + have VLAN_TAG_PRESENT bit set.");
>>> +
>>> + return -EINVAL;
>>> + }
>>> + SW_FLOW_KEY_PUT(match, eth.cvlan.c_tpid, ctci, is_mask);
>>> + SW_FLOW_KEY_PUT(match, eth.cvlan.ctci, ctci, is_mask);
>>> + }
>> Same value is set to tpid and tci.
> Thanks! Fixed in V12.
>>
>>> + return 0;
>>> +}
>>> +
>>> static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>> const struct nlattr **a, bool is_mask,
>>> bool log) @@ -1024,6 +1047,105 @@
>>> static void mask_set_nlattr(struct nlattr *attr, u8 val)
>>> nlattr_set(attr, val, ovs_key_lens);
>>> }
>>>
>>> +static int parse_vlan_from_nlattrs(const struct nlattr *nla,
>>> + struct sw_flow_match *match,
>>> + u64 *key_attrs, bool *ie_valid,
>>> + const struct nlattr **a, bool is_mask,
>>> + bool log) {
>>> + int err;
>>> + __be16 tci;
>>> + const struct nlattr *encap;
>>> +
>>> + if (!is_mask) {
>>> + u64 v_attrs = 0;
>>> +
>>> + tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>>> +
>>> + if (tci & htons(VLAN_TAG_PRESENT)) {
>>> + if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
>>> + htons(ETH_P_8021AD)))) {
>> Since we have added tpid to flow key, we have flexibility of
>> supporting generic double encapsulation. Therefore in netlink parsing
>> of key, double encap should not be limited to just 8021AD. for example
>> it should allow 8021Q in 8021Q header as valid key.
> I agree. Although Open Flow specification doesn't support non 802.1AD double tagged vlans, we probably should be as least restrictive as possible here in the kernel module. I recoded this in V12 to allow a more "generic" qinq.
>>
>>> + err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>>> + if (err)
>>> + return err;
>>> + if (!v_attrs)
>>> + return -EINVAL;
>>> +
>
> _______________________________________________
> dev mailing list
> dev@...nvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
--
Thomas F Herbert Red Hat
--
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
Powered by blists - more mailing lists