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

[Wei Fang]

> > > > > --- 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?

I think it is feasible. The transport_header will be set correctly, and other
behaviors will remain consistent with before, without introducing an
new issues.