Re: Reproducible VLAN/e1000e crash in 2.6.36 vanilla.

[Eric Dumazet <eric.dumazet@gmail.com>]

Le lundi 25 octobre 2010 à 14:34 -0700, John Fastabend a écrit :
> On 10/25/2010 2:18 PM, Ben Greear wrote:
> > On 10/25/2010 10:57 AM, Ben Greear wrote:
> >>
> >> To re-create, setup 2 802.1q vlans on different physical interfaces on
> >> the same system,
> >> set up routing rules such that send-to-self works, and pass traffic
> >> (UDP/IPv4 in this case,
> >> but doesn't seem to matter).
> >> Stop traffic, then attempt to create additional 802.1q vlans on the same
> >> physical interfaces.
> >> The crash only appears to happen after having sent traffic on the
> >> interface.
> >>
> >> Likely it will also crash if one system is sending to another, but so
> >> far we've
> >> just tested sending-to-self.
> >>
> >> This appears very reproducible for us, and appears to be the same
> >> problem that
> >> I had reported against our hacked kernel here:
> >>
> >> http://www.spinics.net/lists/netdev/msg144748.html
> > 
> > Bleh, I think I see the problem.
> > 
> > If a NIC is in promis mode, it can receive VLAN packets for which there
> > are no VLAN devices.
> > 
> > static gro_result_t
> > vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
> >                  unsigned int vlan_tci, struct sk_buff *skb)
> > {
> >          struct sk_buff *p;
> >          struct net_device *vlan_dev;
> >          u16 vlan_id;
> > 
> >          if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
> >                  skb->deliver_no_wcard = 1;
> > 
> >          skb->skb_iif = skb->dev->ifindex;
> >          __vlan_hwaccel_put_tag(skb, vlan_tci);
> >          vlan_id = vlan_tci & VLAN_VID_MASK;
> >          vlan_dev = vlan_group_get_device(grp, vlan_id);
> > 
> >          if (vlan_dev)
> >                  skb->dev = vlan_dev;
> >          else if (vlan_id) {
> >                  if (!(skb->dev->flags & IFF_PROMISC))
> >                          goto drop;
> >                  skb->pkt_type = PACKET_OTHERHOST;
> >          }
> > 
> > You hit that else branch, and then skb->dev remains the physical
> > device.
> > 
> > Later, it's passed to:
> > 
> > int vlan_hwaccel_do_receive(struct sk_buff *skb)
> > {
> > 	struct net_device *dev = skb->dev;
> > 	struct vlan_rx_stats     *rx_stats;
> > 
> > 	skb->dev = vlan_dev_info(dev)->real_dev;
> > 	netif_nit_deliver(skb);
> > 
> 
> Looks like this should be fixed on net-next,
> 
> bool vlan_hwaccel_do_receive(struct sk_buff **skbp)
> {
>         struct sk_buff *skb = *skbp;
>         u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>         struct net_device *vlan_dev;
>         struct vlan_rx_stats *rx_stats;
> 
>         vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>         if (!vlan_dev) {
>                 if (vlan_id)
>                         skb->pkt_type = PACKET_OTHERHOST;
>                 return false;
>         }
> 
> If the vlan_dev is not found do not set skb->dev and return false then
> in __netif_receive_skb,
> 
>       if (vlan_tx_tag_present(skb)) {
>                 if (pt_prev) {
>                         ret = deliver_skb(skb, pt_prev, orig_dev);
>                         pt_prev = NULL;
>                 }
>                 if (vlan_hwaccel_do_receive(&skb)) {
>                         ret = __netif_receive_skb(skb);
>                         goto out;
>                 } else if (unlikely(!skb))
>                         goto out;
>         }
> 

Yes but net-next is totally different beast for vlans ;)

We should make a patch for 2.6.36, not bringing huge vlan stuff added
for 2.6.37