On Mon, 20 Jul 2020 17:22:49 -0400
Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote:
> On Mon, Jul 20, 2020 at 4:57 PM Stephen Hemminger
> <step...@networkplumber.org> wrote:
> >
> > On Mon, 20 Jul 2020 09:52:27 -0400
> > Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote:
> >
> > > On Mon, Jul 20, 2020 at 12:27 AM Sriram Krishnan (srirakr2)
> > > <srira...@cisco.com> wrote:
> > > >
> > > > +Stephen Hemminger
> > > >
> > > > Hi Willem,
> > > > Thanks for looking into the code, I understand that this is more of a
> > > > generic problem wherein many of the filtering functions assume the vlan
> > > > tag to be in the skb rather than in the packet. Hence we moved the fix
> > > > from the driver to the common AF packet that our solution uses.
> > > >
> > > > I recall from the v1 of the patch you had mentioned other common areas
> > > > where this fix might be relevant (such as tap/tun), but I'm afraid I
> > > > cant comprehensively test those patches out. Please let me know your
> > > > thoughts
> > >
> > > Please use plain text to respond. HTML replies do not reach the list.
> > >
> > > Can you be more precise in which other code besides the hyper-v driver
> > > is affected? Do you have an example?
> > >
> > > This is a resubmit of the original patch. My previous
> > > questions/concerns remain valid:
> > >
> > > - if the function can now fail, all callers must be updated to detect
> > > and handle that
> > >
> > > - any solution should probably address all inputs into the tx path:
> > > packet sockets, tuntap, virtio-net
> > >
> > > - this only addresses packet sockets with ETH_P_ALL/ETH_P_NONE. Not
> > > sockets that set ETH_P_8021Q
> > >
> > > - which code in the transmit stack requires the tag to be in the skb,
> > > and does this problem after this patch still persist for Q-in-Q?
> >
> > It matters because the problem is generic, not just to the netvsc driver.
> > For example, BPF programs and netfilter rules will see different packets
> > when send is through AF_PACKET than they would see for sends from the
> > kernel stack.
> >
> > Presenting uniform data to the lower layers makes sense.
>
> Are all forwarded and locally generated packets guaranteed to always
> have VLAN information in the tag (so that this is indeed only an issue
> with input from userspace, through tuntap, virtio and packet sockets)?
>
> I guess the first might be assured due to this in __netif_receive_skb_core:
>
> if (skb->protocol == cpu_to_be16(ETH_P_8021Q) ||
> skb->protocol == cpu_to_be16(ETH_P_8021AD)) {
> skb = skb_vlan_untag(skb);
> if (unlikely(!skb))
> goto out;
> }
>
> and the second by this in vlan_dev_hard_start_xmit:
>
> if (veth->h_vlan_proto != vlan->vlan_proto ||
> vlan->flags & VLAN_FLAG_REORDER_HDR) {
> u16 vlan_tci;
> vlan_tci = vlan->vlan_id;
> vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
> __vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
> }
>
> But I don't know this code very well, so that is based on a very
> cursory glance only. Might well be missing other paths. (update: I
> think pktgen is another example.)
>
> Netfilter and BPF still need to handle tags in the packet for Q-in-Q,
> right? So does this actually simplify their logic?
>
> If the above holds and Q-in-Q is not a problem, then doing the same on
> ingress from userspace may make sense. I don't know the kind of BPF
> or netfilter programs what would be affected, and how.
>
> Then it would be good to all those inputs at once to really plug the hole.
> See also virtio_net_hdr_to_skb for another example of code that
> applies to all of tuntap, virtio, pf_packet and uml.
Older versions of Linux used to handle outer VLAN differentl
based on what the driver supported. It was a mess.
Some drivers and code paths would strip and put in meta-data, some
would leave it in skb data. But in recent (like 5 yrs), the kernel
has tried to be more uniform and only have vlan as skb tag.
It looks like AF_PACKET was overlooked at that time.
