| 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: <SG2PR03MB07969B4C4555CB2E370AE8F3E48F0@SG2PR03MB0796.apcprd03.prod.outlook.com>
Date: Sun, 26 Jul 2015 13:57:15 +0000
From: <ravulakollu.kumar@...ro.com>
To: <thomasfherbert@...il.com>, <pshelar@...ira.com>
CC: <dev@...nvswitch.org>, <netdev@...r.kernel.org>,
<therbert@...hat.com>
Subject: RE: [ovs-dev] [PATCH net-next 3/3] openvswitch: 802.1AD: Flow
handling, actions, and vlan parsing
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).
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
Powered by blists - more mailing lists