| 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
| ||
|
Date: Thu, 10 Oct 2013 09:31:41 +0900
From: Simon Horman <horms@...ge.net.au>
To: dev@...nvswitch.org, netdev@...r.kernel.org,
Jesse Gross <jesse@...ira.com>, Ben Pfaff <blp@...ira.com>
Cc: Pravin B Shelar <pshelar@...ira.com>, Ravi K <rkerur@...il.com>,
Isaku Yamahata <yamahata@...inux.co.jp>,
Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH v2.43 0/5] MPLS actions and matches
On Mon, Oct 07, 2013 at 05:00:42PM +0900, Simon Horman wrote:
> Hi,
>
> This series implements MPLS actions and matches based on work by
> Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.
>
> This series provides two changes
>
> * Patches 1 - 3
>
> Provide user-space support for the VLAN/MPLS tag insertion order
> up to and including OpenFlow 1.2, and the different ordering
> specified from OpenFlow 1.3. In a nutshell the datapath always
> uses the OpenFlow 1.3 ordering, which is to always insert tags
> immediately after the L2 header, regardless of the presence of other
> tags. And ovs-vswtichd provides compatibility for the behaviour up
> to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
> if present.
>
> These patches have been updated since v2.42.
>
> Ben, these are for you to review.
>
> * Patches 4 and 5
>
> Adding basic MPLS action and match support to the kernel datapath
>
> These patches were last updated in v2.41.
>
> Jesse, these are for you to review.
Hi Jesse, Pravin and Ben,
I believe this series addresses the feedback that each of you
gave with regards to recent previous postings of this patch-set.
I'm wondering if you could find some time to review it.
>
>
> Differences between v2.43 and v2.42:
>
> * As suggested by Ben Pfaff
> Move vlan state into struct xlate_ctx
> 1. Add final_vlan_tci member to struct xlate_ctx instead of vlan_tci member
> struct xlate_xin. This seems to be a better palace for it as it does
> not need to be accessible from the caller.
> 2. Move local vlan_tci variable of do_xlate_actions() to be the
> next_vlan_tci member of strict xlate_ctx. This allows for it to work
> across resubmit actions and goto table instructions.
> * Code contributed by Ben Pfaff
> + Use enum for to control order of MPLS LSE insertion
> This makes the logic somewhat clearer
> * Add a helper push_mpls_from_openflow() to consolidate
> the same logic that appears in three locations.
>
>
> Differences between v2.42 and v2.41:
>
> * Rebase for:
> + 0585f7a ("datapath: Simplify mega-flow APIs.")
> + a097c0b ("datapath: Restructure datapath.c and flow.c")
> * As suggested by Jesse Gross
> + Take into account that push_mpls() will have freed the skb on error
> + Remove dubious !eth_p_mpls(skb->protocol) condition from push_mpls
> The !eth_p_mpls(skb->protocol) condition on setting inner_protocol
> has no effect. Its motivation was to ensure that inner_protocol was
> only set the first time that mpls_push occured. However this is already
> ensured by the !ovs_skb_get_inner_protocol(skb) condition.
> + Return -EINVAL instead of -ENOMEM from pop_mpls() if the skb is too short
> + Do not add @inner_protocol to kernel doc for struct ovs_skb_cb.
> The patch no longer adds an inner_protocol member to struct ovs_skb_cb
> + Do not add and set otherwise unsued inner_protocol variable in
> rpl_dev_queue_xmit()
> * As suggested by Pravin Shelar
> + Implement compatibility code in existing rpl_skb_gso_segment
> rather than introducing to use rpl___skb_gso_segment
>
>
> Differences between v2.41 and v2.40:
>
> * As suggested by Ben Pfaff
> + Expand struct ofpact_reg_load to include a mpls_before_vlan field
> rather than using the compat field of the ofpact field of
> struct ofpact_reg_load.
> + Pass version to ofpacts_pull_openflow11_actions and
> ofpacts_pull_openflow11_instructions. This removes the invalid
> assumption that that these functions are passed a full message and are
> thus able to deduce the OpenFlow version.
>
>
> Differences between v2.40 and v2.39:
>
> * Rebase for:
> + New dev_queue_xmit compat code
> + Updated put_vlan()
> + Removal of mpls_depth field from struct flow
> * As suggested by Jesse Gross
> + Remove bogus mac_len update from push_mpls()
> + Slightly simplify push_mpls() by using eth_hdr()
> + Remove dubious condition !eth_p_mpls(inner_protocol) on
> an skb being considered to be MPLS in netdev_send()
> + Only use compatibility code for MPLS GSO segmentation on kernels
> older than 3.11
> + Revamp setting of inner_protocol
> 1. Do not unconditionally set inner_protocol to the value of
> skb->protocol in ovs_execute_actions().
> 2. Initialise inner_protocol it to zero only if compatibility code is in
> use. In the case where compatibility code is not in use it will either
> be zero due since the allocation of the skb or some other value set
> by some other user.
> 3. Conditionally set the inner_protocol in push_mpls() to the value of
> skb->protocol when entering push_mpls(). The condition is that
> inner_protocol is zero and the value of skb->protocol is not an MPLS
> ethernet type.
> - This new scheme:
> + Pushes logic to set inner_protocol closer to the case where it is
> needed.
> + Avoids over-writing values set by other users.
> * As suggested by Pravin Shelar
> + Only set and restore skb->protocol in rpl___skb_gso_segment() in the
> case of MPLS
> + Add inner_protocol field to struct ovs_gso_cb instead of ovs_skb_cb.
> This moves compatibility code closer to where it is used
> and creates fewer differences with mainline.
> * Update comment on mac_len updates in datapath/actions.c
> * Remove HAVE_INNER_PROCOTOL and instead just check
> against kernel version 3.11 directly.
> HAVE_INNER_PROCOTOL is a hang-over from work done prior
> to the merge of inner_protocol into the kernel.
> * Remove dubious condition !eth_p_mpls(inner_protocol) on
> using inner_protocol as the type in rpl_skb_network_protocol()
> * Do not update type of features in rpl_dev_queue_xmit.
> Though arguably correct this is not an inherent part of
> the changes made by this patch.
> * Use skb_cow_head() in push_mpls()
> + Call skb_cow_head(skb, MPLS_HLEN) instead of
> make_writable(skb, skb->mac_len) to ensure that there is enough head
> room to push an MPLS LSE regardless of whether the skb is cloned or not.
> + This is consistent with the behaviour of rpl__vlan_put_tag().
> + This is a fix for crashes reported when performing mpls_push
> with headroom less than 4. This problem was introduced in v3.36.
> * Skip popping in mpls_pop if the skb is too short to contain an MPLS LSE
>
>
> Differences between v2.39 and v2.38:
>
> * Rebase for removal of vlan, checksum and skb->mark compat code
> - This includes adding adding a new patch,
> "[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
> vlan_push" to allow re-use of some existing code.
>
>
> Differences between v2.38 and v2.37:
>
> * Rebase for SCTP support
> * Refactor validate_tp_port() to iterate over eth_types rather
> than open-coding the loop. With the addition of SCTP this logic
> is now used three times.
>
>
> Differences between v2.37 and v2.36:
>
> * Rebase
>
>
> Differences between v2.36 and v2.35:
>
> * Rebase
>
> * Do not add set_ethertype() to datapath/actions.c.
> As this patch has evolved this function had devolved into
> to sets of functionality wrapped into a single function with
> only one line of common code. Refactor things to simply
> open-code setting the ether type in the two locations where
> set_ethertype() was previously used. The aim here is to improve
> readability.
>
> * Update setting skb->ethertype after mpls push and pop.
> - In the case of push_mpls it should be set unconditionally
> as in v2.35 the behaviour of this function to always push
> an MPLS LSE before any VLAN tags.
> - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
> test than skb->protocol != htons(ETH_P_8021Q) as it will give the
> correct behaviour in the presence of other VLAN ethernet types,
> for example 0x88a8 which is used by 802.1ad. Moreover, it seems
> correct to update the ethernet type if it was previously set
> according to the top-most MPLS LSE.
>
> * Deaccelerate VLANs when pushing MPLS tags the
> - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
> This means that if an accelerated tag is present it should be
> deaccelerated to ensure it ends up in the correct position.
>
> * Update skb->mac_len in push_mpls() so that it will be correct
> when used by a subsequent call to pop_mpls().
>
> As things stand I do not believe this is strictly necessary as
> ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
> However, I have added this in order to code more defensively as I believe
> that if such a sequence did occur it would be rather unobvious why
> it didn't work.
>
> * Do not add skb_cow_head() call in push_mpls().
> It is unnecessary as there is a make_writable() call.
> This change was also made in v2.30 but some how the
> code regressed between then and v2.35.
>
>
> Differences between v2.35 and v2.34:
>
> * Add support for the tag ordering specified up until OpenFlow 1.2 and
> the ordering specified from OpenFlow 1.3.
>
> * Correct error in datapath patch's handling of GSO in the presence
> of MPLS and absence of VLANs.
>
>
> To aid review this series is available in git at:
>
> git://github.com/horms/openvswitch.git devel/mpls-v2.43
>
>
> Patch list and overall diffstat:
>
> Joe Stringer (3):
> odp: Allow VLAN actions after MPLS actions
> ofp-actions: Add separate OpenFlow 1.3 action parser
> lib: Support pushing of MPLS LSE before or after VLAN tag
>
> Simon Horman (2):
> datapath: Break out deacceleration portion of vlan_push
> datapath: Add basic MPLS support to kernel
>
> datapath/Modules.mk | 1 +
> datapath/actions.c | 158 ++++++++-
> datapath/datapath.c | 4 +-
> datapath/flow.c | 29 ++
> datapath/flow.h | 17 +-
> datapath/flow_netlink.c | 286 +++++++++++++--
> datapath/flow_netlink.h | 2 +-
> datapath/linux/compat/gso.c | 70 +++-
> datapath/linux/compat/gso.h | 41 +++
> datapath/linux/compat/include/linux/netdevice.h | 6 +-
> datapath/linux/compat/netdevice.c | 10 +-
> datapath/mpls.h | 15 +
> include/linux/openvswitch.h | 7 +-
> lib/flow.c | 2 +-
> lib/odp-util.c | 9 +-
> lib/odp-util.h | 2 +-
> lib/ofp-actions.c | 91 ++++-
> lib/ofp-actions.h | 16 +
> lib/ofp-print.c | 2 +-
> lib/ofp-util.c | 24 +-
> lib/ofp-util.h | 2 +-
> lib/packets.c | 10 +-
> lib/packets.h | 2 +-
> ofproto/ofproto-dpif-xlate.c | 132 +++++--
> tests/ofproto-dpif.at | 446 ++++++++++++++++++++++++
> utilities/ovs-ofctl.c | 8 +-
> 26 files changed, 1272 insertions(+), 120 deletions(-)
> create mode 100644 datapath/mpls.h
> --
> 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
>
--
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