RE: [PATCH net 2/2] net: tap: fix skb->protocol not updated after VLAN network header adjustment

Wei Fang <[email protected]>
Newsgroups dev.linux.lists.imx,org.kernel.vger.bpf,org.kernel.vger.linux-kernel,org.kernel.vger.netdev
Message-ID <GV2PR04MB1173961FAF524C3A3FB49204888D22@GV2PR04MB11739.eurprd04.prod.outlook.com>
> > > --- a/drivers/net/tap.c
> > > +++ b/drivers/net/tap.c
> > > @@ -1081,9 +1081,15 @@ static int tap_get_user_xdp(struct tap_queue
> *q,
> > struct xdp_buff *xdp)
> > >  	}
> > >
> > >  	/* Move network header to the right position for VLAN tagged
> packets */
> > > -	if (eth_type_vlan(skb->protocol) &&
> > > -	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
> > > -		skb_set_network_header(skb, depth);
> > > +	if (eth_type_vlan(skb->protocol)) {
> > > +		__be16 proto = vlan_get_protocol_and_depth(skb,
> skb->protocol,
> > > +							   &depth);
> > > +
> > > +		if (proto != 0) {
> > > +			skb_set_network_header(skb, depth);
> > > +			skb->protocol = proto;
> > > +		}
> > > +	}
> >
> > Does the same apply to the same call in tap_get_user?
> 
> The situation of tap_get_user() is different, skb_probe_transport_header()
> is called before the VLAN adjustment block. So I think transport_header
> should be correct. The only concern is whether skb->protocol needs to be
> updated after calling skb_set_network_header().
> 
> I'm not sure as I am not fairly familiar with the tap driver. I added this patch
> because Sashiko reported that the tap driver has the same issue as af_packet.
> 
> >
> > And in general to other callers of vlan_get_protocol_and_depth,
> > including through wrapper skb_network_protocol?
> 
> I don't think this issue exists elsewhere. The issue arises because
> skb_probe_transport_header() is called after skb_set_network_header(), and
> at this point, skb->protocol and network_header are not synchronized ( In
> __skb_flow_dissect(), nhoff = skb_network_offset(skb) but proto is
> ETH_P_8021Q or ETH_P_8021AD)), causing skb_probe_transport_header() to
> fail to set transport_header correctly.
> 
> Perhaps the correct approach would be to restore the original `skb->protocol`
> value after `skb_probe_transport_header()`, maintaining consistency with the
> previous behavior; otherwise, it might introduce new issues.

For AF_PACKET, it has been confirmed that skb->protocol does not need to be
restored to its initial value; otherwise, the egress tc flower for protocol ip will
not match the packet. A known issue is that the packets cannot match the egress
TC flower rules for protocol 802.1Q, but this issue exists before this series. This
is likely a limitation of using AF_PACKET to send packets.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.