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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Wei Fang wrote:
> > > > --- 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.

Actually this update of network header to start of the IP header may
have been a mistake. If userspace inserts a VLAN packet, that is what
should enter the stack.

But for this specific issue: would it make sense to just move
skb_probe_transport_header before that adjustment?