[PATCH net-next v2 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing

[PATCH net-next v2 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing

[Minxi Hou]

Extend the ovs-dpctl.py flow parser to support vlan() and encap()
match strings.  vlan() accepts tci=, vid=, pcp=, and cfi=
parameters and generates the OVS_KEY_ATTR_VLAN attribute with a TCI
value in network byte order.  encap() parses nested flow strings
and returns OVS_KEY_ATTR_ENCAP with inner key attributes as a
recursive NLA container.

The encap nla_map type is changed from "none" to "nested" so that
pyroute2 recursively encodes the inner flow key attributes.  The
VLAN nla_map type is changed from "uint16" to "be16" to match the
kernel's big-endian wire format.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
v1 -> v2: rebase to latest net-next/main, drop --base=auto

 .../selftests/net/openvswitch/ovs-dpctl.py    | 190 +++++++++++++++++-
 1 file changed, 188 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 848f61fdcee0..317be7878937 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -901,11 +901,11 @@ class ovskey(nla):
     nla_flags = NLA_F_NESTED
     nla_map = (
         ("OVS_KEY_ATTR_UNSPEC", "none"),
-        ("OVS_KEY_ATTR_ENCAP", "none"),
+        ("OVS_KEY_ATTR_ENCAP", "nested"),
         ("OVS_KEY_ATTR_PRIORITY", "uint32"),
         ("OVS_KEY_ATTR_IN_PORT", "uint32"),
         ("OVS_KEY_ATTR_ETHERNET", "ethaddr"),
-        ("OVS_KEY_ATTR_VLAN", "uint16"),
+        ("OVS_KEY_ATTR_VLAN", "be16"),
         ("OVS_KEY_ATTR_ETHERTYPE", "be16"),
         ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"),
         ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"),
@@ -1636,6 +1636,180 @@ class ovskey(nla):
     class ovs_key_mpls(nla):
         fields = (("lse", ">I"),)
 
+    # 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet
+    _VLAN_CFI_MASK = 0x1000
+    _MAX_ENCAP_DEPTH = 4
+    _encap_depth = 0  # single-threaded usage assumed
+
+    @staticmethod
+    def _parse_vlan_from_flowstr(flowstr):
+        """Parse vlan(tci=X) or vlan(vid=X[,pcp=Y,cfi=Z]) from flowstr.
+
+        Returns (remaining_flowstr, key_tci, mask_tci).
+        TCI values use standard bit layout (VID bits 0-11,
+        CFI bit 12, PCP bits 13-15); byte order conversion to
+        big-endian happens in pyroute2 be16 NLA serialization.
+        The mask covers only the fields the caller specified:
+        vid -> 0x0FFF, pcp -> 0xE000, cfi -> 0x1000, tci -> 0xFFFF.
+
+        The tci= key sets the raw TCI bitfield (no CFI validation) to allow
+        non-Ethernet use cases.  Use cfi=1 for standard Ethernet VLAN matching.
+        """
+        tci = 0
+        mask = 0
+        has_tci = False
+        has_vid = has_pcp = has_cfi = False
+        _tci_mix_err = "vlan(): 'tci' cannot be mixed " \
+                       "with 'vid'/'pcp'/'cfi'"
+        first = True
+        while True:
+            flowstr = flowstr.lstrip()
+            if not flowstr:
+                raise ValueError("vlan(): missing ')'")
+            if flowstr[0] == ')':
+                break
+            if not first:
+                flowstr = flowstr[1:]  # skip ','
+                if not flowstr:
+                    raise ValueError("vlan(): missing ')' after trailing comma")
+                flowstr = flowstr.lstrip()
+                if flowstr and flowstr[0] == ')':
+                    break
+                if flowstr and flowstr[0] == ',':
+                    raise ValueError(
+                        "vlan(): empty or extra comma in field list")
+            first = False
+
+            eq = flowstr.find('=')
+            if eq == -1:
+                raise ValueError("vlan(): expected key=value, got '%s'" % flowstr)
+            key = flowstr[:eq].strip()
+            flowstr = flowstr[eq + 1:]
+
+            end = flowstr.find(',')
+            end2 = flowstr.find(')')
+            if end == -1 or (end2 != -1 and end2 < end):
+                end = end2
+            val = flowstr[:end].strip()
+            flowstr = flowstr[end:]
+
+            if not val:
+                raise ValueError("vlan(): empty value for key '%s'" % key)
+            try:
+                v = int(val, 16) if val.startswith(('0x', '0X')) else int(val)
+            except ValueError:
+                raise ValueError("vlan(): invalid value '%s' for key '%s'" %
+                                 (val, key))
+
+            if key == 'tci':
+                if has_tci:
+                    raise ValueError("vlan(): duplicate 'tci'")
+                if has_vid or has_pcp or has_cfi:
+                    raise ValueError(_tci_mix_err)
+                if v > 0xFFFF or v < 0:
+                    raise ValueError("vlan(): tci=0x%x out of range" % v)
+                tci = v
+                mask = 0xFFFF
+                has_tci = True
+            elif key == 'vid':
+                if has_tci:
+                    raise ValueError(_tci_mix_err)
+                if has_vid:
+                    raise ValueError("vlan(): duplicate 'vid'")
+                if v < 0 or v > 0xFFF:
+                    raise ValueError("vlan(): vid=%d out of range (0-4095)" % v)
+                tci |= v
+                mask |= 0x0FFF
+                has_vid = True
+            elif key == 'pcp':
+                if has_tci:
+                    raise ValueError(_tci_mix_err)
+                if has_pcp:
+                    raise ValueError("vlan(): duplicate 'pcp'")
+                if v < 0 or v > 7:
+                    raise ValueError("vlan(): pcp=%d out of range (0-7)" % v)
+                tci |= (v & 0x7) << 13
+                mask |= 0xE000
+                has_pcp = True
+            elif key == 'cfi':
+                if has_tci:
+                    raise ValueError(_tci_mix_err)
+                if has_cfi:
+                    raise ValueError("vlan(): duplicate 'cfi'")
+                if v != 1:
+                    raise ValueError("vlan(): cfi must be 1 for Ethernet")
+                tci |= ovskey._VLAN_CFI_MASK
+                mask |= ovskey._VLAN_CFI_MASK
+                has_cfi = True
+            else:
+                raise ValueError("vlan(): unknown key '%s'" % key)
+
+        flowstr = flowstr[1:]  # skip ')'
+        # Catch immediate '))' (user error).  A ')' after ',' is consumed
+        # by parse()'s strspn(flowstr, "), ") inter-field separator stripping.
+        if flowstr.lstrip().startswith(')'):
+            raise ValueError("vlan(): unmatched ')'")
+        # parse() strips trailing ',', ')', ' ' as inter-field separators,
+        # so we do not need to call strspn here.
+
+        if mask == 0:
+            raise ValueError("vlan(): no fields specified, "
+                             "use vlan(vid=X[,pcp=Y,cfi=Z]) or vlan(tci=X)")
+        if not has_tci:
+            tci |= ovskey._VLAN_CFI_MASK
+            mask |= ovskey._VLAN_CFI_MASK
+        return flowstr, tci, mask
+
+    @staticmethod
+    def _parse_encap_from_flowstr(flowstr):
+        """Parse encap(inner_flow) from flowstr.
+
+        Returns (remaining_flowstr, inner_key_dict, inner_mask_dict)
+        where each dict has an 'attrs' key for recursive NLA encoding.
+        Parenthesis-depth tracking handles nested encap() calls but not
+        quoted strings containing literal parentheses.
+        """
+        if ovskey._encap_depth >= ovskey._MAX_ENCAP_DEPTH:
+            raise ValueError("encap(): max nesting depth %d exceeded" %
+                             ovskey._MAX_ENCAP_DEPTH)
+        try:
+            ovskey._encap_depth += 1
+            depth = 1
+            end = -1
+            for i, c in enumerate(flowstr):
+                if c == '(':
+                    depth += 1
+                elif c == ')':
+                    depth -= 1
+                    if depth < 0:
+                        raise ValueError("encap(): unmatched ')' at position %d" % i)
+                    if depth == 0:
+                        end = i
+                        break
+
+            if end == -1:
+                if depth > 1:
+                    raise ValueError("encap(): missing ')' at end")
+                raise ValueError("encap(): missing closing ')'")
+
+            inner_str = flowstr[:end].strip()
+            if not inner_str:
+                raise ValueError("encap(): empty inner flow")
+
+            flowstr = flowstr[end + 1:]
+            if flowstr.lstrip().startswith(')'):
+                raise ValueError("encap(): unmatched ')' after encap()")
+            # parse() strips trailing ',', ')', ' ' as inter-field separators,
+            # so we do not need to call strspn here.
+
+            inner_key = ovskey()
+            inner_mask = ovskey()
+            inner_key.parse(inner_str, inner_mask)
+
+            return flowstr, inner_key, inner_mask
+        finally:
+            ovskey._encap_depth -= 1
+
     def parse(self, flowstr, mask=None):
         for field in (
             ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
@@ -1657,6 +1831,16 @@ class ovskey(nla):
                 "eth_type",
                 lambda x: intparse(x, "0xffff"),
             ),
+            (
+                "OVS_KEY_ATTR_VLAN",
+                "vlan",
+                ovskey._parse_vlan_from_flowstr,
+            ),
+            (
+                "OVS_KEY_ATTR_ENCAP",
+                "encap",
+                ovskey._parse_encap_from_flowstr,
+            ),
             (
                 "OVS_KEY_ATTR_IPV4",
                 "ipv4",
@@ -1794,6 +1978,8 @@ class ovskey(nla):
                 True,
             ),
             ("OVS_KEY_ATTR_ETHERNET", None, None, False, False),
+            ("OVS_KEY_ATTR_VLAN", "vlan", "0x%04x", lambda x: False, True),
+            ("OVS_KEY_ATTR_ENCAP", None, None, False, False),
             (
                 "OVS_KEY_ATTR_ETHERTYPE",
                 "eth_type",
-- 
2.53.0

Re: [PATCH net-next v2 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing

[Aaron Conole]

Minxi Hou <houminxi@gmail.com> writes:

> Extend the ovs-dpctl.py flow parser to support vlan() and encap()
> match strings.  vlan() accepts tci=, vid=, pcp=, and cfi=
> parameters and generates the OVS_KEY_ATTR_VLAN attribute with a TCI
> value in network byte order.  encap() parses nested flow strings
> and returns OVS_KEY_ATTR_ENCAP with inner key attributes as a
> recursive NLA container.
>
> The encap nla_map type is changed from "none" to "nested" so that
> pyroute2 recursively encodes the inner flow key attributes.  The
> VLAN nla_map type is changed from "uint16" to "be16" to match the
> kernel's big-endian wire format.

^ That's kindof a fix, BUT it also seems like we never actually used it
anywhere, so I hope it is okay to not treat it that way.

> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---
> v1 -> v2: rebase to latest net-next/main, drop --base=auto
>
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 190 +++++++++++++++++-
>  1 file changed, 188 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 848f61fdcee0..317be7878937 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -901,11 +901,11 @@ class ovskey(nla):
>      nla_flags = NLA_F_NESTED
>      nla_map = (
>          ("OVS_KEY_ATTR_UNSPEC", "none"),
> -        ("OVS_KEY_ATTR_ENCAP", "none"),
> +        ("OVS_KEY_ATTR_ENCAP", "nested"),
>          ("OVS_KEY_ATTR_PRIORITY", "uint32"),
>          ("OVS_KEY_ATTR_IN_PORT", "uint32"),
>          ("OVS_KEY_ATTR_ETHERNET", "ethaddr"),
> -        ("OVS_KEY_ATTR_VLAN", "uint16"),
> +        ("OVS_KEY_ATTR_VLAN", "be16"),
>          ("OVS_KEY_ATTR_ETHERTYPE", "be16"),
>          ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"),
>          ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"),
> @@ -1636,6 +1636,180 @@ class ovskey(nla):
>      class ovs_key_mpls(nla):
>          fields = (("lse", ">I"),)
>  
> +    # 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet
> +    _VLAN_CFI_MASK = 0x1000
> +    _MAX_ENCAP_DEPTH = 4

It would be more useful if we could set the MAX_ENCAP_DEPTH separately,
so that we have the option to break assumptions.  Maybe just skip a
depth check here completely.

> +    _encap_depth = 0  # single-threaded usage assumed
> +
> +    @staticmethod
> +    def _parse_vlan_from_flowstr(flowstr):
> +        """Parse vlan(tci=X) or vlan(vid=X[,pcp=Y,cfi=Z]) from flowstr.
> +
> +        Returns (remaining_flowstr, key_tci, mask_tci).
> +        TCI values use standard bit layout (VID bits 0-11,
> +        CFI bit 12, PCP bits 13-15); byte order conversion to
> +        big-endian happens in pyroute2 be16 NLA serialization.
> +        The mask covers only the fields the caller specified:
> +        vid -> 0x0FFF, pcp -> 0xE000, cfi -> 0x1000, tci -> 0xFFFF.
> +
> +        The tci= key sets the raw TCI bitfield (no CFI validation) to allow
> +        non-Ethernet use cases.  Use cfi=1 for standard Ethernet VLAN matching.
> +        """
> +        tci = 0
> +        mask = 0
> +        has_tci = False
> +        has_vid = has_pcp = has_cfi = False
> +        _tci_mix_err = "vlan(): 'tci' cannot be mixed " \
> +                       "with 'vid'/'pcp'/'cfi'"
> +        first = True
> +        while True:
> +            flowstr = flowstr.lstrip()
> +            if not flowstr:
> +                raise ValueError("vlan(): missing ')'")
> +            if flowstr[0] == ')':
> +                break
> +            if not first:
> +                flowstr = flowstr[1:]  # skip ','
> +                if not flowstr:
> +                    raise ValueError("vlan(): missing ')' after trailing comma")
> +                flowstr = flowstr.lstrip()
> +                if flowstr and flowstr[0] == ')':
> +                    break
> +                if flowstr and flowstr[0] == ',':
> +                    raise ValueError(
> +                        "vlan(): empty or extra comma in field list")
> +            first = False
> +
> +            eq = flowstr.find('=')
> +            if eq == -1:
> +                raise ValueError("vlan(): expected key=value, got '%s'" % flowstr)
> +            key = flowstr[:eq].strip()
> +            flowstr = flowstr[eq + 1:]
> +
> +            end = flowstr.find(',')
> +            end2 = flowstr.find(')')
> +            if end == -1 or (end2 != -1 and end2 < end):
> +                end = end2
> +            val = flowstr[:end].strip()
> +            flowstr = flowstr[end:]
> +
> +            if not val:
> +                raise ValueError("vlan(): empty value for key '%s'" % key)
> +            try:
> +                v = int(val, 16) if val.startswith(('0x', '0X')) else int(val)
> +            except ValueError:
> +                raise ValueError("vlan(): invalid value '%s' for key '%s'" %
> +                                 (val, key))
> +
> +            if key == 'tci':
> +                if has_tci:
> +                    raise ValueError("vlan(): duplicate 'tci'")
> +                if has_vid or has_pcp or has_cfi:
> +                    raise ValueError(_tci_mix_err)
> +                if v > 0xFFFF or v < 0:
> +                    raise ValueError("vlan(): tci=0x%x out of range" % v)
> +                tci = v
> +                mask = 0xFFFF
> +                has_tci = True
> +            elif key == 'vid':
> +                if has_tci:
> +                    raise ValueError(_tci_mix_err)
> +                if has_vid:
> +                    raise ValueError("vlan(): duplicate 'vid'")
> +                if v < 0 or v > 0xFFF:
> +                    raise ValueError("vlan(): vid=%d out of range (0-4095)" % v)
> +                tci |= v
> +                mask |= 0x0FFF
> +                has_vid = True
> +            elif key == 'pcp':
> +                if has_tci:
> +                    raise ValueError(_tci_mix_err)
> +                if has_pcp:
> +                    raise ValueError("vlan(): duplicate 'pcp'")
> +                if v < 0 or v > 7:
> +                    raise ValueError("vlan(): pcp=%d out of range (0-7)" % v)
> +                tci |= (v & 0x7) << 13
> +                mask |= 0xE000
> +                has_pcp = True
> +            elif key == 'cfi':
> +                if has_tci:
> +                    raise ValueError(_tci_mix_err)
> +                if has_cfi:
> +                    raise ValueError("vlan(): duplicate 'cfi'")
> +                if v != 1:
> +                    raise ValueError("vlan(): cfi must be 1 for Ethernet")
> +                tci |= ovskey._VLAN_CFI_MASK
> +                mask |= ovskey._VLAN_CFI_MASK
> +                has_cfi = True
> +            else:
> +                raise ValueError("vlan(): unknown key '%s'" % key)
> +
> +        flowstr = flowstr[1:]  # skip ')'
> +        # Catch immediate '))' (user error).  A ')' after ',' is consumed
> +        # by parse()'s strspn(flowstr, "), ") inter-field separator stripping.
> +        if flowstr.lstrip().startswith(')'):
> +            raise ValueError("vlan(): unmatched ')'")
> +        # parse() strips trailing ',', ')', ' ' as inter-field separators,
> +        # so we do not need to call strspn here.
> +
> +        if mask == 0:
> +            raise ValueError("vlan(): no fields specified, "
> +                             "use vlan(vid=X[,pcp=Y,cfi=Z]) or vlan(tci=X)")
> +        if not has_tci:
> +            tci |= ovskey._VLAN_CFI_MASK
> +            mask |= ovskey._VLAN_CFI_MASK
> +        return flowstr, tci, mask
> +
> +    @staticmethod
> +    def _parse_encap_from_flowstr(flowstr):
> +        """Parse encap(inner_flow) from flowstr.
> +
> +        Returns (remaining_flowstr, inner_key_dict, inner_mask_dict)
> +        where each dict has an 'attrs' key for recursive NLA encoding.
> +        Parenthesis-depth tracking handles nested encap() calls but not
> +        quoted strings containing literal parentheses.
> +        """
> +        if ovskey._encap_depth >= ovskey._MAX_ENCAP_DEPTH:
> +            raise ValueError("encap(): max nesting depth %d exceeded" %
> +                             ovskey._MAX_ENCAP_DEPTH)
> +        try:
> +            ovskey._encap_depth += 1
> +            depth = 1
> +            end = -1
> +            for i, c in enumerate(flowstr):
> +                if c == '(':
> +                    depth += 1
> +                elif c == ')':
> +                    depth -= 1
> +                    if depth < 0:
> +                        raise ValueError("encap(): unmatched ')' at position %d" % i)
> +                    if depth == 0:
> +                        end = i
> +                        break
> +
> +            if end == -1:
> +                if depth > 1:
> +                    raise ValueError("encap(): missing ')' at end")
> +                raise ValueError("encap(): missing closing ')'")
> +
> +            inner_str = flowstr[:end].strip()
> +            if not inner_str:
> +                raise ValueError("encap(): empty inner flow")
> +
> +            flowstr = flowstr[end + 1:]
> +            if flowstr.lstrip().startswith(')'):
> +                raise ValueError("encap(): unmatched ')' after encap()")
> +            # parse() strips trailing ',', ')', ' ' as inter-field separators,
> +            # so we do not need to call strspn here.
> +
> +            inner_key = ovskey()
> +            inner_mask = ovskey()
> +            inner_key.parse(inner_str, inner_mask)
> +
> +            return flowstr, inner_key, inner_mask
> +        finally:
> +            ovskey._encap_depth -= 1
> +
>      def parse(self, flowstr, mask=None):
>          for field in (
>              ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
> @@ -1657,6 +1831,16 @@ class ovskey(nla):
>                  "eth_type",
>                  lambda x: intparse(x, "0xffff"),
>              ),
> +            (
> +                "OVS_KEY_ATTR_VLAN",
> +                "vlan",
> +                ovskey._parse_vlan_from_flowstr,
> +            ),
> +            (
> +                "OVS_KEY_ATTR_ENCAP",
> +                "encap",
> +                ovskey._parse_encap_from_flowstr,
> +            ),
>              (
>                  "OVS_KEY_ATTR_IPV4",
>                  "ipv4",
> @@ -1794,6 +1978,8 @@ class ovskey(nla):
>                  True,
>              ),
>              ("OVS_KEY_ATTR_ETHERNET", None, None, False, False),
> +            ("OVS_KEY_ATTR_VLAN", "vlan", "0x%04x", lambda x: False, True),
> +            ("OVS_KEY_ATTR_ENCAP", None, None, False, False),
>              (
>                  "OVS_KEY_ATTR_ETHERTYPE",
>                  "eth_type",

Re: [PATCH net-next v2 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing

[侯敏熙]

Hi Aaron,

Thanks for the review!

A v4 is already posted at:
https://lore.kernel.org/netdev/20260504123713.555461-1-houminxi@gmail.com/

It addresses several issues found during review (see changelog in the
cover letter). Specifically for your comments:

> ^ That's kindof a fix, BUT it also seems like we never actually used it
> anywhere, so I hope it is okay to not treat it that way.

Agreed! Since it was never used, treating it as a new feature is fine.
In v4 the ENCAP type is changed to "encap_ovskey" (a new subclass)
instead of "nested", which restricts the nla_map to L2-L4 attributes
only. This avoids pyroute2 trying to decode metadata attributes
(SKB_MARK, DP_HASH, etc.) that never appear inside ENCAP.

> It would be more useful if we could set the MAX_ENCAP_DEPTH separately,
> so that we have the option to break assumptions.  Maybe just skip a
> depth check here completely.

Makes sense! The kernel already enforces its own nesting limits, so
duplicating that check in the test tool is unnecessary. I'll drop the
depth check in v5.

Best,
Minxi

Aaron Conole <aconole@redhat.com> 于2026年5月4日周一 23:43写道：
>
> Minxi Hou <houminxi@gmail.com> writes:
>
> > Extend the ovs-dpctl.py flow parser to support vlan() and encap()
> > match strings.  vlan() accepts tci=, vid=, pcp=, and cfi=
> > parameters and generates the OVS_KEY_ATTR_VLAN attribute with a TCI
> > value in network byte order.  encap() parses nested flow strings
> > and returns OVS_KEY_ATTR_ENCAP with inner key attributes as a
> > recursive NLA container.
> >
> > The encap nla_map type is changed from "none" to "nested" so that
> > pyroute2 recursively encodes the inner flow key attributes.  The
> > VLAN nla_map type is changed from "uint16" to "be16" to match the
> > kernel's big-endian wire format.
>
> ^ That's kindof a fix, BUT it also seems like we never actually used it
> anywhere, so I hope it is okay to not treat it that way.
>
> > Signed-off-by: Minxi Hou <houminxi@gmail.com>
> > ---
> > v1 -> v2: rebase to latest net-next/main, drop --base=auto
> >
> >  .../selftests/net/openvswitch/ovs-dpctl.py    | 190 +++++++++++++++++-
> >  1 file changed, 188 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index 848f61fdcee0..317be7878937 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -901,11 +901,11 @@ class ovskey(nla):
> >      nla_flags = NLA_F_NESTED
> >      nla_map = (
> >          ("OVS_KEY_ATTR_UNSPEC", "none"),
> > -        ("OVS_KEY_ATTR_ENCAP", "none"),
> > +        ("OVS_KEY_ATTR_ENCAP", "nested"),
> >          ("OVS_KEY_ATTR_PRIORITY", "uint32"),
> >          ("OVS_KEY_ATTR_IN_PORT", "uint32"),
> >          ("OVS_KEY_ATTR_ETHERNET", "ethaddr"),
> > -        ("OVS_KEY_ATTR_VLAN", "uint16"),
> > +        ("OVS_KEY_ATTR_VLAN", "be16"),
> >          ("OVS_KEY_ATTR_ETHERTYPE", "be16"),
> >          ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"),
> >          ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"),
> > @@ -1636,6 +1636,180 @@ class ovskey(nla):
> >      class ovs_key_mpls(nla):
> >          fields = (("lse", ">I"),)
> >
> > +    # 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet
> > +    _VLAN_CFI_MASK = 0x1000
> > +    _MAX_ENCAP_DEPTH = 4
>
> It would be more useful if we could set the MAX_ENCAP_DEPTH separately,
> so that we have the option to break assumptions.  Maybe just skip a
> depth check here completely.
>
> > +    _encap_depth = 0  # single-threaded usage assumed
> > +
> > +    @staticmethod
> > +    def _parse_vlan_from_flowstr(flowstr):
> > +        """Parse vlan(tci=X) or vlan(vid=X[,pcp=Y,cfi=Z]) from flowstr.
> > +
> > +        Returns (remaining_flowstr, key_tci, mask_tci).
> > +        TCI values use standard bit layout (VID bits 0-11,
> > +        CFI bit 12, PCP bits 13-15); byte order conversion to
> > +        big-endian happens in pyroute2 be16 NLA serialization.
> > +        The mask covers only the fields the caller specified:
> > +        vid -> 0x0FFF, pcp -> 0xE000, cfi -> 0x1000, tci -> 0xFFFF.
> > +
> > +        The tci= key sets the raw TCI bitfield (no CFI validation) to allow
> > +        non-Ethernet use cases.  Use cfi=1 for standard Ethernet VLAN matching.
> > +        """
> > +        tci = 0
> > +        mask = 0
> > +        has_tci = False
> > +        has_vid = has_pcp = has_cfi = False
> > +        _tci_mix_err = "vlan(): 'tci' cannot be mixed " \
> > +                       "with 'vid'/'pcp'/'cfi'"
> > +        first = True
> > +        while True:
> > +            flowstr = flowstr.lstrip()
> > +            if not flowstr:
> > +                raise ValueError("vlan(): missing ')'")
> > +            if flowstr[0] == ')':
> > +                break
> > +            if not first:
> > +                flowstr = flowstr[1:]  # skip ','
> > +                if not flowstr:
> > +                    raise ValueError("vlan(): missing ')' after trailing comma")
> > +                flowstr = flowstr.lstrip()
> > +                if flowstr and flowstr[0] == ')':
> > +                    break
> > +                if flowstr and flowstr[0] == ',':
> > +                    raise ValueError(
> > +                        "vlan(): empty or extra comma in field list")
> > +            first = False
> > +
> > +            eq = flowstr.find('=')
> > +            if eq == -1:
> > +                raise ValueError("vlan(): expected key=value, got '%s'" % flowstr)
> > +            key = flowstr[:eq].strip()
> > +            flowstr = flowstr[eq + 1:]
> > +
> > +            end = flowstr.find(',')
> > +            end2 = flowstr.find(')')
> > +            if end == -1 or (end2 != -1 and end2 < end):
> > +                end = end2
> > +            val = flowstr[:end].strip()
> > +            flowstr = flowstr[end:]
> > +
> > +            if not val:
> > +                raise ValueError("vlan(): empty value for key '%s'" % key)
> > +            try:
> > +                v = int(val, 16) if val.startswith(('0x', '0X')) else int(val)
> > +            except ValueError:
> > +                raise ValueError("vlan(): invalid value '%s' for key '%s'" %
> > +                                 (val, key))
> > +
> > +            if key == 'tci':
> > +                if has_tci:
> > +                    raise ValueError("vlan(): duplicate 'tci'")
> > +                if has_vid or has_pcp or has_cfi:
> > +                    raise ValueError(_tci_mix_err)
> > +                if v > 0xFFFF or v < 0:
> > +                    raise ValueError("vlan(): tci=0x%x out of range" % v)
> > +                tci = v
> > +                mask = 0xFFFF
> > +                has_tci = True
> > +            elif key == 'vid':
> > +                if has_tci:
> > +                    raise ValueError(_tci_mix_err)
> > +                if has_vid:
> > +                    raise ValueError("vlan(): duplicate 'vid'")
> > +                if v < 0 or v > 0xFFF:
> > +                    raise ValueError("vlan(): vid=%d out of range (0-4095)" % v)
> > +                tci |= v
> > +                mask |= 0x0FFF
> > +                has_vid = True
> > +            elif key == 'pcp':
> > +                if has_tci:
> > +                    raise ValueError(_tci_mix_err)
> > +                if has_pcp:
> > +                    raise ValueError("vlan(): duplicate 'pcp'")
> > +                if v < 0 or v > 7:
> > +                    raise ValueError("vlan(): pcp=%d out of range (0-7)" % v)
> > +                tci |= (v & 0x7) << 13
> > +                mask |= 0xE000
> > +                has_pcp = True
> > +            elif key == 'cfi':
> > +                if has_tci:
> > +                    raise ValueError(_tci_mix_err)
> > +                if has_cfi:
> > +                    raise ValueError("vlan(): duplicate 'cfi'")
> > +                if v != 1:
> > +                    raise ValueError("vlan(): cfi must be 1 for Ethernet")
> > +                tci |= ovskey._VLAN_CFI_MASK
> > +                mask |= ovskey._VLAN_CFI_MASK
> > +                has_cfi = True
> > +            else:
> > +                raise ValueError("vlan(): unknown key '%s'" % key)
> > +
> > +        flowstr = flowstr[1:]  # skip ')'
> > +        # Catch immediate '))' (user error).  A ')' after ',' is consumed
> > +        # by parse()'s strspn(flowstr, "), ") inter-field separator stripping.
> > +        if flowstr.lstrip().startswith(')'):
> > +            raise ValueError("vlan(): unmatched ')'")
> > +        # parse() strips trailing ',', ')', ' ' as inter-field separators,
> > +        # so we do not need to call strspn here.
> > +
> > +        if mask == 0:
> > +            raise ValueError("vlan(): no fields specified, "
> > +                             "use vlan(vid=X[,pcp=Y,cfi=Z]) or vlan(tci=X)")
> > +        if not has_tci:
> > +            tci |= ovskey._VLAN_CFI_MASK
> > +            mask |= ovskey._VLAN_CFI_MASK
> > +        return flowstr, tci, mask
> > +
> > +    @staticmethod
> > +    def _parse_encap_from_flowstr(flowstr):
> > +        """Parse encap(inner_flow) from flowstr.
> > +
> > +        Returns (remaining_flowstr, inner_key_dict, inner_mask_dict)
> > +        where each dict has an 'attrs' key for recursive NLA encoding.
> > +        Parenthesis-depth tracking handles nested encap() calls but not
> > +        quoted strings containing literal parentheses.
> > +        """
> > +        if ovskey._encap_depth >= ovskey._MAX_ENCAP_DEPTH:
> > +            raise ValueError("encap(): max nesting depth %d exceeded" %
> > +                             ovskey._MAX_ENCAP_DEPTH)
> > +        try:
> > +            ovskey._encap_depth += 1
> > +            depth = 1
> > +            end = -1
> > +            for i, c in enumerate(flowstr):
> > +                if c == '(':
> > +                    depth += 1
> > +                elif c == ')':
> > +                    depth -= 1
> > +                    if depth < 0:
> > +                        raise ValueError("encap(): unmatched ')' at position %d" % i)
> > +                    if depth == 0:
> > +                        end = i
> > +                        break
> > +
> > +            if end == -1:
> > +                if depth > 1:
> > +                    raise ValueError("encap(): missing ')' at end")
> > +                raise ValueError("encap(): missing closing ')'")
> > +
> > +            inner_str = flowstr[:end].strip()
> > +            if not inner_str:
> > +                raise ValueError("encap(): empty inner flow")
> > +
> > +            flowstr = flowstr[end + 1:]
> > +            if flowstr.lstrip().startswith(')'):
> > +                raise ValueError("encap(): unmatched ')' after encap()")
> > +            # parse() strips trailing ',', ')', ' ' as inter-field separators,
> > +            # so we do not need to call strspn here.
> > +
> > +            inner_key = ovskey()
> > +            inner_mask = ovskey()
> > +            inner_key.parse(inner_str, inner_mask)
> > +
> > +            return flowstr, inner_key, inner_mask
> > +        finally:
> > +            ovskey._encap_depth -= 1
> > +
> >      def parse(self, flowstr, mask=None):
> >          for field in (
> >              ("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
> > @@ -1657,6 +1831,16 @@ class ovskey(nla):
> >                  "eth_type",
> >                  lambda x: intparse(x, "0xffff"),
> >              ),
> > +            (
> > +                "OVS_KEY_ATTR_VLAN",
> > +                "vlan",
> > +                ovskey._parse_vlan_from_flowstr,
> > +            ),
> > +            (
> > +                "OVS_KEY_ATTR_ENCAP",
> > +                "encap",
> > +                ovskey._parse_encap_from_flowstr,
> > +            ),
> >              (
> >                  "OVS_KEY_ATTR_IPV4",
> >                  "ipv4",
> > @@ -1794,6 +1978,8 @@ class ovskey(nla):
> >                  True,
> >              ),
> >              ("OVS_KEY_ATTR_ETHERNET", None, None, False, False),
> > +            ("OVS_KEY_ATTR_VLAN", "vlan", "0x%04x", lambda x: False, True),
> > +            ("OVS_KEY_ATTR_ENCAP", None, None, False, False),
> >              (
> >                  "OVS_KEY_ATTR_ETHERTYPE",
> >                  "eth_type",
>

[PATCH net-next v2 2/2] selftests: openvswitch: add pop_vlan test

[Minxi Hou]

Add a test for the OVS datapath POP_VLAN action.  The test uses
the VLAN sub-interface of a veth pair to generate 802.1Q tagged
frames and verifies that pop_vlan correctly strips the VLAN tag
before delivery via pcap frame capture.

The test has two verifications:
- Negative: forward tagged frames without pop_vlan, verify the
  VLAN tag is still present on the egress interface.
- Positive: apply pop_vlan, verify the tag is removed and an
  untagged ICMP echo request arrives at the egress interface.

Static ARP entries avoid the complexity of VLAN-tagged ARP
resolution (the egress side has no VLAN sub-interface and cannot
process tagged ARP).

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
v1 -> v2: rebase to latest net-next/main, drop --base=auto

 .../selftests/net/openvswitch/openvswitch.sh  | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index b327d3061ed5..cd614ff7b740 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -27,6 +27,7 @@ tests="
 	upcall_interfaces			ovs: test the upcall interfaces
 	tunnel_metadata				ovs: test extraction of tunnel metadata
 	drop_reason				drop: test drop reasons are emitted
+	pop_vlan					vlan-pop: POP_VLAN action strips 802.1Q tag
 	psample					psample: Sampling packets with psample"
 
 info() {
@@ -830,6 +831,137 @@ test_tunnel_metadata() {
 	return 0
 }
 
+test_pop_vlan() {
+	modprobe -q openvswitch 2>/dev/null || true
+	[ -d /sys/module/openvswitch ] || return $ksft_skip
+	ip netns add __test_pop_vlan_netns__ 2>/dev/null || \
+		{ info "CONFIG_NET_NS missing"; return $ksft_skip; }
+	ip netns del __test_pop_vlan_netns__ 2>/dev/null
+	modprobe -q 8021q 2>/dev/null || true
+	[ -d /sys/module/8021q ] || { info "CONFIG_VLAN_8021Q missing"; return $ksft_skip; }
+
+	local sbx="test_pop_vlan"
+	sbx_add "$sbx" || return $ksft_skip
+	ovs_add_dp "$sbx" vlandp || return 1
+
+	# --- baseline: untagged forwarding ---
+	ovs_add_netns_and_veths "$sbx" vlandp ns1 veth1 ns1veth 192.0.2.1/24 || return 1
+	ovs_add_netns_and_veths "$sbx" vlandp ns2 veth2 ns2veth 192.0.2.2/24 || return 1
+
+	# ARP + IPv4 bidirectional (all untagged)
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+	ip netns exec ns1 ping -c 3 -W 2 192.0.2.2 || return 1
+
+	# --- POP_VLAN test ---
+	# Register cleanup before creating resources (safe on failure paths)
+	on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null || true
+		 ip -n ns2 addr del 198.51.100.2/24 dev ns2veth 2>/dev/null || true"
+
+	# ns1: VLAN sub-interface generates tagged frames
+	ip -n ns1 link add link ns1veth name ns1veth.10 type vlan id 10
+	ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10
+	ip -n ns1 link set ns1veth.10 up
+
+	# ns2: no VLAN sub-interface. POP delivers untagged frames to ns2veth
+	ip -n ns2 addr add 198.51.100.2/24 dev ns2veth
+
+	# veth disable VLAN offload + GRO (ensure kernel software tag processing)
+	if command -v ethtool >/dev/null 2>&1; then
+		ip netns exec ns1 ethtool -k ns1veth 2>/dev/null | grep -q vlan-offload && \
+			ip netns exec ns1 ethtool -K ns1veth rx-vlan-offload off \
+				tx-vlan-offload off gro off 2>/dev/null || true
+		ip netns exec ns2 ethtool -k ns2veth 2>/dev/null | grep -q vlan-offload && \
+			ip netns exec ns2 ethtool -K ns2veth rx-vlan-offload off \
+				tx-vlan-offload off gro off 2>/dev/null || true
+	fi
+
+	ovs_del_flows "$sbx" vlandp
+
+	# Static ARP avoids VLAN-tagged ARP complexity (ns2 has no VLAN
+	# sub-interface, so tagged ARP would be invisible to ns2).
+	local ns1veth10mac ns2mac
+	ns1veth10mac=$(ip -n ns1 link show ns1veth.10 | \
+		awk '/link\/ether/ {print $2}')
+	ns2mac=$(ip -n ns2 link show ns2veth | \
+		awk '/link\/ether/ {print $2}')
+	[ -n "$ns1veth10mac" ] && echo "$ns1veth10mac" | \
+		grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
+	[ -n "$ns2mac" ] && echo "$ns2mac" | \
+		grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
+	ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \
+		dev ns1veth.10 nud permanent || return 1
+	ip -n ns2 neigh replace 198.51.100.1 lladdr "$ns1veth10mac" \
+		dev ns2veth nud permanent || return 1
+
+	# --- Negative check: fwd without pop_vlan, VLAN tag stays ---
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
+		'2' || return 1
+
+	local pcap_no_pop
+	pcap_no_pop=$(mktemp --suffix=.pcap)
+	on_exit "rm -f $pcap_no_pop"
+	ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap_no_pop" -U &
+	local tpid_no_pop=$!
+	on_exit "kill $tpid_no_pop 2>/dev/null || true"
+	sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
+
+	ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
+		>/dev/null 2>&1 || true
+	kill $tpid_no_pop 2>/dev/null || true; wait $tpid_no_pop 2>/dev/null || true
+
+	# assert: VLAN tag still present (no pop_vlan in action)
+	tcpdump -nr "$pcap_no_pop" 'vlan' 2>/dev/null | grep -q . || {
+		info "FAIL: negative check: no VLAN tag (expected tag present)"; return 1
+	}
+
+	ovs_del_flows "$sbx" vlandp
+
+	# --- Positive: pop_vlan strips tag ---
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
+		'pop_vlan,2' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+
+	local pcap
+	pcap=$(mktemp --suffix=.pcap)
+	on_exit "rm -f $pcap"
+	ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap" -U &
+	local tpid=$!
+	on_exit "kill $tpid 2>/dev/null || true"
+	sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
+
+	# ping reply unreachable: ns1veth.10 only accepts tagged frames,
+	# ns2 sends untagged reply -> dropped by ns1veth.10.
+	local ping_rc=0
+	ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
+		>/dev/null 2>&1 || ping_rc=$?
+	kill $tpid 2>/dev/null || true; wait $tpid 2>/dev/null || true
+
+	# ping failure is expected (reply path asymmetric)
+	[ "$ping_rc" -ne 0 ] || \
+		info "NOTE: ping succeeded unexpectedly (reply reached ns1veth.10)"
+
+	# assert: no VLAN tag (POP succeeded), untagged ICMP echo request arrived
+	tcpdump -nr "$pcap" 'vlan' 2>/dev/null | grep -q . && {
+		info "FAIL: POP_VLAN: VLAN tag still present"; return 1
+	}
+	tcpdump -nr "$pcap" 'icmp and icmp[icmptype]=8' \
+		2>/dev/null | grep -q . || {
+		info "FAIL: POP_VLAN: no untagged ICMP echo request"; return 1
+	}
+
+	return 0
+}
+
 run_test() {
 	(
 	tname="$1"
-- 
2.53.0

Re: [PATCH net-next v2 2/2] selftests: openvswitch: add pop_vlan test

[Aaron Conole]

Hi Minxi,

Thanks for the test (and the support infra in 1/2).  Questions below.

Minxi Hou <houminxi@gmail.com> writes:

> Add a test for the OVS datapath POP_VLAN action.  The test uses
> the VLAN sub-interface of a veth pair to generate 802.1Q tagged
> frames and verifies that pop_vlan correctly strips the VLAN tag
> before delivery via pcap frame capture.
>
> The test has two verifications:
> - Negative: forward tagged frames without pop_vlan, verify the
>   VLAN tag is still present on the egress interface.
> - Positive: apply pop_vlan, verify the tag is removed and an
>   untagged ICMP echo request arrives at the egress interface.
>
> Static ARP entries avoid the complexity of VLAN-tagged ARP
> resolution (the egress side has no VLAN sub-interface and cannot
> process tagged ARP).
>
> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---
> v1 -> v2: rebase to latest net-next/main, drop --base=auto
>
>  .../selftests/net/openvswitch/openvswitch.sh  | 132 ++++++++++++++++++
>  1 file changed, 132 insertions(+)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index b327d3061ed5..cd614ff7b740 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -27,6 +27,7 @@ tests="
>  	upcall_interfaces			ovs: test the upcall interfaces
>  	tunnel_metadata				ovs: test extraction of tunnel metadata
>  	drop_reason				drop: test drop reasons are emitted
> +	pop_vlan					vlan-pop: POP_VLAN action strips 802.1Q tag

nit: The whitespace alignment above is slightly off.

>  	psample					psample: Sampling packets with psample"
>  
>  info() {
> @@ -830,6 +831,137 @@ test_tunnel_metadata() {
>  	return 0
>  }
>  
> +test_pop_vlan() {
> +	modprobe -q openvswitch 2>/dev/null || true
> +	[ -d /sys/module/openvswitch ] || return $ksft_skip
> +	ip netns add __test_pop_vlan_netns__ 2>/dev/null || \
> +		{ info "CONFIG_NET_NS missing"; return $ksft_skip; }
> +	ip netns del __test_pop_vlan_netns__ 2>/dev/null

Weird that this is here.  All other tests would also not work if there
were no netns or OVS module.  Why were these added?

> +	modprobe -q 8021q 2>/dev/null || true
> +	[ -d /sys/module/8021q ] || { info "CONFIG_VLAN_8021Q missing"; return $ksft_skip; }
> +
> +	local sbx="test_pop_vlan"
> +	sbx_add "$sbx" || return $ksft_skip
> +	ovs_add_dp "$sbx" vlandp || return 1
> +
> +	# --- baseline: untagged forwarding ---
> +	ovs_add_netns_and_veths "$sbx" vlandp ns1 veth1 ns1veth 192.0.2.1/24 || return 1
> +	ovs_add_netns_and_veths "$sbx" vlandp ns2 veth2 ns2veth 192.0.2.2/24 || return 1
> +
> +	# ARP + IPv4 bidirectional (all untagged)
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> +	ip netns exec ns1 ping -c 3 -W 2 192.0.2.2 || return 1
> +
> +	# --- POP_VLAN test ---
> +	# Register cleanup before creating resources (safe on failure paths)
> +	on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null || true
> +		 ip -n ns2 addr del 198.51.100.2/24 dev ns2veth 2>/dev/null || true"
> +
> +	# ns1: VLAN sub-interface generates tagged frames
> +	ip -n ns1 link add link ns1veth name ns1veth.10 type vlan id 10
> +	ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10
> +	ip -n ns1 link set ns1veth.10 up
> +
> +	# ns2: no VLAN sub-interface. POP delivers untagged frames to ns2veth
> +	ip -n ns2 addr add 198.51.100.2/24 dev ns2veth
> +
> +	# veth disable VLAN offload + GRO (ensure kernel software tag processing)
> +	if command -v ethtool >/dev/null 2>&1; then
> +		ip netns exec ns1 ethtool -k ns1veth 2>/dev/null | grep -q vlan-offload && \
> +			ip netns exec ns1 ethtool -K ns1veth rx-vlan-offload off \
> +				tx-vlan-offload off gro off 2>/dev/null || true
> +		ip netns exec ns2 ethtool -k ns2veth 2>/dev/null | grep -q vlan-offload && \
> +			ip netns exec ns2 ethtool -K ns2veth rx-vlan-offload off \
> +				tx-vlan-offload off gro off 2>/dev/null || true
> +	fi
> +
> +	ovs_del_flows "$sbx" vlandp
> +
> +	# Static ARP avoids VLAN-tagged ARP complexity (ns2 has no VLAN
> +	# sub-interface, so tagged ARP would be invisible to ns2).
> +	local ns1veth10mac ns2mac
> +	ns1veth10mac=$(ip -n ns1 link show ns1veth.10 | \
> +		awk '/link\/ether/ {print $2}')
> +	ns2mac=$(ip -n ns2 link show ns2veth | \
> +		awk '/link\/ether/ {print $2}')
> +	[ -n "$ns1veth10mac" ] && echo "$ns1veth10mac" | \
> +		grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
> +	[ -n "$ns2mac" ] && echo "$ns2mac" | \
> +		grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
> +	ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \
> +		dev ns1veth.10 nud permanent || return 1
> +	ip -n ns2 neigh replace 198.51.100.1 lladdr "$ns1veth10mac" \
> +		dev ns2veth nud permanent || return 1
> +
> +	# --- Negative check: fwd without pop_vlan, VLAN tag stays ---
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
> +		'2' || return 1
> +
> +	local pcap_no_pop
> +	pcap_no_pop=$(mktemp --suffix=.pcap)
> +	on_exit "rm -f $pcap_no_pop"
> +	ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap_no_pop" -U &
> +	local tpid_no_pop=$!
> +	on_exit "kill $tpid_no_pop 2>/dev/null || true"
> +	sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
> +

We don't like to make extra files when not requested.  For example, this
is making some pcap file but do we need it?  We also have a specific
spawn function `ovs_netns_spawn_daemon` that should take care of
cleaning up at least the PID.

> +	ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
> +		>/dev/null 2>&1 || true
> +	kill $tpid_no_pop 2>/dev/null || true; wait $tpid_no_pop 2>/dev/null || true
> +
> +	# assert: VLAN tag still present (no pop_vlan in action)
> +	tcpdump -nr "$pcap_no_pop" 'vlan' 2>/dev/null | grep -q . || {
> +		info "FAIL: negative check: no VLAN tag (expected tag present)"; return 1
> +	}
> +
> +	ovs_del_flows "$sbx" vlandp
> +
> +	# --- Positive: pop_vlan strips tag ---
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
> +		'pop_vlan,2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> +
> +	local pcap
> +	pcap=$(mktemp --suffix=.pcap)
> +	on_exit "rm -f $pcap"
> +	ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap" -U &
> +	local tpid=$!
> +	on_exit "kill $tpid 2>/dev/null || true"
> +	sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
> +
> +	# ping reply unreachable: ns1veth.10 only accepts tagged frames,
> +	# ns2 sends untagged reply -> dropped by ns1veth.10.
> +	local ping_rc=0
> +	ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
> +		>/dev/null 2>&1 || ping_rc=$?
> +	kill $tpid 2>/dev/null || true; wait $tpid 2>/dev/null || true
> +
> +	# ping failure is expected (reply path asymmetric)
> +	[ "$ping_rc" -ne 0 ] || \
> +		info "NOTE: ping succeeded unexpectedly (reply reached ns1veth.10)"
> +
> +	# assert: no VLAN tag (POP succeeded), untagged ICMP echo request arrived
> +	tcpdump -nr "$pcap" 'vlan' 2>/dev/null | grep -q . && {
> +		info "FAIL: POP_VLAN: VLAN tag still present"; return 1
> +	}
> +	tcpdump -nr "$pcap" 'icmp and icmp[icmptype]=8' \
> +		2>/dev/null | grep -q . || {
> +		info "FAIL: POP_VLAN: no untagged ICMP echo request"; return 1
> +	}

Do we really need these tcpdump checks?  The ping commands above should
already capture when we are expecting tagged / untagged traffic.

> +	return 0
> +}
> +
>  run_test() {
>  	(
>  	tname="$1"

Re: [PATCH net-next v2 2/2] selftests: openvswitch: add pop_vlan test

[侯敏熙]

Hi Aaron,

Thanks for the detailed review!

> Weird that this is here.  All other tests would also not work if there
> were no netns or OVS module.  Why were these added?

You're right, the modprobe and netns pre-checks are unnecessary.
run_test already gates on the module, and ovs_add_netns_and_veths
will fail naturally if netns isn't available.  I'll remove both
pre-flight blocks in v5.

> We don't like to make extra files when not requested.  For example,
> this is making some pcap file but do we need it?

Agreed. The pcap capture was overly cautious, ping success/failure
already tells us whether the VLAN tag was correctly stripped (or not).

> We also have a specific spawn function `ovs_netns_spawn_daemon` that
> should take care of cleaning up at least the PID.

Understood. I'll use existing helpers rather than rolling custom
infrastructure.

> Do we really need these tcpdump checks?  The ping commands above
> should already capture when we are expecting tagged / untagged
> traffic.

Makes sense.  In v5 I'll drop the start_capture/stop_capture helpers
entirely and rely solely on ping results:

  - Baseline: ping without VLAN flow succeeds (ARP resolution works)
  - Negative: ping with tagged traffic and no pop_vlan fails
  - Positive: ping with tagged traffic and pop_vlan succeeds

This will significantly simplify the test.

I was previously working as QE, so my instinct was to add
defensive pre-checks and packet-level capture for every test.
I'm still adjusting to the kernel selftest convention of keeping
tests minimal and relying on existing infrastructure.

Appreciate the guidance.

Combined with the depth-check removal from patch 1/2 feedback, v5 will
be a much leaner series.

B,R
Minxi

Aaron Conole <aconole@redhat.com> 于2026年5月4日周一 23:51写道：
>
> Hi Minxi,
>
> Thanks for the test (and the support infra in 1/2).  Questions below.
>
> Minxi Hou <houminxi@gmail.com> writes:
>
> > Add a test for the OVS datapath POP_VLAN action.  The test uses
> > the VLAN sub-interface of a veth pair to generate 802.1Q tagged
> > frames and verifies that pop_vlan correctly strips the VLAN tag
> > before delivery via pcap frame capture.
> >
> > The test has two verifications:
> > - Negative: forward tagged frames without pop_vlan, verify the
> >   VLAN tag is still present on the egress interface.
> > - Positive: apply pop_vlan, verify the tag is removed and an
> >   untagged ICMP echo request arrives at the egress interface.
> >
> > Static ARP entries avoid the complexity of VLAN-tagged ARP
> > resolution (the egress side has no VLAN sub-interface and cannot
> > process tagged ARP).
> >
> > Signed-off-by: Minxi Hou <houminxi@gmail.com>
> > ---
> > v1 -> v2: rebase to latest net-next/main, drop --base=auto
> >
> >  .../selftests/net/openvswitch/openvswitch.sh  | 132 ++++++++++++++++++
> >  1 file changed, 132 insertions(+)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > index b327d3061ed5..cd614ff7b740 100755
> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > @@ -27,6 +27,7 @@ tests="
> >       upcall_interfaces                       ovs: test the upcall interfaces
> >       tunnel_metadata                         ovs: test extraction of tunnel metadata
> >       drop_reason                             drop: test drop reasons are emitted
> > +     pop_vlan                                        vlan-pop: POP_VLAN action strips 802.1Q tag
>
> nit: The whitespace alignment above is slightly off.
>
> >       psample                                 psample: Sampling packets with psample"
> >
> >  info() {
> > @@ -830,6 +831,137 @@ test_tunnel_metadata() {
> >       return 0
> >  }
> >
> > +test_pop_vlan() {
> > +     modprobe -q openvswitch 2>/dev/null || true
> > +     [ -d /sys/module/openvswitch ] || return $ksft_skip
> > +     ip netns add __test_pop_vlan_netns__ 2>/dev/null || \
> > +             { info "CONFIG_NET_NS missing"; return $ksft_skip; }
> > +     ip netns del __test_pop_vlan_netns__ 2>/dev/null
>
> Weird that this is here.  All other tests would also not work if there
> were no netns or OVS module.  Why were these added?
>
> > +     modprobe -q 8021q 2>/dev/null || true
> > +     [ -d /sys/module/8021q ] || { info "CONFIG_VLAN_8021Q missing"; return $ksft_skip; }
> > +
> > +     local sbx="test_pop_vlan"
> > +     sbx_add "$sbx" || return $ksft_skip
> > +     ovs_add_dp "$sbx" vlandp || return 1
> > +
> > +     # --- baseline: untagged forwarding ---
> > +     ovs_add_netns_and_veths "$sbx" vlandp ns1 veth1 ns1veth 192.0.2.1/24 || return 1
> > +     ovs_add_netns_and_veths "$sbx" vlandp ns2 veth2 ns2veth 192.0.2.2/24 || return 1
> > +
> > +     # ARP + IPv4 bidirectional (all untagged)
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> > +     ip netns exec ns1 ping -c 3 -W 2 192.0.2.2 || return 1
> > +
> > +     # --- POP_VLAN test ---
> > +     # Register cleanup before creating resources (safe on failure paths)
> > +     on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null || true
> > +              ip -n ns2 addr del 198.51.100.2/24 dev ns2veth 2>/dev/null || true"
> > +
> > +     # ns1: VLAN sub-interface generates tagged frames
> > +     ip -n ns1 link add link ns1veth name ns1veth.10 type vlan id 10
> > +     ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10
> > +     ip -n ns1 link set ns1veth.10 up
> > +
> > +     # ns2: no VLAN sub-interface. POP delivers untagged frames to ns2veth
> > +     ip -n ns2 addr add 198.51.100.2/24 dev ns2veth
> > +
> > +     # veth disable VLAN offload + GRO (ensure kernel software tag processing)
> > +     if command -v ethtool >/dev/null 2>&1; then
> > +             ip netns exec ns1 ethtool -k ns1veth 2>/dev/null | grep -q vlan-offload && \
> > +                     ip netns exec ns1 ethtool -K ns1veth rx-vlan-offload off \
> > +                             tx-vlan-offload off gro off 2>/dev/null || true
> > +             ip netns exec ns2 ethtool -k ns2veth 2>/dev/null | grep -q vlan-offload && \
> > +                     ip netns exec ns2 ethtool -K ns2veth rx-vlan-offload off \
> > +                             tx-vlan-offload off gro off 2>/dev/null || true
> > +     fi
> > +
> > +     ovs_del_flows "$sbx" vlandp
> > +
> > +     # Static ARP avoids VLAN-tagged ARP complexity (ns2 has no VLAN
> > +     # sub-interface, so tagged ARP would be invisible to ns2).
> > +     local ns1veth10mac ns2mac
> > +     ns1veth10mac=$(ip -n ns1 link show ns1veth.10 | \
> > +             awk '/link\/ether/ {print $2}')
> > +     ns2mac=$(ip -n ns2 link show ns2veth | \
> > +             awk '/link\/ether/ {print $2}')
> > +     [ -n "$ns1veth10mac" ] && echo "$ns1veth10mac" | \
> > +             grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
> > +     [ -n "$ns2mac" ] && echo "$ns2mac" | \
> > +             grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
> > +     ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \
> > +             dev ns1veth.10 nud permanent || return 1
> > +     ip -n ns2 neigh replace 198.51.100.1 lladdr "$ns1veth10mac" \
> > +             dev ns2veth nud permanent || return 1
> > +
> > +     # --- Negative check: fwd without pop_vlan, VLAN tag stays ---
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
> > +             '2' || return 1
> > +
> > +     local pcap_no_pop
> > +     pcap_no_pop=$(mktemp --suffix=.pcap)
> > +     on_exit "rm -f $pcap_no_pop"
> > +     ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap_no_pop" -U &
> > +     local tpid_no_pop=$!
> > +     on_exit "kill $tpid_no_pop 2>/dev/null || true"
> > +     sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
> > +
>
> We don't like to make extra files when not requested.  For example, this
> is making some pcap file but do we need it?  We also have a specific
> spawn function `ovs_netns_spawn_daemon` that should take care of
> cleaning up at least the PID.
>
> > +     ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
> > +             >/dev/null 2>&1 || true
> > +     kill $tpid_no_pop 2>/dev/null || true; wait $tpid_no_pop 2>/dev/null || true
> > +
> > +     # assert: VLAN tag still present (no pop_vlan in action)
> > +     tcpdump -nr "$pcap_no_pop" 'vlan' 2>/dev/null | grep -q . || {
> > +             info "FAIL: negative check: no VLAN tag (expected tag present)"; return 1
> > +     }
> > +
> > +     ovs_del_flows "$sbx" vlandp
> > +
> > +     # --- Positive: pop_vlan strips tag ---
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' \
> > +             'pop_vlan,2' || return 1
> > +     ovs_add_flow "$sbx" vlandp \
> > +             'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> > +
> > +     local pcap
> > +     pcap=$(mktemp --suffix=.pcap)
> > +     on_exit "rm -f $pcap"
> > +     ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap" -U &
> > +     local tpid=$!
> > +     on_exit "kill $tpid 2>/dev/null || true"
> > +     sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5))
> > +
> > +     # ping reply unreachable: ns1veth.10 only accepts tagged frames,
> > +     # ns2 sends untagged reply -> dropped by ns1veth.10.
> > +     local ping_rc=0
> > +     ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
> > +             >/dev/null 2>&1 || ping_rc=$?
> > +     kill $tpid 2>/dev/null || true; wait $tpid 2>/dev/null || true
> > +
> > +     # ping failure is expected (reply path asymmetric)
> > +     [ "$ping_rc" -ne 0 ] || \
> > +             info "NOTE: ping succeeded unexpectedly (reply reached ns1veth.10)"
> > +
> > +     # assert: no VLAN tag (POP succeeded), untagged ICMP echo request arrived
> > +     tcpdump -nr "$pcap" 'vlan' 2>/dev/null | grep -q . && {
> > +             info "FAIL: POP_VLAN: VLAN tag still present"; return 1
> > +     }
> > +     tcpdump -nr "$pcap" 'icmp and icmp[icmptype]=8' \
> > +             2>/dev/null | grep -q . || {
> > +             info "FAIL: POP_VLAN: no untagged ICMP echo request"; return 1
> > +     }
>
> Do we really need these tcpdump checks?  The ping commands above should
> already capture when we are expecting tagged / untagged traffic.
>
> > +     return 0
> > +}
> > +
> >  run_test() {
> >       (
> >       tname="$1"
>