| Message ID | 1480462253-114713-2-git-send-email-jarno@ovn.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Nov 29, 2016 at 3:30 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > Use is_skb_forwardable() instead of an explicit length check. This > gets around the apparent MTU check failure in OVS test cases when > skb->protocol is not properly set in case of non-accelerated VLAN > skbs. > > Suggested-by: Pravin Shelar <pshelar@ovn.org> > Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets") > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > --- > v3: New patch suggested by Pravin. > > net/openvswitch/vport.c | 38 ++++++++++++++------------------------ > 1 file changed, 14 insertions(+), 24 deletions(-) > > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index b6c8524..076b39f 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) > { > - int mtu = vport->dev->mtu; > - > switch (vport->dev->type) { > case ARPHRD_NONE: > if (mac_proto == MAC_PROTO_ETHERNET) { > @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) > goto drop; > } > > - if (unlikely(packet_length(skb, vport->dev) > mtu && > - !skb_is_gso(skb))) { > - net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", > - vport->dev->name, > - packet_length(skb, vport->dev), mtu); > + if (unlikely(!is_skb_forwardable(vport->dev, skb))) { This would easy to read if you inverse the if condition here. > + /* Log only if the device is up. */ > + if (vport->dev->flags & IFF_UP) { since is_skb_forwardable() is checking for IFF_UP we can remove same check from internal-device send() op. > + unsigned int length = skb->len > + - vport->dev->hard_header_len; > + > + if (!skb_vlan_tag_present(skb) > + && eth_type_vlan(skb->protocol)) > + length -= VLAN_HLEN; > + > + net_warn_ratelimited("%s: dropped over-mtu packet %d > %d\n", > + vport->dev->name, length, > + vport->dev->mtu); > + } > vport->dev->stats.tx_errors++; > goto drop; > } > -- > 2.1.4 >
On Tue, 29 Nov 2016 15:30:52 -0800, Jarno Rajahalme wrote: > @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) > goto drop; > } > > - if (unlikely(packet_length(skb, vport->dev) > mtu && > - !skb_is_gso(skb))) { > - net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", > - vport->dev->name, > - packet_length(skb, vport->dev), mtu); > + if (unlikely(!is_skb_forwardable(vport->dev, skb))) { How does this work when the vlan tag is accelerated? Then we can be over MTU, yet the check will pass. Jiri
> On Nov 30, 2016, at 5:51 AM, Jiri Benc <jbenc@redhat.com> wrote: > > On Tue, 29 Nov 2016 15:30:52 -0800, Jarno Rajahalme wrote: >> @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) >> goto drop; >> } >> >> - if (unlikely(packet_length(skb, vport->dev) > mtu && >> - !skb_is_gso(skb))) { >> - net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", >> - vport->dev->name, >> - packet_length(skb, vport->dev), mtu); >> + if (unlikely(!is_skb_forwardable(vport->dev, skb))) { > > How does this work when the vlan tag is accelerated? Then we can be > over MTU, yet the check will pass. > I’ll check how other call sites use this. Jarno > Jiri
On Wed, Nov 30, 2016 at 5:51 AM, Jiri Benc <jbenc@redhat.com> wrote: > On Tue, 29 Nov 2016 15:30:52 -0800, Jarno Rajahalme wrote: >> @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) >> goto drop; >> } >> >> - if (unlikely(packet_length(skb, vport->dev) > mtu && >> - !skb_is_gso(skb))) { >> - net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", >> - vport->dev->name, >> - packet_length(skb, vport->dev), mtu); >> + if (unlikely(!is_skb_forwardable(vport->dev, skb))) { > > How does this work when the vlan tag is accelerated? Then we can be > over MTU, yet the check will pass. > This is not changing any behavior compared to current OVS vlan checks. Single vlan header is not considered for MTU check.
On Thu, 1 Dec 2016 11:50:00 -0800, Pravin Shelar wrote: > This is not changing any behavior compared to current OVS vlan checks. > Single vlan header is not considered for MTU check. It is changing it. Consider the case when there's an interface with MTU 1500 forwarding to an interface with MTU 1496. Obviously, full-sized vlan frames ingressing on the first interface are not forwardable to the second one. Yet, if the vlan tag is accelerated (and thus not counted in skb->len), is_skb_forwardable happily returns true because of the check len = dev->mtu + dev->hard_header_len + VLAN_HLEN; if (skb->len <= len) Jiri
On Fri, Dec 2, 2016 at 1:25 AM, Jiri Benc <jbenc@redhat.com> wrote: > On Thu, 1 Dec 2016 11:50:00 -0800, Pravin Shelar wrote: >> This is not changing any behavior compared to current OVS vlan checks. >> Single vlan header is not considered for MTU check. > > It is changing it. > > Consider the case when there's an interface with MTU 1500 forwarding to > an interface with MTU 1496. Obviously, full-sized vlan frames > ingressing on the first interface are not forwardable to the second > one. Yet, if the vlan tag is accelerated (and thus not counted in > skb->len), is_skb_forwardable happily returns true because of the check > > len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > if (skb->len <= len) > ok, This case would be allowed due to this patch. But core linux stack and bridge is using this check then why not just use same forwarding check in OVS too, this make it consistent with core networking forwarding expectations.
On Sun, Dec 04, 2016 at 04:22:40PM -0800, Pravin Shelar wrote: > On Fri, Dec 2, 2016 at 1:25 AM, Jiri Benc <jbenc@redhat.com> wrote: > > On Thu, 1 Dec 2016 11:50:00 -0800, Pravin Shelar wrote: > >> This is not changing any behavior compared to current OVS vlan checks. > >> Single vlan header is not considered for MTU check. > > > > It is changing it. > > > > Consider the case when there's an interface with MTU 1500 forwarding to > > an interface with MTU 1496. Obviously, full-sized vlan frames > > ingressing on the first interface are not forwardable to the second > > one. Yet, if the vlan tag is accelerated (and thus not counted in > > skb->len), is_skb_forwardable happily returns true because of the check > > > > len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > > if (skb->len <= len) > > > ok, This case would be allowed due to this patch. But core linux stack > and bridge is using this check then why not just use same forwarding > check in OVS too, this make it consistent with core networking > forwarding expectations. Should we not also follow the "skbs are untagged" approach that the rest of the kernel uses? I'm referring to patches 1 and 2 form Jiri's series "openvswitch: make vlan handling consistent". With those changes is_skb_forwardable() would behave as expected here.
On Thu, 8 Dec 2016 15:50:41 -0500, Eric Garver wrote: > Should we not also follow the "skbs are untagged" approach that the rest > of the kernel uses? I'm referring to patches 1 and 2 form Jiri's series > "openvswitch: make vlan handling consistent". > > With those changes is_skb_forwardable() would behave as expected here. Yes, this would make the check easy and consistent (and was actually my original motivation for the mentioned patchset). Still, is_skb_forwardable would be off by 4 bytes. I wonder whether it's not off even for the bridge case. And dev_forward_skb seems to be fishy, too. Jiri
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c index b6c8524..076b39f 100644 --- a/net/openvswitch/vport.c +++ b/net/openvswitch/vport.c @@ -464,27 +464,8 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb, return 0; } -static unsigned int packet_length(const struct sk_buff *skb, - struct net_device *dev) -{ - unsigned int length = skb->len - dev->hard_header_len; - - if (!skb_vlan_tag_present(skb) && - eth_type_vlan(skb->protocol)) - length -= VLAN_HLEN; - - /* Don't subtract for multiple VLAN tags. Most (all?) drivers allow - * (ETH_LEN + VLAN_HLEN) in addition to the mtu value, but almost none - * account for 802.1ad. e.g. is_skb_forwardable(). - */ - - return length; -} - void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) { - int mtu = vport->dev->mtu; - switch (vport->dev->type) { case ARPHRD_NONE: if (mac_proto == MAC_PROTO_ETHERNET) { @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) goto drop; } - if (unlikely(packet_length(skb, vport->dev) > mtu && - !skb_is_gso(skb))) { - net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", - vport->dev->name, - packet_length(skb, vport->dev), mtu); + if (unlikely(!is_skb_forwardable(vport->dev, skb))) { + /* Log only if the device is up. */ + if (vport->dev->flags & IFF_UP) { + unsigned int length = skb->len + - vport->dev->hard_header_len; + + if (!skb_vlan_tag_present(skb) + && eth_type_vlan(skb->protocol)) + length -= VLAN_HLEN; + + net_warn_ratelimited("%s: dropped over-mtu packet %d > %d\n", + vport->dev->name, length, + vport->dev->mtu); + } vport->dev->stats.tx_errors++; goto drop; }
Use is_skb_forwardable() instead of an explicit length check. This gets around the apparent MTU check failure in OVS test cases when skb->protocol is not properly set in case of non-accelerated VLAN skbs. Suggested-by: Pravin Shelar <pshelar@ovn.org> Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets") Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- v3: New patch suggested by Pravin. net/openvswitch/vport.c | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-)