| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130918220756.GA24919@verge.net.au>
Date: Wed, 18 Sep 2013 17:07:59 -0500
From: Simon Horman <horms@...ge.net.au>
To: Pravin Shelar <pshelar@...ira.com>
Cc: "dev@...nvswitch.org" <dev@...nvswitch.org>,
netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
Isaku Yamahata <yamahata@...inux.co.jp>,
Jesse Gross <jesse@...ira.com>, Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@...ge.net.au> wrote:
> > Allow datapath to recognize and extract MPLS labels into flow keys
> > and execute actions which push, pop, and set labels on packets.
> >
> > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> >
> > Cc: Ravi K <rkerur@...il.com>
> > Cc: Leo Alterman <lalterman@...ira.com>
> > Cc: Isaku Yamahata <yamahata@...inux.co.jp>
> > Cc: Joe Stringer <joe@...d.net.nz>
> > Signed-off-by: Simon Horman <horms@...ge.net.au>
> >
> > ---
> ....
> > diff --git a/datapath/datapath.h b/datapath/datapath.h
> > index 5d50dd4..babae3b 100644
> > --- a/datapath/datapath.h
> > +++ b/datapath/datapath.h
> > @@ -36,6 +36,10 @@
> >
> > #define SAMPLE_ACTION_DEPTH 3
> >
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> > +#define HAVE_INNER_PROTOCOL
> > +#endif
> > +
> > /**
> > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> > * datapath.
> > @@ -93,11 +97,16 @@ struct datapath {
> > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> > * packet is not being tunneled.
> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> > + * kernels before 3.11.
> > */
> > struct ovs_skb_cb {
> > struct sw_flow *flow;
> > struct sw_flow_key *pkt_key;
> > struct ovs_key_ipv4_tunnel *tun_key;
> > +#ifndef HAVE_INNER_PROTOCOL
> > + __be16 inner_protocol;
> > +#endif
> > };
> > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
> >
> Can you move this to compat struct ovs_gso_cb {}
I think that you are correct and inner_protocol needs
to be in struct ovs_gso_cb so that it can
be accessed via skb_network_protocol() from
rpl___skb_gso_segment().
However I think it may also need to be present in struct ovs_cb
so that it can be set correctly.
Currently it is set unconditionally
in ovs_execute_actions() and Jesse has suggested setting
it conditionally in pop_mpls() (which is called by do_execute_actions()).
But regardless it seems to me that the field would need to be available
in struct ovs_cb.
>
> ....
> >
> > +struct sk_buff *rpl___skb_gso_segment(struct sk_buff *skb,
> > + netdev_features_t features,
> > + bool tx_path)
> > +{
> > + struct sk_buff *skb_gso;
> > + __be16 type = skb->protocol;
> > +
> > + skb->protocol = skb_network_protocol(skb);
> > +
> > + /* this hack needed to get regular skb_gso_segment() */
> > +#ifdef HAVE___SKB_GSO_SEGMENT
> > +#undef __skb_gso_segment
> > + skb_gso = __skb_gso_segment(skb, features, tx_path);
> > +#else
> > +#undef skb_gso_segment
> > + skb_gso = skb_gso_segment(skb, features);
> > +#endif
> > +
> > + if (!skb_gso || IS_ERR(skb_gso))
> > + return skb_gso;
> > +
> > + skb = skb_gso;
> > + while (skb) {
> > + skb->protocol = type;
> > + skb = skb->next;
> > + }
> > +
>
> Protocol set is required if there is MPLS header, which is rare case.
> So I think we can skip this loop if there is no mpls.
I'm not sure I see the benefit but I do agree with your analysis and
I'll make it so.
>
> > + return skb_gso;
> > +}
> > +
> > +struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb,
> .....
> > +
> > + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
> > + vlan = true;
> > +
> > + if (vlan || mpls) {
> > + netdev_features_t features;
> >
> > features = netif_skb_features(skb);
> >
> > @@ -296,6 +309,20 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
> > NETIF_F_UFO | NETIF_F_FSO);
> >
> > + /* As of v3.11 the kernel provides an mpls_features field in
> > + * struct net_device which allows devices to advertise which
> > + * features its supports for MPLS. This value defaults to
> > + * NETIF_F_SG and as of v3.11.
> > + *
> > + * This compatibility code is intended for kernels older
> > + * than v3.11 that do not support MPLS GSO and thus do not
> > + * provide mpls_features. Thus this code uses NETIF_F_SG
> > + * directly in place of mpls_features.
> > + */
> > +
> > + if (mpls)
> > + features &= NETIF_F_SG;
> > +
> > if (netif_needs_gso(skb, features)) {
> > struct sk_buff *nskb;
> >
> > @@ -319,10 +346,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > nskb = skb->next;
> > skb->next = NULL;
> >
> > - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > + if (vlan)
> > + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > if (likely(skb)) {
> > len += skb->len;
> > - vlan_set_tci(skb, 0);
> > + if (vlan)
> > + vlan_set_tci(skb, 0);
> > dev_queue_xmit(skb);
> > }
> >
> > @@ -333,10 +362,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > }
> >
> > tag:
> > - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > - if (unlikely(!skb))
> > - return 0;
> > - vlan_set_tci(skb, 0);
> > + if (vlan) {
> > + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > + if (unlikely(!skb))
> > + return 0;
> > + vlan_set_tci(skb, 0);
> > + }
> > }
> >
> I think we can simplify code by pushing vlan and then segmenting skb,
> the way we do it for MPLS.
Are you thinking of something like the following which applies
prior to the MPLS code.
[RFC] datapath: simplify VLAN segmentation
*** Do not apply: for informational purposes only
Push vlan tag onto packet before segmentation to simplify the code.
As suggested by Pravin Shelar.
Compile tested only.
Signed-off-by: Simon Horman <horms@...ge.net.au>
---
datapath/vport-netdev.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index 215a47e..31680fd 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -296,6 +296,11 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
NETIF_F_UFO | NETIF_F_FSO);
+ skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
+ if (unlikely(!skb))
+ return 0;
+ vlan_set_tci(skb, 0);
+
if (netif_needs_gso(skb, features)) {
struct sk_buff *nskb;
@@ -306,7 +311,7 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
goto drop;
skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
- goto tag;
+ goto xmit;
}
if (IS_ERR(nskb))
@@ -318,27 +323,16 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
do {
nskb = skb->next;
skb->next = NULL;
-
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (likely(skb)) {
- len += skb->len;
- vlan_set_tci(skb, 0);
- dev_queue_xmit(skb);
- }
-
+ len += skb->len;
+ dev_queue_xmit(skb);
skb = nskb;
} while (skb);
return len;
}
-
-tag:
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (unlikely(!skb))
- return 0;
- vlan_set_tci(skb, 0);
}
+xmit:
len = skb->len;
dev_queue_xmit(skb);
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists