From dd852aa7a8e6eb99cb677851a7c55eb33bf64cb2 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Tue, 5 Nov 2024 13:06:59 +0100 Subject: [PATCH 1/5] build: add support for cross-compiled rpms (#4649) This is enabled by a pending PR to the rules_pkg project: https://github.com/bazelbuild/rules_pkg/pull/729 I just added the patch to our build while we wait for the PR to get merged. --- .buildkite/pipeline.yml | 6 + WORKSPACE | 7 +- dist/BUILD.bazel | 3 + dist/package.bzl | 6 + dist/rpm/rpm_rules.patch | 120 +++++++++++++++++++ licenses/data/platforms/LICENSE | 201 ++++++++++++++++++++++++++++++++ 6 files changed, 340 insertions(+), 3 deletions(-) create mode 100644 dist/rpm/rpm_rules.patch create mode 100644 licenses/data/platforms/LICENSE diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index ca8d28325e..926c255dee 100644 --- a/.buildkite/pipeline.yml +++ b/.buildkite/pipeline.yml @@ -44,6 +44,9 @@ steps: tar -chaf scion_${SCION_VERSION}_deb_armel.tar.gz *_${SCION_VERSION}_armel.deb tar -chaf scion_${SCION_VERSION}_openwrt_x86_64.tar.gz *_${SCION_VERSION}_x86_64.ipk tar -chaf scion_${SCION_VERSION}_rpm_x86_64.tar.gz *_${SCION_VERSION}_x86_64.rpm + tar -chaf scion_${SCION_VERSION}_rpm_arm64.tar.gz *_${SCION_VERSION}_x86_64.rpm + tar -chaf scion_${SCION_VERSION}_rpm_i386.tar.gz *_${SCION_VERSION}_x86_64.rpm + tar -chaf scion_${SCION_VERSION}_rpm_armel.tar.gz *_${SCION_VERSION}_x86_64.rpm popd ls installables post-artifact: | @@ -57,6 +60,9 @@ steps: - x86_64 #### Packages :rpm: - x86_64 + - arm64 + - i386 + - armel EOF key: dist retry: *automatic-retry diff --git a/WORKSPACE b/WORKSPACE index 1bd17a7c1a..d001a454fa 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -122,10 +122,11 @@ install_python_doc_deps() http_archive( name = "rules_pkg", - sha256 = "8f9ee2dc10c1ae514ee599a8b42ed99fa262b757058f65ad3c384289ff70c4b8", + patch_args = ["-p1"], + patches = ["@//dist:rpm/rpm_rules.patch"], + sha256 = "d250924a2ecc5176808fc4c25d5cf5e9e79e6346d79d5ab1c493e289e722d1d0", urls = [ - "https://mirror.bazel.build/github.com/bazelbuild/rules_pkg/releases/download/0.9.1/rules_pkg-0.9.1.tar.gz", - "https://github.com/bazelbuild/rules_pkg/releases/download/0.9.1/rules_pkg-0.9.1.tar.gz", + "https://github.com/bazelbuild/rules_pkg/releases/download/0.10.1/rules_pkg-0.10.1.tar.gz", ], ) diff --git a/dist/BUILD.bazel b/dist/BUILD.bazel index 221e68638b..5711181308 100644 --- a/dist/BUILD.bazel +++ b/dist/BUILD.bazel @@ -11,6 +11,9 @@ DEB_PLATFORMS = [ RPM_PLATFORMS = [ "@io_bazel_rules_go//go/toolchain:linux_amd64", + "@io_bazel_rules_go//go/toolchain:linux_arm64", + "@io_bazel_rules_go//go/toolchain:linux_386", + "@io_bazel_rules_go//go/toolchain:linux_arm", ] # TODO(jice@scion.org): diff --git a/dist/package.bzl b/dist/package.bzl index 95a4c48f67..cd4c63af54 100644 --- a/dist/package.bzl +++ b/dist/package.bzl @@ -113,6 +113,10 @@ def scion_pkg_rpm(name, package, executables = {}, systemds = [], configs = [], else: deps = [] + tarch = kwargs.get("architecture") + if tarch: + kwargs.pop("architecture") + post = kwargs.get("postinst") if post: kwargs.pop("postinst") @@ -122,11 +126,13 @@ def scion_pkg_rpm(name, package, executables = {}, systemds = [], configs = [], summary = kwargs["description"], srcs = ["%s_configs" % name, "%s_systemds" % name, "%s_execs" % name], target_compatible_with = ["@platforms//os:linux"], + target_architecture = tarch, package_file_name = "{package}_{file_name_version}_{architecture}.rpm", package_variables = ":package_file_naming_" + name, package_name = package, release = "%autorelease", version_file = ":%s_version" % name, + defines = {"_smp_build_ncpus": "1"}, requires = deps, post_scriptlet_file = post, **kwargs diff --git a/dist/rpm/rpm_rules.patch b/dist/rpm/rpm_rules.patch new file mode 100644 index 0000000000..a00fd61248 --- /dev/null +++ b/dist/rpm/rpm_rules.patch @@ -0,0 +1,120 @@ +The following patch comes from: +https://github.com/bazelbuild/rules_pkg/pull/729 +Ownership as per the github project's provisions. + +I provide it here in advance of it being merged because it might take a while +to happen. + +From 6c27a34cfe5a37901803ad8478f3b9ec668a3b69 Mon Sep 17 00:00:00 2001 +From: Alex Blago +Date: Sun, 13 Aug 2023 00:33:00 -0700 +Subject: [PATCH] Support for cross-platform RPM package generation +diff --git a/pkg/make_rpm.py b/pkg/make_rpm.py +index e2ffca0a..76a2e51d 100644 +--- a/pkg/make_rpm.py ++++ b/pkg/make_rpm.py +@@ -178,13 +178,14 @@ class RpmBuilder(object): + RPMS_DIR = 'RPMS' + DIRS = [SOURCE_DIR, BUILD_DIR, RPMS_DIR, TEMP_DIR] + +- def __init__(self, name, version, release, arch, rpmbuild_path, +- source_date_epoch=None, ++ def __init__(self, name, version, release, arch, target_arch, ++ rpmbuild_path, source_date_epoch=None, + debug=False): + self.name = name + self.version = helpers.GetFlagValue(version) + self.release = helpers.GetFlagValue(release) + self.arch = arch ++ self.target_arch = target_arch + self.files = [] + self.rpmbuild_path = FindRpmbuild(rpmbuild_path) + self.rpm_path = None +@@ -354,6 +355,10 @@ def CallRpmBuild(self, dirname, rpmbuild_args): + '--buildroot=%s' % buildroot, + ] # yapf: disable + ++ # Target platform ++ if self.target_arch: ++ args += ['--target=%s' % self.target_arch] ++ + # Macro-based RPM parameter substitution, if necessary inputs provided. + if self.preamble_file: + args += ['--define', 'build_rpm_options %s' % self.preamble_file] +@@ -462,7 +467,10 @@ def main(argv): + help='The release of the software being packaged.') + parser.add_argument( + '--arch', +- help='The CPU architecture of the software being packaged.') ++ help='The CPU architecture of the machine on which it is built. ' ++ 'If the package is not architecture dependent, set this to "noarch".') ++ parser.add_argument('--target_arch', ++ help='The CPU architecture of the target platform the software being packaged for.') + parser.add_argument('--spec_file', required=True, + help='The file containing the RPM specification.') + parser.add_argument('--out_file', required=True, +@@ -501,7 +509,7 @@ def main(argv): + try: + builder = RpmBuilder(options.name, + options.version, options.release, +- options.arch, options.rpmbuild, ++ options.arch, options.target_arch, options.rpmbuild, + source_date_epoch=options.source_date_epoch, + debug=options.debug) + builder.AddFiles(options.files) +diff --git a/pkg/rpm_pfg.bzl b/pkg/rpm_pfg.bzl +index 1e3450c1..596dc26d 100644 +--- a/pkg/rpm_pfg.bzl ++++ b/pkg/rpm_pfg.bzl +@@ -251,7 +251,7 @@ def _pkg_rpm_impl(ctx): + rpm_name, + ctx.attr.version, + ctx.attr.release, +- ctx.attr.architecture, ++ ctx.attr.architecture if ctx.attr.architecture else ctx.attr.target_architecture, + ) + + _, output_file, _ = setup_output_files( +@@ -454,5 +454,8 @@ def _pkg_rpm_impl(ctx): + + args.append("--out_file=" + output_file.path) + ++ if ctx.attr.target_architecture: ++ args.append("--target_arch=" + ctx.attr.target_architecture) ++ + # Add data files + files += ctx.files.srcs +@@ -791,20 +794,29 @@ pkg_rpm = rule( + # funny if it's not provided. The contents of the RPM are believed to + # be set as expected, though. + "architecture": attr.string( +- doc = """Package architecture. ++ doc = """Host architecture. + + This currently sets the `BuildArch` tag, which influences the output + architecture of the package. + + Typically, `BuildArch` only needs to be set when the package is +- known to be cross-platform (e.g. written in an interpreted +- language), or, less common, when it is known that the application is +- only valid for specific architectures. ++ not architecture dependent (e.g. written in an interpreted ++ language). + + When no attribute is provided, this will default to your host's + architecture. This is usually what you want. + + """, + ), ++ "target_architecture": attr.string( ++ doc = """Package architecture. ++ ++ This currently sets the value for the "--target" argument to "rpmbuild" ++ to specify platform package is built for. ++ ++ When no attribute is provided, this will default to your host's ++ architecture. ++ """, ++ ), + "license": attr.string( + doc = """RPM "License" tag. diff --git a/licenses/data/platforms/LICENSE b/licenses/data/platforms/LICENSE new file mode 100644 index 0000000000..261eeb9e9f --- /dev/null +++ b/licenses/data/platforms/LICENSE @@ -0,0 +1,201 @@ + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + + 4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + + 5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + + 6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + + 7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + + 8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + + 9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + + END OF TERMS AND CONDITIONS + + APPENDIX: How to apply the Apache License to your work. + + To apply the Apache License to your work, attach the following + boilerplate notice, with the fields enclosed by brackets "[]" + replaced with your own identifying information. (Don't include + the brackets!) The text should be enclosed in the appropriate + comment syntax for the file format. We also recommend that a + file or class name and description of purpose be included on the + same "printed page" as the copyright notice for easier + identification within third-party archives. + + Copyright [yyyy] [name of copyright owner] + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. From 5c3b50dce3ba227579b1f18420c3e28790fc385d Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:40:17 +0100 Subject: [PATCH 2/5] router: small refactoring to remove the need for a batchconn.WriteTo (#4651) WriteTo needs to be given the destination address explicitly. WriteBatch, on the other end, can either find it in each packet structure, or rely on the connection's destination. WriteTo is only used to send BFD packets. It turns out that BFD packets can also very easily be sent via the regular forwarders that use WriteBtach. The motivation to do that is to simplify the interface between the dataplane and the forwarders, in view of supporting multiple underlays with possibly very different interfaces. So the channels around the processor tasks seems like a good universal interface. In passing, also removed a duplicate field. Slightly off-topic but still in the spirit of noise abatement. As this is to facilitate the necessary refactoring... Contributes to #4593 --- router/control/conf.go | 13 +++--- router/dataplane.go | 73 +++++++++++++++++++++++-------- router/dataplane_internal_test.go | 4 +- router/dataplane_test.go | 33 +++++++------- router/export_test.go | 3 +- router/mock_router/mock.go | 16 ------- 6 files changed, 81 insertions(+), 61 deletions(-) diff --git a/router/control/conf.go b/router/control/conf.go index fc8c0520f9..99c9859a8b 100644 --- a/router/control/conf.go +++ b/router/control/conf.go @@ -196,13 +196,14 @@ func confExternalInterfaces(dp Dataplane, cfg *Config) error { _, owned := cfg.BR.IFs[ifID] if !owned { - // XXX The current implementation effectively uses IP/UDP tunnels to create - // the SCION network as an overlay, with forwarding to local hosts being a special case. - // When setting up external interfaces that belong to other routers in the AS, they - // are basically IP/UDP tunnels between the two border routers, and as such is - // configured in the data plane. + // The current implementation effectively uses IP/UDP tunnels to create the SCION + // network as an overlay, with forwarding to local hosts being a special case. When + // setting up external interfaces that belong to other routers in the AS, they are + // basically IP/UDP tunnels between the two border routers. Those are described as a + // link from the internal address of the local router to the internal address of the + // sibling router; and not to the router in the remote AS. linkInfo.Local.Addr = cfg.BR.InternalAddr - linkInfo.Remote.Addr = iface.InternalAddr + linkInfo.Remote.Addr = iface.InternalAddr // i.e. via sibling router. // For internal BFD always use the default configuration. linkInfo.BFD = BFD{} } diff --git a/router/dataplane.go b/router/dataplane.go index 1701fcc8ab..c1322a5f4a 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -86,7 +86,6 @@ type bfdSession interface { // BatchConn is a connection that supports batch reads and writes. type BatchConn interface { ReadBatch(underlayconn.Messages) (int, error) - WriteTo([]byte, netip.AddrPort) (int, error) WriteBatch(msgs underlayconn.Messages, flags int) (int, error) Close() error } @@ -177,7 +176,6 @@ type DataPlane struct { linkTypes map[uint16]topology.LinkType neighborIAs map[uint16]addr.IA peerInterfaces map[uint16]uint16 - internal BatchConn internalIP netip.Addr internalNextHops map[uint16]netip.AddrPort svc *services @@ -193,6 +191,10 @@ type DataPlane struct { ExperimentalSCMPAuthentication bool + // The forwarding queues. Each is consumed by a forwarder process and fed by + // one bfd sender and the packet processors. + fwQs map[uint16]chan *packet + // The pool that stores all the packet buffers as described in the design document. See // https://github.com/scionproto/scion/blob/master/doc/dev/design/BorderRouter.rst // To avoid garbage collection, most the meta-data that is produced during the processing of a @@ -334,14 +336,12 @@ func (d *DataPlane) AddInternalInterface(conn BatchConn, ip netip.Addr) error { if conn == nil { return emptyValue } - if d.internal != nil { - return alreadySet - } if d.interfaces == nil { d.interfaces = make(map[uint16]BatchConn) + } else if d.interfaces[0] != nil { + return alreadySet } d.interfaces[0] = conn - d.internal = conn d.internalIP = ip return nil } @@ -361,7 +361,7 @@ func (d *DataPlane) AddExternalInterface(ifID uint16, conn BatchConn, if conn == nil || !src.Addr.IsValid() || !dst.Addr.IsValid() { return emptyValue } - err := d.addExternalInterfaceBFD(ifID, conn, src, dst, cfg) + err := d.addExternalInterfaceBFD(ifID, src, dst, cfg) if err != nil { return serrors.Wrap("adding external BFD", err, "if_id", ifID) } @@ -381,7 +381,7 @@ func (d *DataPlane) AddExternalInterface(ifID uint16, conn BatchConn, // AddNeighborIA adds the neighboring IA for a given interface ID. If an IA for // the given ID is already set, this method will return an error. This can only -// be called on a yet running dataplane. +// be called on a not yet running dataplane. func (d *DataPlane) AddNeighborIA(ifID uint16, remote addr.IA) error { d.mtx.Lock() defer d.mtx.Unlock() @@ -439,7 +439,7 @@ func (d *DataPlane) AddRemotePeer(local, remote uint16) error { } // AddExternalInterfaceBFD adds the inter AS connection BFD session. -func (d *DataPlane) addExternalInterfaceBFD(ifID uint16, conn BatchConn, +func (d *DataPlane) addExternalInterfaceBFD(ifID uint16, src, dst control.LinkEnd, cfg control.BFD) error { if *cfg.Disable { @@ -459,7 +459,7 @@ func (d *DataPlane) addExternalInterfaceBFD(ifID uint16, conn BatchConn, PacketsReceived: d.Metrics.BFDPacketsReceived.With(labels), } } - s, err := newBFDSend(conn, src.IA, dst.IA, src.Addr, dst.Addr, ifID, d.macFactory()) + s, err := newBFDSend(d, src.IA, dst.IA, src.Addr, dst.Addr, ifID, d.macFactory()) if err != nil { return err } @@ -477,7 +477,7 @@ func (d *DataPlane) getInterfaceState(ifID uint16) control.InterfaceState { return control.InterfaceUp } -func (d *DataPlane) addBFDController(ifID uint16, s *bfdSend, cfg control.BFD, +func (d *DataPlane) addBFDController(ifID uint16, s bfd.Sender, cfg control.BFD, metrics bfd.Metrics) error { if d.bfdSessions == nil { @@ -599,7 +599,7 @@ func (d *DataPlane) addNextHopBFD(ifID uint16, src, dst netip.AddrPort, cfg cont } } - s, err := newBFDSend(d.internal, d.localIA, d.localIA, src, dst, 0, d.macFactory()) + s, err := newBFDSend(d, d.localIA, d.localIA, src, dst, 0, d.macFactory()) if err != nil { return err } @@ -621,7 +621,6 @@ type RunConfig struct { func (d *DataPlane) Run(ctx context.Context, cfg *RunConfig) error { d.mtx.Lock() - d.setRunning() d.initMetrics() processorQueueSize := max( @@ -630,7 +629,9 @@ func (d *DataPlane) Run(ctx context.Context, cfg *RunConfig) error { d.initPacketPool(cfg, processorQueueSize) procQs, fwQs, slowQs := initQueues(cfg, d.interfaces, processorQueueSize) + d.fwQs = fwQs // Shared with BFD senders + d.setRunning() for ifID, conn := range d.interfaces { go func(ifID uint16, conn BatchConn) { defer log.HandlePanic() @@ -759,7 +760,7 @@ func (d *DataPlane) runReceiver(ifID uint16, conn BatchConn, cfg *RunConfig, // Give a new buffer to the msgs elements that have been used in the previous loop. for i := 0; i < cfg.BatchSize-numReusable; i++ { - p := <-d.packetPool + p := d.getPacketFromPool() p.reset() packets[i] = p msgs[i].Buffers[0] = p.rawPacket @@ -805,6 +806,10 @@ func computeProcID(data []byte, numProcRoutines int, hashSeed uint32) (uint32, e return s % uint32(numProcRoutines), nil } +func (d *DataPlane) getPacketFromPool() *packet { + return <-d.packetPool +} + func (d *DataPlane) returnPacketToPool(pkt *packet) { d.packetPool <- pkt } @@ -1821,6 +1826,7 @@ func (p *scionPacketProcessor) handleEgressRouterAlert() disposition { return pForward } if _, ok := p.d.external[p.pkt.egress]; !ok { + // the egress router is not this one. return pForward } *alert = false @@ -1990,7 +1996,6 @@ func (p *scionPacketProcessor) process() disposition { if disp := p.validateEgressUp(); disp != pForward { return disp } - if _, ok := p.d.external[egressID]; ok { // Not ASTransit in if disp := p.processEgress(); disp != pForward { @@ -2316,7 +2321,8 @@ func updateSCIONLayer(rawPkt []byte, s slayers.SCION, buffer gopacket.SerializeB } type bfdSend struct { - conn BatchConn + dataPlane *DataPlane + ifID uint16 srcAddr, dstAddr netip.AddrPort scn *slayers.SCION ohp *onehop.Path @@ -2326,7 +2332,7 @@ type bfdSend struct { } // newBFDSend creates and initializes a BFD Sender -func newBFDSend(conn BatchConn, srcIA, dstIA addr.IA, srcAddr, dstAddr netip.AddrPort, +func newBFDSend(d *DataPlane, srcIA, dstIA addr.IA, srcAddr, dstAddr netip.AddrPort, ifID uint16, mac hash.Hash) (*bfdSend, error) { scn := &slayers.SCION{ @@ -2373,8 +2379,13 @@ func newBFDSend(conn BatchConn, srcIA, dstIA addr.IA, srcAddr, dstAddr netip.Add scn.Path = ohp } + // bfdSend includes a reference to the dataplane. In general this must not be used until the + // dataplane is running. This is ensured by the fact that bfdSend objects are owned by bfd + // sessions, which are started by dataplane.Run() itself. + return &bfdSend{ - conn: conn, + dataPlane: d, + ifID: ifID, srcAddr: srcAddr, dstAddr: dstAddr, scn: scn, @@ -2405,7 +2416,31 @@ func (b *bfdSend) Send(bfd *layers.BFD) error { if err != nil { return err } - _, err = b.conn.WriteTo(b.buffer.Bytes(), b.dstAddr) + + // BfdControllers and fwQs are initialized from the same set of ifIDs. So not finding + // the forwarding queue is an serious internal error. Let that panic. + fwChan := b.dataPlane.fwQs[b.ifID] + + p := b.dataPlane.getPacketFromPool() + p.reset() + + // TODO: it would be best to serialize directly into the packet buffer. This would require + // a custom SerializeBuffer implementation and some changes to the packet structure. To be + // considered in a future refactoring. + sz := copy(p.rawPacket, b.buffer.Bytes()) + p.rawPacket = p.rawPacket[:sz] + if b.ifID == 0 { + // Using the internal interface: must specify the destination address + updateNetAddrFromAddrPort(p.dstAddr, b.dstAddr) + } + // No need to specify pkt.egress. It isn't used downstream from here. + select { + case fwChan <- p: + default: + // We do not care if some BFD packets get bounced under high load. If it becomes a problem, + // the solution is do use BFD's demand-mode. To be considered in a future refactoring. + b.dataPlane.returnPacketToPool(p) + } return err } diff --git a/router/dataplane_internal_test.go b/router/dataplane_internal_test.go index 86b91f4088..f6274f5f3c 100644 --- a/router/dataplane_internal_test.go +++ b/router/dataplane_internal_test.go @@ -89,7 +89,7 @@ func TestReceiver(t *testing.T) { dp.setRunning() dp.initMetrics() go func() { - dp.runReceiver(0, dp.internal, runConfig, procCh) + dp.runReceiver(0, dp.interfaces[0], runConfig, procCh) }() ptrMap := make(map[uintptr]struct{}) for i := 0; i < 21; i++ { @@ -182,7 +182,7 @@ func TestForwarder(t *testing.T) { initialPoolSize := len(dp.packetPool) dp.setRunning() dp.initMetrics() - go dp.runForwarder(0, dp.internal, runConfig, fwCh[0]) + go dp.runForwarder(0, dp.interfaces[0], runConfig, fwCh[0]) dstAddr := &net.UDPAddr{IP: net.IP{10, 0, 200, 200}} for i := 0; i < 255; i++ { diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 87f2e9f1b4..1c812f9816 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -306,7 +306,7 @@ func TestDataPlaneRun(t *testing.T) { }, ).Times(1) mExternal.EXPECT().ReadBatch(gomock.Any()).Return(0, nil).AnyTimes() - mExternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() + mExternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() l := control.LinkEnd{ IA: addr.MustParseIA("1-ff00:0:1"), Addr: netip.MustParseAddrPort("10.0.0.100:0"), @@ -373,10 +373,9 @@ func TestDataPlaneRun(t *testing.T) { }, ).Times(1) mInternal.EXPECT().ReadBatch(gomock.Any()).Return(0, nil).AnyTimes() - - mInternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).DoAndReturn( - func(data []byte, _ netip.AddrPort) (int, error) { - pkt := gopacket.NewPacket(data, + mInternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).DoAndReturn( + func(msgs underlayconn.Messages, _ int) (int, error) { + pkt := gopacket.NewPacket(msgs[0].Buffers[0], slayers.LayerTypeSCION, gopacket.Default) if b := pkt.Layer(layers.LayerTypeBFD); b != nil { v := b.(*layers.BFD).YourDiscriminator @@ -391,7 +390,7 @@ func TestDataPlaneRun(t *testing.T) { return 0, fmt.Errorf("no valid BFD message") }).MinTimes(1) - mInternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() + mInternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() local := netip.MustParseAddrPort("10.0.200.100:0") _ = ret.SetKey([]byte("randomkeyformacs")) @@ -410,9 +409,9 @@ func TestDataPlaneRun(t *testing.T) { localAddr := netip.MustParseAddrPort("10.0.200.100:0") remoteAddr := netip.MustParseAddrPort("10.0.200.200:0") mInternal := mock_router.NewMockBatchConn(ctrl) - mInternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).DoAndReturn( - func(data []byte, _ netip.AddrPort) (int, error) { - pkt := gopacket.NewPacket(data, + mInternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).DoAndReturn( + func(msgs underlayconn.Messages, _ int) (int, error) { + pkt := gopacket.NewPacket(msgs[0].Buffers[0], slayers.LayerTypeSCION, gopacket.Default) if b := pkt.Layer(layers.LayerTypeBFD); b == nil { @@ -464,9 +463,9 @@ func TestDataPlaneRun(t *testing.T) { mExternal := mock_router.NewMockBatchConn(ctrl) mExternal.EXPECT().ReadBatch(gomock.Any()).Return(0, nil).AnyTimes() - mExternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).DoAndReturn( - func(data []byte, _ netip.AddrPort) (int, error) { - pkt := gopacket.NewPacket(data, + mExternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).DoAndReturn( + func(msgs underlayconn.Messages, _ int) (int, error) { + pkt := gopacket.NewPacket(msgs[0].Buffers[0], slayers.LayerTypeSCION, gopacket.Default) if b := pkt.Layer(layers.LayerTypeBFD); b == nil { @@ -491,7 +490,7 @@ func TestDataPlaneRun(t *testing.T) { done <- struct{}{} return 1, nil }).MinTimes(1) - mExternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() + mExternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() local := control.LinkEnd{ IA: addr.MustParseIA("1-ff00:0:1"), @@ -552,9 +551,9 @@ func TestDataPlaneRun(t *testing.T) { ).Times(1) mExternal.EXPECT().ReadBatch(gomock.Any()).Return(0, nil).AnyTimes() - mExternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).DoAndReturn( - func(data []byte, _ netip.AddrPort) (int, error) { - pkt := gopacket.NewPacket(data, + mExternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).DoAndReturn( + func(msgs underlayconn.Messages, _ int) (int, error) { + pkt := gopacket.NewPacket(msgs[0].Buffers[0], slayers.LayerTypeSCION, gopacket.Default) if b := pkt.Layer(layers.LayerTypeBFD); b != nil { @@ -569,7 +568,7 @@ func TestDataPlaneRun(t *testing.T) { } return 0, fmt.Errorf("no valid BFD message") }).MinTimes(1) - mExternal.EXPECT().WriteTo(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() + mExternal.EXPECT().WriteBatch(gomock.Any(), gomock.Any()).Return(0, nil).AnyTimes() local := control.LinkEnd{ IA: addr.MustParseIA("1-ff00:0:1"), diff --git a/router/export_test.go b/router/export_test.go index e5f4cc0d18..1c87bcce8b 100644 --- a/router/export_test.go +++ b/router/export_test.go @@ -76,6 +76,7 @@ func NewDP( key []byte) *DataPlane { dp := &DataPlane{ + interfaces: map[uint16]BatchConn{0: internal}, localIA: local, external: external, linkTypes: linkTypes, @@ -84,10 +85,10 @@ func NewDP( dispatchedPortStart: uint16(dispatchedPortStart), dispatchedPortEnd: uint16(dispatchedPortEnd), svc: &services{m: svc}, - internal: internal, internalIP: netip.MustParseAddr("198.51.100.1"), Metrics: metrics, } + if err := dp.SetKey(key); err != nil { panic(err) } diff --git a/router/mock_router/mock.go b/router/mock_router/mock.go index d2c5a6fb87..b5238fb4fa 100644 --- a/router/mock_router/mock.go +++ b/router/mock_router/mock.go @@ -5,7 +5,6 @@ package mock_router import ( - netip "net/netip" reflect "reflect" gomock "github.com/golang/mock/gomock" @@ -78,18 +77,3 @@ func (mr *MockBatchConnMockRecorder) WriteBatch(arg0, arg1 interface{}) *gomock. mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WriteBatch", reflect.TypeOf((*MockBatchConn)(nil).WriteBatch), arg0, arg1) } - -// WriteTo mocks base method. -func (m *MockBatchConn) WriteTo(arg0 []byte, arg1 netip.AddrPort) (int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "WriteTo", arg0, arg1) - ret0, _ := ret[0].(int) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// WriteTo indicates an expected call of WriteTo. -func (mr *MockBatchConnMockRecorder) WriteTo(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WriteTo", reflect.TypeOf((*MockBatchConn)(nil).WriteTo), arg0, arg1) -} From 8109a33f2c8cb01d80fea6529cd7ac3c23f52145 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Wed, 20 Nov 2024 14:33:56 +0100 Subject: [PATCH 3/5] build: work around issues between pkg_rpm and rpmbuild after 4.20 (#4652) Since rpmbuild v4.20, the _builddir variable is forcefully set to / (which is created if it doesn't exist). Any files that are not built/copied by the %prep% phase must be already present in //[%buildsubdir]. pkg_rpm doesn't expect that and places the files to be packaged in . Since %buildsubdir is not used (=="."), we compensate by setting it to "..". As a result //[%buildsubdir] evaluates to thereby restoring pre-4.20 behavior. Since 4.20, debug-info rpms are built by default. We don't support or want that, so we now set "don't-do-it" explicitly. Fixes #4653 --- dist/package.bzl | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/dist/package.bzl b/dist/package.bzl index cd4c63af54..f1025af77a 100644 --- a/dist/package.bzl +++ b/dist/package.bzl @@ -121,6 +121,17 @@ def scion_pkg_rpm(name, package, executables = {}, systemds = [], configs = [], if post: kwargs.pop("postinst") + # defines are work-arounds for pkg_rpm issues: + # _smp_build_ncpus: used to be default. + # buildsubdir: starting with rpmbuild 4.20, builddir is forcefully replaced with: + # / (which is created if it doesn't exist). + # pkg_rpm assumes that one can place ready-to-package files directly into + # , we need / to evealuate as + # . Hence the "buildsubdir=..". It's a hack. Check if later + # versions of pkg_rpm fix that. + # debug_package: Starting with rpmbuild 4.20, debug-info packages are built by default. We + # currently don't want that and our binaries do not have the required symbols. + pkg_rpm( name = name, summary = kwargs["description"], @@ -132,9 +143,10 @@ def scion_pkg_rpm(name, package, executables = {}, systemds = [], configs = [], package_name = package, release = "%autorelease", version_file = ":%s_version" % name, - defines = {"_smp_build_ncpus": "1"}, + defines = {"_smp_build_ncpus": "1", "buildsubdir": "..", "debug_package": "%{nil}"}, requires = deps, post_scriptlet_file = post, + source_date_epoch = 0, **kwargs ) From a118737fd69e61ac1ace3331bb32ee8259b6b136 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Thu, 21 Nov 2024 06:56:01 +0100 Subject: [PATCH 4/5] build: address upstream vulnerability (#4655) Reported by dependabot in https://github.com/scionproto/scion/security/dependabot/82 --- private/mgmtapi/tools/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/mgmtapi/tools/package.json b/private/mgmtapi/tools/package.json index 9e347c6ccc..e955fcd14f 100644 --- a/private/mgmtapi/tools/package.json +++ b/private/mgmtapi/tools/package.json @@ -5,7 +5,7 @@ }, "pnpm": { "overrides": { - "@types/jsonpath-plus": ">=10.0.0" + "@types/jsonpath-plus": ">=10.0.7" }, "onlyBuiltDependencies": [], "packageExtensions": { From 3c8e30fbdb64f27f8d166a3a116c12ca353aa601 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Fri, 29 Nov 2024 15:33:32 +0100 Subject: [PATCH 5/5] router: serialize directly into packet buffers (#4654) In every case where the router modified packets it would serialize updated headers to a temporary buffer and then copy that to the packet buffer. To avoid this extra copy, replaced gopacket.serializeBuffer with a custom implementation that writes to a given buffer. In this case, the packet's raw buffer. There is one remaining copy for some SCMP messages because we have to move the existing packet to the payload. This too could be avoided but that's for another PR; it would require to leave headroom in received packets. The performance impact is very small since, on the critical path, it just avoids copying a scion hdr per packet, but it is a simplification. It also pays back the copy added by a previous simplification of the BFD code. As such... Contributes to #4593 --- router/BUILD.bazel | 1 + router/dataplane.go | 154 +++++++++++++++++--------------------- router/serialize_proxy.go | 103 +++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 86 deletions(-) create mode 100644 router/serialize_proxy.go diff --git a/router/BUILD.bazel b/router/BUILD.bazel index caf311a7f1..ef54cc4e5e 100644 --- a/router/BUILD.bazel +++ b/router/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "dataplane.go", "fnv1aCheap.go", "metrics.go", + "serialize_proxy.go", "svc.go", ], importpath = "github.com/scionproto/scion/router", diff --git a/router/dataplane.go b/router/dataplane.go index c1322a5f4a..6c407ab1c6 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -1,5 +1,6 @@ // Copyright 2020 Anapaya Systems // Copyright 2023 ETH Zurich +// Copyright 2024 SCION Association // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -111,8 +112,10 @@ const ( // required to occupy exactly 64 bytes depends on the architecture. type packet struct { // The useful part of the raw packet at a point in time (i.e. a slice of the full buffer). - // TODO(jiceatscion): would it be beneficial to store the length instead, like readBatch does? + // It can be any portion of the full buffer; not necessarily the start. rawPacket []byte + // The entire packet buffer. We don't need it as a slice; we know its size. + buffer *[bufSize]byte // The source address. Will be set by the receiver from smsg.Addr. We could update it in-place, // but the IP address bytes in it are allocated by readbatch, so if we copy into a recyclable // location, it's the original we throw away. No gain (may be a tiny bit?). @@ -130,9 +133,7 @@ type packet struct { // economically determined at the processing stage. So store it here. It's 2 bytes long. trafficType trafficType // Pad to 64 bytes. For 64bit arch, add 12 bytes. For 32bit arch, add 32 bytes. - // TODO(jiceatscion): see if packing two packets per cache line instead is good or bad for 32bit - // machines. - _ [12 + is32bit*20]byte + _ [4 + is32bit*24]byte } // Keep this 6 bytes long. See comment for packet. @@ -150,7 +151,8 @@ const _ uintptr = unsafe.Sizeof(packet{}) - 64 // assert sizeof(packet) >= 64 // initPacket configures the given blank packet (and returns it, for convenience). func (p *packet) init(buffer *[bufSize]byte) *packet { - p.rawPacket = buffer[:] + p.buffer = buffer + p.rawPacket = p.buffer[:] p.dstAddr = &net.UDPAddr{IP: make(net.IP, net.IPv6len)} return p } @@ -160,11 +162,11 @@ func (p *packet) init(buffer *[bufSize]byte) *packet { func (p *packet) reset() { p.dstAddr.IP = p.dstAddr.IP[0:0] // We're keeping the object, just blank it. *p = packet{ - rawPacket: p.rawPacket[:cap(p.rawPacket)], // keep the slice and so the backing array. - dstAddr: p.dstAddr, // keep the dstAddr and so the IP slice and bytes + buffer: p.buffer, // keep the buffer + rawPacket: p.buffer[:], // restore the full packet capacity + dstAddr: p.dstAddr, // keep the dstAddr and so the IP slice and bytes } // Everything else is reset to zero value. - } // DataPlane contains a SCION Border Router's forwarding logic. It reads packets @@ -907,7 +909,6 @@ func (d *DataPlane) runSlowPathProcessor(id int, q <-chan *packet, func newSlowPathProcessor(d *DataPlane) *slowPathPacketProcessor { p := &slowPathPacketProcessor{ d: d, - buffer: gopacket.NewSerializeBuffer(), macInputBuffer: make([]byte, spao.MACBufferSize), drkeyProvider: &drkeyutil.FakeProvider{ EpochDuration: drkeyutil.LoadEpochDuration(), @@ -921,9 +922,8 @@ func newSlowPathProcessor(d *DataPlane) *slowPathPacketProcessor { } type slowPathPacketProcessor struct { - d *DataPlane - pkt *packet - buffer gopacket.SerializeBuffer + d *DataPlane + pkt *packet scionLayer slayers.SCION hbhLayer slayers.HopByHopExtnSkipper @@ -945,11 +945,6 @@ type slowPathPacketProcessor struct { } func (p *slowPathPacketProcessor) reset() { - if err := p.buffer.Clear(); err != nil { - // The serializeBuffer returned by NewSerializeBuffer isn't actually capable of failing to - // clear, so planning on doing something about it is pointless (and what might that be?). - panic(fmt.Sprintf("Error while clearing buffer: %v", err)) - } p.path = nil p.hbhLayer = slayers.HopByHopExtnSkipper{} p.e2eLayer = slayers.EndToEndExtnSkipper{} @@ -1123,7 +1118,6 @@ func readUpTo(c <-chan *packet, n int, needsBlocking bool, pkts []*packet) int { func newPacketProcessor(d *DataPlane) *scionPacketProcessor { p := &scionPacketProcessor{ d: d, - buffer: gopacket.NewSerializeBuffer(), mac: d.macFactory(), macInputBuffer: make([]byte, max(path.MACBufferSize, libepic.MACBufferSize)), } @@ -1139,11 +1133,6 @@ func (p *scionPacketProcessor) reset() error { p.infoField = path.InfoField{} p.effectiveXover = false p.peering = false - if err := p.buffer.Clear(); err != nil { - // The serializeBuffer returned by NewSerializeBuffer isn't actually capable of failing to - // clear, so planning on doing something about it is pointless (and what might that be?). - panic(fmt.Sprintf("Error while clearing buffer: %v", err)) - } p.mac.Reset() p.cachedMac = nil // Reset hbh layer @@ -1313,8 +1302,6 @@ type scionPacketProcessor struct { d *DataPlane // pkt is the packet currently being processed by this processor. pkt *packet - // buffer is the buffer that can be used to serialize gopacket layers. - buffer gopacket.SerializeBuffer // mac is the hasher for the MAC computation. mac hash.Hash @@ -1374,10 +1361,8 @@ func (p *slowPathPacketProcessor) packSCMP( } } - rawSCMP, err := p.prepareSCMP(typ, code, scmpP, isError) - if rawSCMP != nil { - p.pkt.rawPacket = p.pkt.rawPacket[:len(rawSCMP)] - copy(p.pkt.rawPacket, rawSCMP) + if err := p.prepareSCMP(typ, code, scmpP, isError); err != nil { + return err } // We're about to send a packet that has little to do with the one we received. @@ -1385,7 +1370,7 @@ func (p *slowPathPacketProcessor) packSCMP( p.pkt.trafficType = ttOther p.pkt.egress = p.pkt.ingress updateNetAddrFromNetAddr(p.pkt.dstAddr, p.pkt.srcAddr) - return err + return nil } func (p *scionPacketProcessor) parsePath() disposition { @@ -2072,7 +2057,7 @@ func (p *scionPacketProcessor) processOHP() disposition { } ohp.Info.UpdateSegID(ohp.FirstHop.Mac) - if err := updateSCIONLayer(p.pkt.rawPacket, s, p.buffer); err != nil { + if err := updateSCIONLayer(p.pkt.rawPacket, s); err != nil { return errorDiscard("error", err) } p.pkt.egress = ohp.FirstHop.ConsEgress @@ -2098,7 +2083,7 @@ func (p *scionPacketProcessor) processOHP() disposition { ohp.SecondHop.Mac = path.MAC(p.mac, ohp.Info, ohp.SecondHop, p.macInputBuffer[:path.MACBufferSize]) - if err := updateSCIONLayer(p.pkt.rawPacket, s, p.buffer); err != nil { + if err := updateSCIONLayer(p.pkt.rawPacket, s); err != nil { return errorDiscard("error", err) } err := p.d.resolveLocalDst(p.pkt.dstAddr, s, p.lastLayer) @@ -2302,22 +2287,18 @@ func decodeSCMP(scmp *slayers.SCMP) ([]gopacket.SerializableLayer, error) { return ret, nil } -// TODO(matzf) this function is now only used to update the OneHop-path. -// This should be changed so that the OneHop-path can be updated in-place, like -// the scion.Raw path. -func updateSCIONLayer(rawPkt []byte, s slayers.SCION, buffer gopacket.SerializeBuffer) error { - if err := buffer.Clear(); err != nil { - return err - } - if err := s.SerializeTo(buffer, gopacket.SerializeOptions{}); err != nil { - return err - } - // TODO(lukedirtwalker): We should add a method to the scion layers - // which can write into the existing buffer, see also the discussion in - // https://fsnets.slack.com/archives/C8ADBBG0J/p1592805884250700 - rawContents := buffer.Bytes() - copy(rawPkt[:len(rawContents)], rawContents) - return nil +// updateSCIONLayer rewrites the SCION header at the start of the given raw packet buffer; replacing +// it with the serialization of the given new SCION header. This works only if the new header is of +// the same size as the old one. This function has no knowledge of the actual size of the headers; +// it only ensures that the new one ends exactly where the old one did. It is possible to use this +// function to replace a header with a smaller one; but the rawPacket's slice must be fixed +// afterwards (and the preceding headers, if any). +func updateSCIONLayer(rawPkt []byte, s slayers.SCION) error { + payloadOffset := len(rawPkt) - len(s.LayerPayload()) + + // Prepends must go just before payload. (and any Append will wreck it) + serBuf := newSerializeProxyStart(rawPkt, payloadOffset) + return s.SerializeTo(&serBuf, gopacket.SerializeOptions{}) } type bfdSend struct { @@ -2328,7 +2309,6 @@ type bfdSend struct { ohp *onehop.Path mac hash.Hash macBuffer []byte - buffer gopacket.SerializeBuffer } // newBFDSend creates and initializes a BFD Sender @@ -2392,7 +2372,6 @@ func newBFDSend(d *DataPlane, srcIA, dstIA addr.IA, srcAddr, dstAddr netip.AddrP ohp: ohp, mac: mac, macBuffer: make([]byte, path.MACBufferSize), - buffer: gopacket.NewSerializeBuffer(), }, nil } @@ -2411,24 +2390,26 @@ func (b *bfdSend) Send(bfd *layers.BFD) error { ohp.FirstHop.Mac = path.MAC(b.mac, ohp.Info, ohp.FirstHop, b.macBuffer) } - err := gopacket.SerializeLayers(b.buffer, gopacket.SerializeOptions{FixLengths: true}, + p := b.dataPlane.getPacketFromPool() + p.reset() + + serBuf := newSerializeProxy(p.rawPacket) // set for prepend-only by default. Perfect here. + + // serialized bytes lend directly into p.rawPacket (alignedd at the end). + err := gopacket.SerializeLayers(&serBuf, gopacket.SerializeOptions{FixLengths: true}, b.scn, bfd) if err != nil { return err } + // The useful part of the buffer is given by Bytes. We don't copy the bytes; just the slice's + // metadata. + p.rawPacket = serBuf.Bytes() + // BfdControllers and fwQs are initialized from the same set of ifIDs. So not finding // the forwarding queue is an serious internal error. Let that panic. fwChan := b.dataPlane.fwQs[b.ifID] - p := b.dataPlane.getPacketFromPool() - p.reset() - - // TODO: it would be best to serialize directly into the packet buffer. This would require - // a custom SerializeBuffer implementation and some changes to the packet structure. To be - // considered in a future refactoring. - sz := copy(p.rawPacket, b.buffer.Bytes()) - p.rawPacket = p.rawPacket[:sz] if b.ifID == 0 { // Using the internal interface: must specify the destination address updateNetAddrFromAddrPort(p.dstAddr, b.dstAddr) @@ -2449,7 +2430,7 @@ func (p *slowPathPacketProcessor) prepareSCMP( code slayers.SCMPCode, scmpP gopacket.SerializableLayer, isError bool, -) ([]byte, error) { +) error { // *copy* and reverse path -- the original path should not be modified as this writes directly // back to rawPkt (quote). @@ -2460,36 +2441,36 @@ func (p *slowPathPacketProcessor) prepareSCMP( var ok bool path, ok = p.scionLayer.Path.(*scion.Raw) if !ok { - return nil, serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", + return serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", "path type", pathType) } case epic.PathType: epicPath, ok := p.scionLayer.Path.(*epic.Path) if !ok { - return nil, serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", + return serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", "path type", pathType) } path = epicPath.ScionPath default: - return nil, serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", + return serrors.JoinNoStack(cannotRoute, nil, "details", "unsupported path type", "path type", pathType) } decPath, err := path.ToDecoded() if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "decoding raw path") + return serrors.JoinNoStack(cannotRoute, err, "details", "decoding raw path") } revPathTmp, err := decPath.Reverse() if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "reversing path for SCMP") + return serrors.JoinNoStack(cannotRoute, err, "details", "reversing path for SCMP") } revPath := revPathTmp.(*scion.Decoded) peering, err := determinePeer(revPath.PathMeta, revPath.InfoFields[revPath.PathMeta.CurrINF]) if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "peering cannot be determined") + return serrors.JoinNoStack(cannotRoute, err, "details", "peering cannot be determined") } // Revert potential path segment switches that were done during processing. @@ -2497,7 +2478,7 @@ func (p *slowPathPacketProcessor) prepareSCMP( // An effective cross-over is a change of segment other than at // a peering hop. if err := revPath.IncPath(); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, + return serrors.JoinNoStack(cannotRoute, err, "details", "reverting cross over for SCMP") } } @@ -2511,7 +2492,7 @@ func (p *slowPathPacketProcessor) prepareSCMP( infoField.UpdateSegID(hopField.Mac) } if err := revPath.IncPath(); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, + return serrors.JoinNoStack(cannotRoute, err, "details", "incrementing path for SCMP") } } @@ -2529,7 +2510,7 @@ func (p *slowPathPacketProcessor) prepareSCMP( scionL.NextHdr = slayers.L4SCMP if err := scionL.SetSrcAddr(addr.HostIP(p.d.internalIP)); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "setting src addr") + return serrors.JoinNoStack(cannotRoute, err, "details", "setting src addr") } typeCode := slayers.CreateSCMPTypeCode(typ, code) scmpH := slayers.SCMP{TypeCode: typeCode} @@ -2569,19 +2550,19 @@ func (p *slowPathPacketProcessor) prepareSCMP( } } - if err := p.buffer.Clear(); err != nil { - return nil, err - } + serBuf := newSerializeProxy(p.pkt.rawPacket) // Prepend-only by default. It's all we need. sopts := gopacket.SerializeOptions{ ComputeChecksums: true, FixLengths: true, } // First write the SCMP message only without the SCION header(s) to get a buffer that we - // can (re-)use as input in the MAC computation. - // XXX(matzf) could we use iovec gather to avoid copying quote? - err = gopacket.SerializeLayers(p.buffer, sopts, &scmpH, scmpP, gopacket.Payload(quote)) + // can (re-)use as input in the MAC computation. Note that we move the quoted part of the packet + // to the end of the buffer (go supports overlaps properly). + // TODO(jiceatscion): in the future we may be able to leave room at the head of the + // buffer on ingest, so we won't need to move the quote at all. + err = gopacket.SerializeLayers(&serBuf, sopts, &scmpH, scmpP, gopacket.Payload(quote)) if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "serializing SCMP message") + return serrors.JoinNoStack(cannotRoute, err, "details", "serializing SCMP message") } if needsAuth { @@ -2591,15 +2572,15 @@ func (p *slowPathPacketProcessor) prepareSCMP( now := time.Now() dstA, err := scionL.DstAddr() if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, + return serrors.JoinNoStack(cannotRoute, err, "details", "parsing destination address") } key, err := p.drkeyProvider.GetASHostKey(now, scionL.DstIA, dstA) if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "retrieving DRKey") + return serrors.JoinNoStack(cannotRoute, err, "details", "retrieving DRKey") } if err := p.resetSPAOMetadata(key, now); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "resetting SPAO header") + return serrors.JoinNoStack(cannotRoute, err, "details", "resetting SPAO header") } e2e.Options = []*slayers.EndToEndOption{p.optAuth.EndToEndOption} @@ -2610,27 +2591,28 @@ func (p *slowPathPacketProcessor) prepareSCMP( Header: p.optAuth, ScionLayer: &scionL, PldType: slayers.L4SCMP, - Pld: p.buffer.Bytes(), + Pld: serBuf.Bytes(), }, p.macInputBuffer, p.optAuth.Authenticator(), ) if err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "computing CMAC") + return serrors.JoinNoStack(cannotRoute, err, "details", "computing CMAC") } - if err := e2e.SerializeTo(p.buffer, sopts); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, + if err := e2e.SerializeTo(&serBuf, sopts); err != nil { + return serrors.JoinNoStack(cannotRoute, err, "details", "serializing SCION E2E headers") } } else { scionL.NextHdr = slayers.L4SCMP } - if err := scionL.SerializeTo(p.buffer, sopts); err != nil { - return nil, serrors.JoinNoStack(cannotRoute, err, "details", "serializing SCION header") + if err := scionL.SerializeTo(&serBuf, sopts); err != nil { + return serrors.JoinNoStack(cannotRoute, err, "details", "serializing SCION header") } + p.pkt.rawPacket = serBuf.Bytes() log.Debug("scmp", "typecode", scmpH.TypeCode) - return p.buffer.Bytes(), nil + return nil } func (p *slowPathPacketProcessor) resetSPAOMetadata(key drkey.ASHostKey, now time.Time) error { diff --git a/router/serialize_proxy.go b/router/serialize_proxy.go new file mode 100644 index 0000000000..e3b6b1cfbc --- /dev/null +++ b/router/serialize_proxy.go @@ -0,0 +1,103 @@ +// Copyright 2024 SCION Association +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package router + +import ( + "github.com/google/gopacket" +) + +// serializeProxy implements gopacket.SerializeBuffer. It is a very simple implementation that +// writes to a separately allocated buffer (such as a packet's raw buffer). Space is added to the +// buffer via PrependBytes and AppendBytes simply by changing the starting point and length of the +// data slice. No reallocation is ever performed. Running out of append or prepend space triggers a +// panic. It is designed to be a local variable, so New() returns a value. The entire buffer +// underpinning the given slice may be used; that is, from the start up to the remaining capacity. +type serializeProxy struct { + + // The slice's offset can't be changed as that is irreversible. + // So we keep track of the prepend point separately from the slice. + + restart int // the value to reset start to during Clear(). + start int // current start of the useful data in the buffer. + data []byte + layers []gopacket.LayerType +} + +// newSerializeProxy returns a new serializeProxy. The initial prepend/append point is set to the +// end of the buffer in anticipation of AppendBytes never being used. The prepend/append point can +// be changed when calling clear(). +func newSerializeProxy(buf []byte) serializeProxy { + return newSerializeProxyStart(buf, cap(buf)) +} + +// newSerializeProxyStart returns a new serializeProxy. The initial prepend/append point is set to +// the given start value. This has the same effect as calling clear(statr). +func newSerializeProxyStart(buf []byte, start int) serializeProxy { + serBuf := serializeProxy{ + data: buf, + } + serBuf.clear(start) + return serBuf +} + +// Resets the buffer to empty and sets the initial prepend/append point to the given position. +// The next prepend will claim an area ending with index newStart - 1. The next append will claim an +// area starting with index newStart. +func (s *serializeProxy) clear(newStart int) { + s.restart = newStart + s.start = newStart + s.data = s.data[:newStart] + s.layers = s.layers[:0] +} + +// Implements serializeBuffer.Clear(). This implementation never returns an error. +// The initial prepend/append point is reset to that which was set by the last call to clear(). +func (s *serializeProxy) Clear() error { + s.clear(s.restart) + return nil +} + +// PrependBytes implements serializeBuffer.PrependBytes(). It never returns an error. +// It can panic if attenpting to prepend before the start of the buffer. +func (s *serializeProxy) PrependBytes(num int) ([]byte, error) { + s.start -= num + return s.data[s.start : s.start+num], nil +} + +// AppendBytes implements serializeBuffer.AppendBytes(). It never returns an error. +// It can panic if attempting to append past the end of the buffer. +func (s *serializeProxy) AppendBytes(num int) ([]byte, error) { + ol := len(s.data) + nl := ol + num + s.data = s.data[:nl] + return s.data[ol:nl], nil +} + +// Bytes implements serializeBuffer.Bytes(). It returns a slice that represents the useful portion +// of the buffer. That is the portion that contains all the prepended and appended bytes since the +// last call to Clear(). +func (s *serializeProxy) Bytes() []byte { + return s.data[s.start:] +} + +// Bytes implements serializeBuffer.Layers. +func (s *serializeProxy) Layers() []gopacket.LayerType { + return s.layers +} + +// Bytes implements serializeBuffer.PushLayer. +func (s *serializeProxy) PushLayer(l gopacket.LayerType) { + s.layers = append(s.layers, l) +}