[ovs-dev,2/5] OVS: remove use of VLAN_TAG_PRESENT
diff mbox series

Message ID 1560210191-9414-3-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series
  • datapath: Support 5.0.x kernel version
Related show

Commit Message

Yifeng Sun June 10, 2019, 11:43 p.m. UTC
From: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Upstream commit:
    commit 9df46aefafa6dee81a27c2a9d8ba360abd8c5fe3
    Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>
    Date:   Thu Nov 8 18:44:50 2018 +0100

    OVS: remove use of VLAN_TAG_PRESENT

    This is a minimal change to allow removing of VLAN_TAG_PRESENT.
    It leaves OVS unable to use CFI bit, as fixing this would need
    a deeper surgery involving userspace interface.

    Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
    Signed-off-by: David S. Miller <davem@davemloft.net>

This patch backports the above upstream patch to OVS and adds
extra checking in kernel module's compat code.

Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
---
 acinclude.m4                                  |  4 ++++
 datapath/actions.c                            | 13 +++++++++----
 datapath/flow.c                               |  6 +++---
 datapath/flow.h                               |  2 +-
 datapath/flow_netlink.c                       | 22 +++++++++++-----------
 datapath/linux/compat/include/linux/if_vlan.h | 19 +++++++++++++++++++
 6 files changed, 47 insertions(+), 19 deletions(-)

Comments

0-day Robot June 11, 2019, 12:05 a.m. UTC | #1
Bleep bloop.  Greetings Yifeng Sun, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Michał Mirosław <mirq-linux@rere.qmqm.pl> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Yifeng Sun <pkusunyifeng@gmail.com>
Lines checked: 254, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Yi-Hung Wei June 12, 2019, 9:19 p.m. UTC | #2
On Mon, Jun 10, 2019 at 4:44 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote:
>
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Upstream commit:
>     commit 9df46aefafa6dee81a27c2a9d8ba360abd8c5fe3
>     Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>     Date:   Thu Nov 8 18:44:50 2018 +0100
>
>     OVS: remove use of VLAN_TAG_PRESENT
>
>     This is a minimal change to allow removing of VLAN_TAG_PRESENT.
>     It leaves OVS unable to use CFI bit, as fixing this would need
>     a deeper surgery involving userspace interface.
>
>     Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>     Signed-off-by: David S. Miller <davem@davemloft.net>

This backport actually contains the other upstream patch 6083e28aa02d
("OVS: remove VLAN_TAG_PRESENT - fixup"). Can we update the commit
message for reference?

Other than that, this patch LGTM. Thanks Yifeng!

Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>

> --- a/datapath/linux/compat/include/linux/if_vlan.h
> +++ b/datapath/linux/compat/include/linux/if_vlan.h
> @@ -53,6 +53,25 @@ static inline struct sk_buff *rpl_vlan_insert_tag_set_proto(struct sk_buff *skb,
>  }
>  #endif
>
> +#ifndef HAVE_VLAN_HWACCEL_CLEAR_TAG
> +/**
> + * __vlan_hwaccel_clear_tag - clear hardware accelerated VLAN info
> + * @skb: skbuff to clear
> + *
> + * Clears the VLAN information from @skb
> + */
> +#define __vlan_hwaccel_clear_tag rpl_vlan_hwaccel_clear_tag
> +static inline void rpl_vlan_hwaccel_clear_tag(struct sk_buff *skb)
> +{
> +#ifdef HAVE_SKBUFF_VLAN_PRESENT
> +        skb->vlan_present = 0;
This is very minor. In datapath code, we do not expand tab to be
consisent with the other kernel code. Can we replace eight space to a
tab here?



> +#else
> +       skb->vlan_tci = 0;
> +       skb->vlan_proto = 0;
> +#endif
> +}
> +#endif
> +
>  #ifndef HAVE_VLAN_HWACCEL_PUSH_INSIDE
>
>  /*

Patch
diff mbox series

diff --git a/acinclude.m4 b/acinclude.m4
index 6783512a68f3..eb978e0fae6c 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -511,6 +511,7 @@  AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/linux/if_link.h], [rtnl_link_stats64])
   OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [vlan_set_encap_proto])
   OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [vlan_hwaccel_push_inside])
+  OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [__vlan_hwaccel_clear_tag])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/in.h], [ipv4_is_multicast])
   OVS_GREP_IFELSE([$KSRC/include/linux/in.h], [proto_ports_offset])
@@ -925,6 +926,9 @@  AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/skbuff.h], [sk_buff],
                         [csum_valid],
                         [OVS_DEFINE([HAVE_SKBUFF_CSUM_VALID])])
+  OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/skbuff.h], [sk_buff],
+                        [vlan_present],
+                        [OVS_DEFINE([HAVE_SKBUFF_VLAN_PRESENT])])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h],
                   [skb_checksum_simple_validate])
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h],
diff --git a/datapath/actions.c b/datapath/actions.c
index f5db12389889..5a1d3206a101 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -306,7 +306,7 @@  static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
 		key->eth.vlan.tpid = vlan->vlan_tpid;
 	}
 	return skb_vlan_push(skb, vlan->vlan_tpid,
-			     ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
+			     ntohs(vlan->vlan_tci) & ~VLAN_CFI_MASK);
 }
 
 /* 'src' is already properly masked. */
@@ -828,8 +828,10 @@  static int ovs_vport_output(OVS_VPORT_OUTPUT_PARAMS)
 	__skb_dst_copy(skb, data->dst);
 	*OVS_GSO_CB(skb) = data->cb;
 	ovs_skb_set_inner_protocol(skb, data->inner_protocol);
-	skb->vlan_tci = data->vlan_tci;
-	skb->vlan_proto = data->vlan_proto;
+	if (data->vlan_tci & VLAN_CFI_MASK)
+		__vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci & ~VLAN_CFI_MASK);
+	else
+		__vlan_hwaccel_clear_tag(skb);
 
 	/* Reconstruct the MAC header.  */
 	skb_push(skb, data->l2_len);
@@ -873,7 +875,10 @@  static void prepare_frag(struct vport *vport, struct sk_buff *skb,
 	data->cb = *OVS_GSO_CB(skb);
 	data->inner_protocol = ovs_skb_get_inner_protocol(skb);
 	data->network_offset = orig_network_offset;
-	data->vlan_tci = skb->vlan_tci;
+	if (skb_vlan_tag_present(skb))
+		data->vlan_tci = skb_vlan_tag_get(skb) | VLAN_CFI_MASK;
+	else
+		data->vlan_tci = 0;
 	data->vlan_proto = skb->vlan_proto;
 	data->mac_proto = mac_proto;
 	data->l2_len = hlen;
diff --git a/datapath/flow.c b/datapath/flow.c
index f6a95f9fd86c..618c25e0c3ad 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -327,7 +327,7 @@  static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh,
 		return -ENOMEM;
 
 	vh = (struct vlan_head *)skb->data;
-	key_vh->tci = vh->tci | htons(VLAN_TAG_PRESENT);
+	key_vh->tci = vh->tci | htons(VLAN_CFI_MASK);
 	key_vh->tpid = vh->tpid;
 
 	if (unlikely(untag_vlan)) {
@@ -365,7 +365,7 @@  static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 	key->eth.cvlan.tpid = 0;
 
 	if (skb_vlan_tag_present(skb)) {
-		key->eth.vlan.tci = htons(skb->vlan_tci);
+		key->eth.vlan.tci = htons(skb->vlan_tci) | htons(VLAN_CFI_MASK);
 		key->eth.vlan.tpid = skb->vlan_proto;
 	} else {
 		/* Parse outer vlan tag in the non-accelerated case. */
@@ -604,7 +604,7 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 		 * skb_vlan_pop(), which will later shift the ethertype into
 		 * skb->protocol.
 		 */
-		if (key->eth.cvlan.tci & htons(VLAN_TAG_PRESENT))
+		if (key->eth.cvlan.tci & htons(VLAN_CFI_MASK))
 			skb->protocol = key->eth.cvlan.tpid;
 		else
 			skb->protocol = key->eth.type;
diff --git a/datapath/flow.h b/datapath/flow.h
index 87e212fdfd9f..4884c006c786 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -59,7 +59,7 @@  struct ovs_tunnel_info {
 
 struct vlan_head {
 	__be16 tpid; /* Vlan type. Generally 802.1q or 802.1ad.*/
-	__be16 tci;  /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
+	__be16 tci;  /* 0 if no VLAN, VLAN_CFI_MASK set otherwise. */
 };
 
 #define OVS_SW_FLOW_KEY_METADATA_SIZE			\
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index c4704a5b3041..0f7ab53fc141 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -993,9 +993,9 @@  static int validate_vlan_from_nlattrs(const struct sw_flow_match *match,
 	if (a[OVS_KEY_ATTR_VLAN])
 		tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
 
-	if (!(tci & htons(VLAN_TAG_PRESENT))) {
+	if (!(tci & htons(VLAN_CFI_MASK))) {
 		if (tci) {
-			OVS_NLERR(log, "%s TCI does not have VLAN_TAG_PRESENT bit set.",
+			OVS_NLERR(log, "%s TCI does not have VLAN_CFI_MASK bit set.",
 				  (inner) ? "C-VLAN" : "VLAN");
 			return -EINVAL;
 		} else if (nla_len(a[OVS_KEY_ATTR_ENCAP])) {
@@ -1016,9 +1016,9 @@  static int validate_vlan_mask_from_nlattrs(const struct sw_flow_match *match,
 	__be16 tci = 0;
 	__be16 tpid = 0;
 	bool encap_valid = !!(match->key->eth.vlan.tci &
-			      htons(VLAN_TAG_PRESENT));
+			      htons(VLAN_CFI_MASK));
 	bool i_encap_valid = !!(match->key->eth.cvlan.tci &
-				htons(VLAN_TAG_PRESENT));
+				htons(VLAN_CFI_MASK));
 
 	if (!(key_attrs & (1 << OVS_KEY_ATTR_ENCAP))) {
 		/* Not a VLAN. */
@@ -1042,8 +1042,8 @@  static int validate_vlan_mask_from_nlattrs(const struct sw_flow_match *match,
 			  (inner) ? "C-VLAN" : "VLAN", ntohs(tpid));
 		return -EINVAL;
 	}
-	if (!(tci & htons(VLAN_TAG_PRESENT))) {
-		OVS_NLERR(log, "%s TCI mask does not have exact match for VLAN_TAG_PRESENT bit.",
+	if (!(tci & htons(VLAN_CFI_MASK))) {
+		OVS_NLERR(log, "%s TCI mask does not have exact match for VLAN_CFI_MASK bit.",
 			  (inner) ? "C-VLAN" : "VLAN");
 		return -EINVAL;
 	}
@@ -1098,7 +1098,7 @@  static int parse_vlan_from_nlattrs(struct sw_flow_match *match,
 	if (err)
 		return err;
 
-	encap_valid = !!(match->key->eth.vlan.tci & htons(VLAN_TAG_PRESENT));
+	encap_valid = !!(match->key->eth.vlan.tci & htons(VLAN_CFI_MASK));
 	if (encap_valid) {
 		err = __parse_vlan_from_nlattrs(match, key_attrs, true, a,
 						is_mask, log);
@@ -3034,7 +3034,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			vlan = nla_data(a);
 			if (!eth_type_vlan(vlan->vlan_tpid))
 				return -EINVAL;
-			if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT)))
+			if (!(vlan->vlan_tci & htons(VLAN_CFI_MASK)))
 				return -EINVAL;
 			vlan_tci = vlan->vlan_tci;
 			break;
@@ -3050,7 +3050,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			/* Prohibit push MPLS other than to a white list
 			 * for packets that have a known tag order.
 			 */
-			if (vlan_tci & htons(VLAN_TAG_PRESENT) ||
+			if (vlan_tci & htons(VLAN_CFI_MASK) ||
 			    (eth_type != htons(ETH_P_IP) &&
 			     eth_type != htons(ETH_P_IPV6) &&
 			     eth_type != htons(ETH_P_ARP) &&
@@ -3062,7 +3062,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 		}
 
 		case OVS_ACTION_ATTR_POP_MPLS:
-			if (vlan_tci & htons(VLAN_TAG_PRESENT) ||
+			if (vlan_tci & htons(VLAN_CFI_MASK) ||
 			    !eth_p_mpls(eth_type))
 				return -EINVAL;
 
@@ -3127,7 +3127,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 		case OVS_ACTION_ATTR_POP_ETH:
 			if (mac_proto != MAC_PROTO_ETHERNET)
 				return -EINVAL;
-			if (vlan_tci & htons(VLAN_TAG_PRESENT))
+			if (vlan_tci & htons(VLAN_CFI_MASK))
 				return -EINVAL;
 			mac_proto = MAC_PROTO_NONE;
 			break;
diff --git a/datapath/linux/compat/include/linux/if_vlan.h b/datapath/linux/compat/include/linux/if_vlan.h
index 2cf18e5fa3f3..4e9cc48500f1 100644
--- a/datapath/linux/compat/include/linux/if_vlan.h
+++ b/datapath/linux/compat/include/linux/if_vlan.h
@@ -53,6 +53,25 @@  static inline struct sk_buff *rpl_vlan_insert_tag_set_proto(struct sk_buff *skb,
 }
 #endif
 
+#ifndef HAVE_VLAN_HWACCEL_CLEAR_TAG
+/**
+ * __vlan_hwaccel_clear_tag - clear hardware accelerated VLAN info
+ * @skb: skbuff to clear
+ *
+ * Clears the VLAN information from @skb
+ */
+#define __vlan_hwaccel_clear_tag rpl_vlan_hwaccel_clear_tag
+static inline void rpl_vlan_hwaccel_clear_tag(struct sk_buff *skb)
+{
+#ifdef HAVE_SKBUFF_VLAN_PRESENT
+        skb->vlan_present = 0;
+#else
+	skb->vlan_tci = 0;
+	skb->vlan_proto = 0;
+#endif
+}
+#endif
+
 #ifndef HAVE_VLAN_HWACCEL_PUSH_INSIDE
 
 /*