Willem de Bruijn
2022-Apr-18 15:38 UTC
[PATCH net 1/2] net/af_packet: adjust network header position for VLAN tagged packets
On Mon, Apr 18, 2022 at 12:44 AM Hangbin Liu <liuhangbin at gmail.com> wrote:> > Flavio reported that the kernel drops GSO VLAN tagged packet if it's > created with socket(AF_PACKET, SOCK_RAW, 0) plus virtio_net_hdr. > > The reason is AF_PACKET doesn't adjust the skb network header if there is > a VLAN tag. Then after virtio_net_hdr_set_proto() called, the skb->protocol > will be set to ETH_P_IP/IPv6. And in later inet/ipv6_gso_segment() the skb > is dropped as network header position is invalid. > > Fix it by adjusting network header position in packet_parse_headers() > and move the function before calling virtio_net_hdr_* functions. > > I also moved skb->no_fcs setting upper to make all skb setting together > and keep consistence of function packet_sendmsg_spkt(). > > No need to update tpacket_snd() as it calls packet_parse_headers() in > tpacket_fill_skb() before calling virtio_net_hdr_* functions. > > Fixes: 75c65772c3d1 ("net/packet: Ask driver for protocol if not provided by user") > Reported-by: Flavio Leitner <fbl at redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin at gmail.com>Strictly speaking VLAN tagged GSO packets have never been supported. The only defined types are TCP and UDP over IPv4 and IPv6: define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */ define VIRTIO_NET_HDR_GSO_UDP 3 /* GSO frame, IPv4 UDP (UFO) */ define VIRTIO_NET_HDR_GSO_TCPV6 4 /* GSO frame, IPv6 TCP */ I don't think this is a bug, more a stretching of the definition of those flags.> --- > net/packet/af_packet.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 002d2b9c69dd..cfdbda28ef82 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1924,12 +1924,20 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, > > static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) > { > + int depth; > + > if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && > sock->type == SOCK_RAW) { > skb_reset_mac_header(skb); > skb->protocol = dev_parse_header_protocol(skb); > } > > + /* Move network header to the right position for VLAN tagged packets */ > + if (likely(skb->dev->type == ARPHRD_ETHER) && > + eth_type_vlan(skb->protocol) && > + __vlan_get_protocol(skb, skb->protocol, &depth) != 0) > + skb_set_network_header(skb, depth); > + > skb_probe_transport_header(skb); > } > > @@ -3047,6 +3055,11 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)> skb->mark = sockc.mark; > skb->tstamp = sockc.transmit_time; > > + if (unlikely(extra_len == 4)) > + skb->no_fcs = 1; > + > + packet_parse_headers(skb, sock); > + > if (has_vnet_hdr) { > err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()); > if (err) > @@ -3055,11 +3068,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) > virtio_net_hdr_set_proto(skb, &vnet_hdr); > } > > - packet_parse_headers(skb, sock); > - > - if (unlikely(extra_len == 4)) > - skb->no_fcs = 1; > -Moving packet_parse_headers before or after virtio_net_hdr_to_skb may have additional subtle effects on protocol detection. I think it's probably okay, as tpacket_snd also calls in the inverse order. But there have been many issues in this codepath. We should also maintain feature consistency between packet_snd, tpacket_snd and to the limitations of its feature set to packet_sendmsg_spkt. The no_fcs is already lacking in tpacket_snd as far as I can tell. But packet_sendmsg_spkt also sets it and calls packet_parse_headers. Because this patch touches many other packets besides the ones intended, I am a bit concerned about unintended consequences. Perhaps stretching the definition of the flags to include VLAN is acceptable (unlike outright tunnels), but even then I would suggest for net-next.> err = po->xmit(skb); > if (unlikely(err != 0)) { > if (err > 0)