| Message ID | 1474658127-23477-1-git-send-email-e@erig.me |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
On Fri, Sep 23, 2016 at 03:15:27PM -0400, Eric Garver wrote: > We need to check if a packet is double tagged. If so make sure to push > 0x88a8 instead of 0x8100. Without this a simple port redirect of 802.1ad > frames means the outer tag gets translated from 0x88a8 to 0x8100 by the > userspace datapath. > > This only affected kernels that don't use TP_STATUS_VLAN_TPID_VALID, > which is kernels < 3.14. > > Signed-off-by: Eric Garver <e@erig.me> ... > if (auxdata_has_vlan_tci(aux)) { > + struct eth_header *eth = dp_packet_data(buffer); > + bool doubleTagged = eth->eth_type == htons(ETH_TYPE_VLAN_8021Q); > + > if (retval < ETH_HEADER_LEN) { > return EINVAL; > } > > - eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux), > + eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux, doubleTagged), > htons(aux->tp_vlan_tci)); > break; > } The new code here dereferences eth->eth_type before checking that the packet is long enough. That is, the retval check should precede the doubletagged check. As a minor point of style, we prefer underscores over capitals: s/doubleTagged/double_tagged/.
On Mon, Oct 03, 2016 at 01:16:55PM -0700, Ben Pfaff wrote: > On Fri, Sep 23, 2016 at 03:15:27PM -0400, Eric Garver wrote: > > We need to check if a packet is double tagged. If so make sure to push > > 0x88a8 instead of 0x8100. Without this a simple port redirect of 802.1ad > > frames means the outer tag gets translated from 0x88a8 to 0x8100 by the > > userspace datapath. > > > > This only affected kernels that don't use TP_STATUS_VLAN_TPID_VALID, > > which is kernels < 3.14. > > > > Signed-off-by: Eric Garver <e@erig.me> > > ... > > > if (auxdata_has_vlan_tci(aux)) { > > + struct eth_header *eth = dp_packet_data(buffer); > > + bool doubleTagged = eth->eth_type == htons(ETH_TYPE_VLAN_8021Q); > > + > > if (retval < ETH_HEADER_LEN) { > > return EINVAL; > > } > > > > - eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux), > > + eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux, doubleTagged), > > htons(aux->tp_vlan_tci)); > > break; > > } > > The new code here dereferences eth->eth_type before checking that the > packet is long enough. That is, the retval check should precede the > doubletagged check. > Oops. Thanks for catching. Will send a v2. > As a minor point of style, we prefer underscores over capitals: > s/doubleTagged/double_tagged/.
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index ef8471995a3d..0f929c9cfc3b 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -995,12 +995,14 @@ netdev_linux_rxq_dealloc(struct netdev_rxq *rxq_) } static ovs_be16 -auxdata_to_vlan_tpid(const struct tpacket_auxdata *aux) +auxdata_to_vlan_tpid(const struct tpacket_auxdata *aux, bool doubleTagged) { if (aux->tp_status & TP_STATUS_VLAN_TPID_VALID) { return htons(aux->tp_vlan_tpid); + } else if (doubleTagged) { + return htons(ETH_TYPE_VLAN_8021AD); } else { - return htons(ETH_TYPE_VLAN); + return htons(ETH_TYPE_VLAN_8021Q); } } @@ -1060,11 +1062,14 @@ netdev_linux_rxq_recv_sock(int fd, struct dp_packet *buffer) aux = ALIGNED_CAST(struct tpacket_auxdata *, CMSG_DATA(cmsg)); if (auxdata_has_vlan_tci(aux)) { + struct eth_header *eth = dp_packet_data(buffer); + bool doubleTagged = eth->eth_type == htons(ETH_TYPE_VLAN_8021Q); + if (retval < ETH_HEADER_LEN) { return EINVAL; } - eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux), + eth_push_vlan(buffer, auxdata_to_vlan_tpid(aux, doubleTagged), htons(aux->tp_vlan_tci)); break; }
We need to check if a packet is double tagged. If so make sure to push 0x88a8 instead of 0x8100. Without this a simple port redirect of 802.1ad frames means the outer tag gets translated from 0x88a8 to 0x8100 by the userspace datapath. This only affected kernels that don't use TP_STATUS_VLAN_TPID_VALID, which is kernels < 3.14. Signed-off-by: Eric Garver <e@erig.me> --- lib/netdev-linux.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)