| Message ID | 1223322707.24688.46.camel@deepthought.nh.local |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jarod Neuner <j.neuner@networkharbor.com> Date: Mon, 6 Oct 2008 14:51:47 -0500 > I noticed a problem where the kernel is not responding to IGMP Queries > from several Level-3 network switches. This behavior is caused by these > switches using VLAN tags on IGMP messages. At first I thought the > switches were out of spec, but every other network stack I've checked > delivers packets with unconfigured VLAN assignments directly to the > incoming interface. > > This patch corrects my problem, but I'm still interested in discussion > about whether or not this should be a parameter in /proc/sys/net or even > if this policy change should be applied elsewhere in the the network > stack. Interesting. Patrick, any opinion? > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index 4bf014e..b65a8fd 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -158,9 +158,13 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev, > rcu_read_lock(); > skb->dev = __find_vlan_dev(dev, vlan_id); > if (!skb->dev) { > - pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n", > + pr_debug("%s: WARNING: Forwarding VID: %u to dev: %s\n", > __func__, vlan_id, dev->name); > - goto err_unlock; > + skb->dev = dev; > + skb_pull_rcsum(skb, VLAN_HLEN); > + vlan_set_encap_proto(skb, vhdr); > + rcu_read_lock(); > + return 0; > } > > skb->dev->last_rx = jiffies; > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Jarod Neuner <j.neuner@networkharbor.com> > Date: Mon, 6 Oct 2008 14:51:47 -0500 > > >> I noticed a problem where the kernel is not responding to IGMP Queries >> from several Level-3 network switches. This behavior is caused by these >> switches using VLAN tags on IGMP messages. At first I thought the >> switches were out of spec, but every other network stack I've checked >> delivers packets with unconfigured VLAN assignments directly to the >> incoming interface. >> >> This patch corrects my problem, but I'm still interested in discussion >> about whether or not this should be a parameter in /proc/sys/net or even >> if this policy change should be applied elsewhere in the the network >> stack. >> > > Interesting. > Indeed. > Patrick, any opinion? > I don't like a /proc flag very much, but I believe it should be configurable. To make this behaviour consistent there also needs to be some indication to drivers to disable hardware filters, so we really should have an explicit action from the user first. We've been talking about an IFF_ALLVLAN flag on netdev a while ago, which would disable VLAN hardware filters, similar to IFF_ALLMULTI. An additional flag on the ethernet device could indicate that it should receive unknown VLANs directly. That would introduce some possible inconsistencies however since the flag could be set without the VLAN code even loaded, in which case it would not have any effect. Another possibility would be to use a catch-all VLAN device with VID 0xfff (reserved for implementation use). This would allow to configure priority mappings, header reordering etc. and have separate statistics. The drivers could just use the magic VID as an indication to disable filtering, but I would still suggest to add the IFF_ALLVLAN flag because its useful on its own. >> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c >> index 4bf014e..b65a8fd 100644 >> --- a/net/8021q/vlan_dev.c >> +++ b/net/8021q/vlan_dev.c >> @@ -158,9 +158,13 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev, >> rcu_read_lock(); >> skb->dev = __find_vlan_dev(dev, vlan_id); >> if (!skb->dev) { >> - pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n", >> + pr_debug("%s: WARNING: Forwarding VID: %u to dev: %s\n", >> __func__, vlan_id, dev->name); >> - goto err_unlock; >> + skb->dev = dev; >> + skb_pull_rcsum(skb, VLAN_HLEN); >> + vlan_set_encap_proto(skb, vhdr); >> + rcu_read_lock(); >> + return 0; >> This can't be right though, the packet needs to be passed up the stack. The same change will also be needed in __vlan_hwaccel_rx(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Patrick McHardy wrote: > We've been talking about an IFF_ALLVLAN flag on netdev a while > ago, which would disable VLAN hardware filters, similar to > IFF_ALLMULTI. An additional flag on the ethernet device could > indicate that it should receive unknown VLANs directly. That > would introduce some possible inconsistencies however since the > flag could be set without the VLAN code even loaded, in which > case it would not have any effect. My original thought was to do something like this in net/core/dev.c using a method similar to handle_bridge or handle_macvlen. So, if the packet doesn't get handled by the ptype_base list and IFF_ALLVLAN is set, then strip the header and let the packet through. The sticky point would be whether or not this policy should be enabled by default, as it seems to be in other network stacks. > > Another possibility would be to use a catch-all VLAN device with > VID 0xfff (reserved for implementation use). This would allow > to configure priority mappings, header reordering etc. and have > separate statistics. The drivers could just use the magic VID > as an indication to disable filtering, but I would still suggest > to add the IFF_ALLVLAN flag because its useful on its own. Most switches treat VLAN 1 as the "Default" or "Administration" VLAN. It might make sense to map VLAN 1 to the incoming interface, and then use that as a catch all. Then again, that might be a terrible idea as well. =) > This can't be right though, the packet needs to be passed > up the stack. The same change will also be needed in > __vlan_hwaccel_rx(). Duly noted - thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jarod Neuner wrote: >> Patrick McHardy wrote: >> We've been talking about an IFF_ALLVLAN flag on netdev a while >> ago, which would disable VLAN hardware filters, similar to >> IFF_ALLMULTI. An additional flag on the ethernet device could >> indicate that it should receive unknown VLANs directly. That >> would introduce some possible inconsistencies however since the >> flag could be set without the VLAN code even loaded, in which >> case it would not have any effect. > > My original thought was to do something like this in net/core/dev.c > using a method similar to handle_bridge or handle_macvlen. So, if the > packet doesn't get handled by the ptype_base list and IFF_ALLVLAN is > set, then strip the header and let the packet through. The sticky point > would be whether or not this policy should be enabled by default, as it > seems to be in other network stacks. I don't think we should change the default, it would probably catch some people by surprise. It might not be handled properly by packet filtering rules etc. >> Another possibility would be to use a catch-all VLAN device with >> VID 0xfff (reserved for implementation use). This would allow >> to configure priority mappings, header reordering etc. and have >> separate statistics. The drivers could just use the magic VID >> as an indication to disable filtering, but I would still suggest >> to add the IFF_ALLVLAN flag because its useful on its own. > > Most switches treat VLAN 1 as the "Default" or "Administration" VLAN. > It might make sense to map VLAN 1 to the incoming interface, and then > use that as a catch all. Then again, that might be a terrible idea as > well. =) I prefer 0xfff because its not used for anything else so far. Especially the administrative VLAN (even if only by convention) doesn't seem by a good idea because its pretty likely you would treat it differently from unknown VLANs wrt. filtering. So .. would you be interested in implementing this properly? I think its a good change and I could help you if needed or take care of some parts like the drivers myself. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2008-10-09 at 07:31 -0500, Patrick McHardy wrote: > Jarod Neuner wrote: > > My original thought was to do something like this in net/core/dev.c > > using a method similar to handle_bridge or handle_macvlen. So, if the > > packet doesn't get handled by the ptype_base list and IFF_ALLVLAN is > > set, then strip the header and let the packet through. The sticky point > > would be whether or not this policy should be enabled by default, as it > > seems to be in other network stacks. > > I don't think we should change the default, it would probably > catch some people by surprise. It might not be handled properly > by packet filtering rules etc. On the other hand, I was surprised that VLAN packets were being dropped altogether. Net admins tend to assign a link to a particular VLAN with little regard to the VLAN configuration of the hosts on that link. I'm thinking of two general situations: 1) If the kernel is resident on an application device (PC, Multimedia Device, SOHO Router, etc.), and a packet for a particular VLAN reaches the network interface with a correct MAC and a correct IP, then they were probably delivered correctly, whether that host is configured with that VLAN ID or even if the VLAN module is loaded. 2) If the kernel is configured to route incoming VLAN packets, and a packet arrives with an unconfigured VLAN ID, then it seems perfectly reasonable to route it as if it had no VLAN tag. I'm sure someone has a setup that expects that foreign VLANs will be dropped - but I suspect far more are generally indifferent to the policy. There might even be a handful that will be pleasantly surprised when IGMP snooping suddenly starts to work. > > Most switches treat VLAN 1 as the "Default" or "Administration" VLAN. > > It might make sense to map VLAN 1 to the incoming interface, and then > > use that as a catch all. Then again, that might be a terrible idea as > > well. =) > > I prefer 0xfff because its not used for anything else so far. > Especially the administrative VLAN (even if only by convention) > doesn't seem by a good idea because its pretty likely you > would treat it differently from unknown VLANs wrt. filtering. Agreed. > So .. would you be interested in implementing this properly? > I think its a good change and I could help you if needed or > take care of some parts like the drivers myself. I've got quite a bit on my plate at the moment, but I will give it a shot. I'll try to come up with some of the IFF_ALLVLAN functionality over the next few days and get back to you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jarod Neuner wrote: > On Thu, 2008-10-09 at 07:31 -0500, Patrick McHardy wrote: >> I don't think we should change the default, it would probably >> catch some people by surprise. It might not be handled properly >> by packet filtering rules etc. > > On the other hand, I was surprised that VLAN packets were being dropped > altogether. Net admins tend to assign a link to a particular VLAN with > little regard to the VLAN configuration of the hosts on that link. I'm > thinking of two general situations: > > 1) If the kernel is resident on an application device (PC, Multimedia > Device, SOHO Router, etc.), and a packet for a particular VLAN reaches > the network interface with a correct MAC and a correct IP, then they > were probably delivered correctly, whether that host is configured with > that VLAN ID or even if the VLAN module is loaded. > > 2) If the kernel is configured to route incoming VLAN packets, and a > packet arrives with an unconfigured VLAN ID, then it seems perfectly > reasonable to route it as if it had no VLAN tag. > > I'm sure someone has a setup that expects that foreign VLANs will be > dropped - but I suspect far more are generally indifferent to the > policy. There might even be a handful that will be pleasantly surprised > when IGMP snooping suddenly starts to work. Possible, but besides the fact that this has been our default since even before VLAN was merged, a reason why this absolutely has to be manually enabled is that it requires to disable hardware filters for consistent, driver-independant behaviour. >> So .. would you be interested in implementing this properly? >> I think its a good change and I could help you if needed or >> take care of some parts like the drivers myself. > > I've got quite a bit on my plate at the moment, but I will give it a > shot. I'll try to come up with some of the IFF_ALLVLAN functionality > over the next few days and get back to you. Great, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jarod Neuner <j.neuner@networkharbor.com> writes: > My original thought was to do something like this in net/core/dev.c > using a method similar to handle_bridge or handle_macvlen. So, if the > packet doesn't get handled by the ptype_base list and IFF_ALLVLAN is > set, then strip the header and let the packet through. The sticky point > would be whether or not this policy should be enabled by default, as it > seems to be in other network stacks. So if I tcpdump on the base device, IFF_ALLVLAN gets set and packets from unknown VLAN's suddenly lose their VLAN headers? That's a security problem. /Benny -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 4bf014e..b65a8fd 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -158,9 +158,13 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev, rcu_read_lock(); skb->dev = __find_vlan_dev(dev, vlan_id); if (!skb->dev) { - pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n", + pr_debug("%s: WARNING: Forwarding VID: %u to dev: %s\n", __func__, vlan_id, dev->name); - goto err_unlock; + skb->dev = dev; + skb_pull_rcsum(skb, VLAN_HLEN); + vlan_set_encap_proto(skb, vhdr); + rcu_read_lock(); + return 0; } skb->dev->last_rx = jiffies;