From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [bpf-next PATCH 1/3] net: fix generic XDP to handle if eth header was mangled Date: Mon, 1 Oct 2018 21:13:56 +0200 Message-ID: References: <153788541527.7040.8722029216689268562.stgit@firesoul> <153788554969.7040.12321291244532889727.stgit@firesoul> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Networking , Daniel Borkmann , yoel@kviknet.dk, Alexei Starovoitov To: Song Liu , Jesper Dangaard Brouer Return-path: Received: from www62.your-server.de ([213.133.104.62]:42828 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725958AbeJBBxN (ORCPT ); Mon, 1 Oct 2018 21:53:13 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: [ ping to Jesper wrt feedback ] On 09/26/2018 07:36 AM, Song Liu wrote: > On Tue, Sep 25, 2018 at 7:26 AM Jesper Dangaard Brouer > wrote: >> >> XDP can modify (and resize) the Ethernet header in the packet. >> >> There is a bug in generic-XDP, because skb->protocol and skb->pkt_type >> are setup before reaching (netif_receive_)generic_xdp. >> >> This bug was hit when XDP were popping VLAN headers (changing >> eth->h_proto), as skb->protocol still contains VLAN-indication >> (ETH_P_8021Q) causing invocation of skb_vlan_untag(skb), which corrupt >> the packet (basically popping the VLAN again). >> >> This patch catch if XDP changed eth header in such a way, that SKB >> fields needs to be updated. >> >> Fixes: d445516966dc ("net: xdp: support xdp generic on virtual devices") >> Signed-off-by: Jesper Dangaard Brouer >> --- >> net/core/dev.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index ca78dc5a79a3..db6d89f536cb 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -4258,6 +4258,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, >> struct netdev_rx_queue *rxqueue; >> void *orig_data, *orig_data_end; >> u32 metalen, act = XDP_DROP; >> + __be16 orig_eth_type; >> + struct ethhdr *eth; >> + bool orig_bcast; >> int hlen, off; >> u32 mac_len; >> >> @@ -4298,6 +4301,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, >> xdp->data_hard_start = skb->data - skb_headroom(skb); >> orig_data_end = xdp->data_end; >> orig_data = xdp->data; >> + eth = (struct ethhdr *)xdp->data; >> + orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest); >> + orig_eth_type = eth->h_proto; >> >> rxqueue = netif_get_rxqueue(skb); >> xdp->rxq = &rxqueue->xdp_rxq; >> @@ -4321,6 +4327,14 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, >> >> } >> >> + /* check if XDP changed eth hdr such SKB needs update */ >> + eth = (struct ethhdr *)xdp->data; >> + if ((orig_eth_type != eth->h_proto) || >> + (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) { > > Is the actions below always correct for the condition above? Do we need > to confirm the SKB is updated properly? > >> + __skb_push(skb, mac_len); >> + skb->protocol = eth_type_trans(skb, skb->dev); >> + } >> + >> switch (act) { >> case XDP_REDIRECT: >> case XDP_TX: >>