diff mbox series

[Openvpn-devel,3/9] maddr: export VLAN ID from client context to maddr object

Message ID 20191009143422.9419-4-a@unstable.cc
State Accepted
Headers show
Series support VLANs in TAP mode | expand

Commit Message

Antonio Quartulli Oct. 9, 2019, 3:34 a.m. UTC 
When receiving a packet from a client, the associated maddr needs to
carry also the VID associated with that client. This way the VID can be
appended to the packet later, if needed.

This patch adds support for exporting the VID from the client context to
the related per-packet maddr object.

Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/mroute.c | 54 +++++++++++++++++++++++++++++++-------------
 src/openvpn/mroute.h | 11 ++++++---
 src/openvpn/multi.c  |  3 +++
 3 files changed, 49 insertions(+), 19 deletions(-)

Comments

Gert Doering Nov. 6, 2019, 10:11 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Stared at the code, ran client side tests, full set of server side tests,
and compiled (+client tested) on FreeBSD 7.4 to give the "anonymous union 
explosion" bits a good checking :-) (it does not explode anymore, thanks 
for fixing that).

Similar to 2/9, this patch does not actually change behaviour yet - it 
adds the 2-byte vid to the hashed ethernet address for per-client learning,
but as all callers set it to "0", there is no "vlan separation" effect
yet (I know it's coming in a future patch, just pointing out why this 
patch does not change visible behaviour yet).

It shows up in the "MULTI: Learn" lines already, though

Nov  6 22:04:51 gentoo tap-udp-p2mp[11510]: freebsd-11-amd64/2001:608:0:814::f000:16 MULTI: Learn: 00:bd:b5:63:b5:04@0 -> freebsd-11-amd64/2001:608:0:814::f000:16

.. the "@0" bit after the MAC address is the PVID.


As a side note: it seems that whoever did the IPv6 payload patches was
a bit sloppy (it was me, so I am allowed to say that).  MAC learning is 
only done on IPv4 and ARP packets, but not on IPv6 packets - so I expect 
"ipv6 only mode on tap" to be a bit wacky...  this needs re-testing
before we can declare "ipv6-only works reliably".


Your patch has been applied to the master branch.

commit a2b7230712dbc8cfab85d5bd59605f58fc5fe5f8
Author: Antonio Quartulli
Date:   Wed Oct 9 16:34:16 2019 +0200

     maddr: export VLAN ID from client context to maddr object

     Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20191009143422.9419-4-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18917.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Nov. 6, 2019, 11:12 a.m. UTC | #2 
Hi,

On Wed, Nov 06, 2019 at 10:11:58PM +0100, Gert Doering wrote:
> As a side note: it seems that whoever did the IPv6 payload patches was
> a bit sloppy (it was me, so I am allowed to say that).  MAC learning is 
> only done on IPv4 and ARP packets, but not on IPv6 packets - so I expect 
> "ipv6 only mode on tap" to be a bit wacky...  this needs re-testing
> before we can declare "ipv6-only works reliably".

I was wrong here.  This part is only for the extraction of the "inner"
addresses in an tap/ethernet packet, which are then subsequently used
for ENABLE_PF filtering.

Since *that* only has IPv4 (because nobody reviewed and ACKed Antonio's
"make PF support IPv6" patches yet) it makes sense that the code only
extract IPv4 addresses.

Now, I see a slight merge conflict with the PF/IPv6 patch set... :-)

gert
diff mbox series

Patch

diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index c52ef7b1..bdb1b0c0 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -58,7 +58,7 @@  static inline bool
 is_mac_mcast_maddr(const struct mroute_addr *addr)
 {
     return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER
-           && is_mac_mcast_addr(addr->eth_addr);
+           && is_mac_mcast_addr(addr->ether.addr);
 }
 
 /*
@@ -249,12 +249,15 @@  mroute_extract_addr_ip(struct mroute_addr *src, struct mroute_addr *dest,
 
 static void
 mroute_copy_ether_to_addr(struct mroute_addr *maddr,
-                          const uint8_t *ether_addr)
+                          const uint8_t *ether_addr,
+                          uint16_t vid)
 {
     maddr->type = MR_ADDR_ETHER;
     maddr->netbits = 0;
     maddr->len = OPENVPN_ETH_ALEN;
-    memcpy(maddr->eth_addr, ether_addr, OPENVPN_ETH_ALEN);
+    memcpy(maddr->ether.addr, ether_addr, OPENVPN_ETH_ALEN);
+    maddr->len += sizeof(vid);
+    maddr->ether.vid = vid;
 }
 
 unsigned int
@@ -262,6 +265,7 @@  mroute_extract_addr_ether(struct mroute_addr *src,
                           struct mroute_addr *dest,
                           struct mroute_addr *esrc,
                           struct mroute_addr *edest,
+                          uint16_t vid,
                           const struct buffer *buf)
 {
     unsigned int ret = 0;
@@ -270,11 +274,11 @@  mroute_extract_addr_ether(struct mroute_addr *src,
         const struct openvpn_ethhdr *eth = (const struct openvpn_ethhdr *) BPTR(buf);
         if (src)
         {
-            mroute_copy_ether_to_addr(src, eth->source);
+            mroute_copy_ether_to_addr(src, eth->source, vid);
         }
         if (dest)
         {
-            mroute_copy_ether_to_addr(dest, eth->dest);
+            mroute_copy_ether_to_addr(dest, eth->dest, vid);
 
             /* ethernet broadcast/multicast packet? */
             if (is_mac_mcast_addr(eth->dest))
@@ -289,18 +293,35 @@  mroute_extract_addr_ether(struct mroute_addr *src,
         if (esrc || edest)
         {
             struct buffer b = *buf;
-            if (buf_advance(&b, sizeof(struct openvpn_ethhdr)))
+            if (!buf_advance(&b, sizeof(struct openvpn_ethhdr)))
             {
-                switch (ntohs(eth->proto))
-                {
-                    case OPENVPN_ETH_P_IPV4:
-                        ret |= (mroute_extract_addr_ip(esrc, edest, &b) << MROUTE_SEC_SHIFT);
-                        break;
+                return 0;
+            }
 
-                    case OPENVPN_ETH_P_ARP:
-                        ret |= (mroute_extract_addr_arp(esrc, edest, &b) << MROUTE_SEC_SHIFT);
-                        break;
+            uint16_t proto = eth->proto;
+            if (proto == htons(OPENVPN_ETH_P_8021Q))
+            {
+                if (!buf_advance(&b, SIZE_ETH_TO_8021Q_HDR))
+                {
+                    /* It's an 802.1Q packet, but doesn't have a full header,
+                     * so something went wrong */
+                    return 0;
                 }
+
+                const struct openvpn_8021qhdr *tag;
+                tag = (const struct openvpn_8021qhdr *)BPTR(buf);
+                proto = tag->proto;
+            }
+
+            switch (ntohs(proto))
+            {
+                case OPENVPN_ETH_P_IPV4:
+                    ret |= (mroute_extract_addr_ip(esrc, edest, &b) << MROUTE_SEC_SHIFT);
+                    break;
+
+                case OPENVPN_ETH_P_ARP:
+                    ret |= (mroute_extract_addr_arp(esrc, edest, &b) << MROUTE_SEC_SHIFT);
+                    break;
             }
         }
 #endif
@@ -444,8 +465,9 @@  mroute_addr_print_ex(const struct mroute_addr *ma,
         switch (maddr.type & MR_ADDR_MASK)
         {
             case MR_ADDR_ETHER:
-                buf_printf(&out, "%s", format_hex_ex(ma->eth_addr,
-                                                     sizeof(ma->eth_addr), 0, 1, ":", gc));
+                buf_printf(&out, "%s", format_hex_ex(ma->ether.addr,
+                                                     sizeof(ma->ether.addr), 0, 1, ":", gc));
+                buf_printf(&out, "@%hu", ma->ether.vid);
                 break;
 
             case MR_ADDR_IPV4:
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 7fcd9956..113aa8c5 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -82,7 +82,10 @@  struct mroute_addr {
                       * valid if MR_WITH_NETBITS is set */
     union {
         uint8_t raw_addr[MR_MAX_ADDR_LEN]; /* actual address */
-        uint8_t eth_addr[OPENVPN_ETH_ALEN];
+        struct {
+            uint8_t addr[OPENVPN_ETH_ALEN];
+            uint16_t vid;
+        } ether;
         struct {
             in_addr_t addr;     /* _network order_ IPv4 address */
             in_port_t port;     /* _network order_ TCP/UDP port */
@@ -100,7 +103,7 @@  struct mroute_addr {
 /* Wrappers to support compilers that do not grok anonymous unions */
         mroute_union
 #define raw_addr mroute_union.raw_addr
-#define eth_addr mroute_union.eth_addr
+#define ether mroute_union.ether
 #define v4 mroute_union.v4
 #define v6 mroute_union.v6
 #define v4mappedv6 mroute_union.v4mappedv6
@@ -178,6 +181,7 @@  unsigned int mroute_extract_addr_ether(struct mroute_addr *src,
                                        struct mroute_addr *dest,
                                        struct mroute_addr *esrc,
                                        struct mroute_addr *edest,
+                                       uint16_t vid,
                                        const struct buffer *buf);
 
 /*
@@ -189,6 +193,7 @@  mroute_extract_addr_from_packet(struct mroute_addr *src,
                                 struct mroute_addr *dest,
                                 struct mroute_addr *esrc,
                                 struct mroute_addr *edest,
+                                uint16_t vid,
                                 const struct buffer *buf,
                                 int tunnel_type)
 {
@@ -200,7 +205,7 @@  mroute_extract_addr_from_packet(struct mroute_addr *src,
     }
     else if (tunnel_type == DEV_TYPE_TAP)
     {
-        ret = mroute_extract_addr_ether(src, dest, esrc, edest, buf);
+        ret = mroute_extract_addr_ether(src, dest, esrc, edest, vid, buf);
     }
     return ret;
 }
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 8caaa868..95b33e7a 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2570,6 +2570,7 @@  multi_process_incoming_link(struct multi_context *m, struct multi_instance *inst
                                                                &dest,
                                                                NULL,
                                                                NULL,
+                                                               0,
                                                                &c->c2.to_tun,
                                                                DEV_TYPE_TUN);
 
@@ -2664,6 +2665,7 @@  multi_process_incoming_link(struct multi_context *m, struct multi_instance *inst
 #else
                                                                NULL,
 #endif
+                                                               0,
                                                                &c->c2.to_tun,
                                                                DEV_TYPE_TAP);
 
@@ -2791,6 +2793,7 @@  multi_process_incoming_tun(struct multi_context *m, const unsigned int mpp_flags
                                                        NULL,
 #endif
                                                        NULL,
+                                                       0,
                                                        &m->top.c2.buf,
                                                        dev_type);