Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with nd_light #273

Merged
merged 4 commits into from
Oct 6, 2014
Merged

Conversation

alexandergall
Copy link
Contributor

The nd_light app did not interoperate with itself due to the way the link-layer address options of ND messages were handled. Also, a bug in the initialization caused next-hop resolution to be started only for one instance of nd_light in an app that uses multiple instances of it. Thanks to Max Rottenkolber for finding these issues.

The semantics of nd_light with respect to packets received from the north port has also been changed. The app now expects complete Ethernet frames and overwrites the Ethernet header with the header constructed from the address resolution process (see commit message for the rationale for this)

The parse iovec index was not properly updated when a new buffer is
prepended to the packet.
Send source link-layer address option with neighbor solicitations.

Ignore options in received neighbor advertisements. Self-originated
solicited neighbor advertisement are created from a pre-fabricated
packet that contains the target link-layer address option instead of
turning around the incoming neighbor solicitation.

Bug fix: The nh table as well as the pre-allocated datagram object
were erroneously added to the class instead of the instance.  This
caused next-hop resolution to be executed only for one instance of
nd_light running in the same app.
In order to maintain symmetry, the semantics of nd_light are changed
such that it expects full Ethernet frames on its north port.  Instead
of adding the header, it merely overwrites it.

The rationale for this change is as follows.  It is desrieable to keep
the Ethernet header in the packets received from the south port when
transmitting them to the north port, which will allow the receiving
app to inspect the entire packet.  In particular, this is necessary if
BPF filters need to be applied to the packet higher up in the app
chain.  To obey the principle of least astonishment and to keep the
interface symmetric, the northbound apps must make sure that the
packets passed to nd_light contain an Ethernet header as well, but the
contents is irrelevant.
@lukego
Copy link
Member

lukego commented Sep 27, 2014

SnabbBot CI says the lib.nfv.config module selftest fails. Is there a bug there?

@alexandergall
Copy link
Contributor Author

The nfv selftest is only enabled on davos. I can't run it there myself because I don't have root credentials. Can you please run it and check the output of the failed test?

@lukego
Copy link
Member

lukego commented Sep 29, 2014

That test should also run on other machines (e.g. chur) if you have define SNABB_TEST_INTEL10G_PCIDEVA to be the PCI-ID of a 10G port.

But @eugeneia can you tell us if the new nd_light works for your NFV test cases now?

@alexandergall
Copy link
Contributor Author

Got it. When the test creates the nd_light app, it passes it the MAC address as string instead of the on-the-wire format. One way to get this right is to call lib.protocol.ethernet:pton(). I guess the API needs to be documented better.

The bug gets exposed now because the new version of nd_light passes this value on to some other code that expects it to be in a particular format.

Accept MAC and IPv6 addresses as strings and perform conversion.
@eugeneia
Copy link
Member

nd_light still succeeds to perfom ND. But it doesn't play well with keyed_ipv6_tunnel yet. Lets re-iterate:

  • nd_light now overwrites etherner src and dst.
  • keyed_ipv6_tunnel is unchanged (prepends L2PTv3 header)

So I'd expect nd_light to smash the L2PTv3 header and tunnel to cast the wrong section of the packet to a header struct. From the tests output it looks as if the ping packet is delivered though, but VM_B doesn't answer. No packets are dropped by the tunnel. Am I missing something?

@alexandergall
Copy link
Contributor Author

Well, of course it breaks keyed_ipv6_tunnel. It's an incompatible change. My understanding was that someone will adapt the tunnel app. Maybe I misunderstood and Max and Luke expected me to do this? Anyway, the selftest should probably catch this.

@lukego
Copy link
Member

lukego commented Sep 30, 2014

This could be an opportunity to experiment with some advanced git-fu :-)

Max, how about if you pull Alex's branch (from this pull request), add a commit that makes L2TPv3 app add the ethernet header (for overwrite), then send me a pull request from that branch? Then I think I can press Merge and both this pull request and that one will be closed (because all the commits they contain will be merged into master).

@eugeneia
Copy link
Member

I wasn't sure who should do it thats why I brought it up again. I was hesistant to do it myself because I am a little bit intimidated by the code. I am on it now.

@eugeneia
Copy link
Member

Now I know what confused me yesterday: Turns out that keyed_ipv6_tunnel already does the right thing. It adds a dst:<default_gateway> src:0 Ethernet header on the encapsulation path (part of header_template) and it failed before because nd_light added a second Ethernet header.. This also explains my test results (packets passing the tunnel).

lukego added a commit that referenced this pull request Oct 6, 2014
@lukego lukego merged commit 73b4cae into snabbco:master Oct 6, 2014
@alexandergall alexandergall deleted the nd_light-fix branch October 2, 2015 11:14
mwiget pushed a commit to mwiget/snabb that referenced this pull request Feb 18, 2016
Several improvements to generator interface
eugeneia added a commit to eugeneia/snabb that referenced this pull request Aug 22, 2024
1d83bb4ec Merge branch PR snabbco#273 (Apply commits up to 04dca791 from luajit/v2.1)
1833ed219 FFI: Add missing coercion when recording 64-bit bit.*().
44fd7ae6e Merge branch 'master' into v2.1
90ef6f69a FFI: Drop finalizer table rehash after GC cycle.
05168b7b6 Merge branch 'master' into v2.1
7ac3ae7d2 Fix another potential file descriptor leak in luaL_loadfile*().
99326d32c Merge branch 'master' into v2.1
e5933a41d Correctly close VM state after early OOM during open.
83587d06e Fix potential file descriptor leak in luaL_loadfile*().
650458c16 Add more FOLD rules for integer conversions.
7715a67c1 Merge branch 'master' into v2.1
7273a3724 Different fix for partial snapshot restore due to stack overflow.
e34177cf0 Fix IR_ABC hoisting.
9dc578a8a Limit CSE for IR_CARG to fix loop optimizations.
97b0aa3dc Merge PR snabbco#270 (Apply commits up to 04dca791 from luajit/v2.1) into master
99790e663 Merge branch 'master' into sync-04dca791-2024-07-11
007433238 Revert "LJ_FR2: Fix stack checks in vararg calls."
078553b4f Merge PR snabbco#271 (Merge Snabb-specific changes) into master
cb2589f16 Update JIT engine default parameters to what works for Snabb
4f88c2eff Don't count tail calls towards loop unroll limit.
38098e387 Revert "FFI: Unify stack setup for C calls in interpreter."
750c02ae0 Revert "FFI/ARM64/OSX: Handle non-standard OSX C calling conventions."
3382ac80f Revert "Windows/ARM64: Support Windows calling conventions."
5628e7b72 Call math.randomseed() without arguments to seed from system entropy.
67daf039f Restore state when recording __concat metamethod throws an error.
516ec81b8 Add build flag LUAJIT_DISABLE_TAILCALL to disable tailcall generation.
dccfe4b73 Clarify that lj_buf_shrink() does not keep any buffer data.
fd7893967 Merge branch 'master' into v2.1
13e71a66f FFI: Fix various issues in recff_cdata_arith.
efc90e402 Fix predict_next() in parser (for real now).
d0663fb1b FFI: Fix __tostring metamethod access to enum cdata value.
91c76f317 Fix typo.
0c09495e8 Handle partial snapshot restore due to stack overflow.
a9722be94 Prevent sanitizer warning in snap_restoredata().
53de3789d Limit number of string format elements to compile.
bf49fb11e FFI: Clarify scalar boxing behavior.
002f73e89 Fix internal link in docs.
e8d803522 Fix segment release check in internal memory allocator.
698261f44 unifdef
4a1c5a0e9 FFI: Turn FFI finalizer table into a proper GC root.
13bf7ef58 Show name of NYI bytecode in -jv and -jdump.
c4d9f914a Use generic trace error for OOM during trace stitching.
27dbce8e5 Fix serialization format docs.
93a4c0d45 Handle all types of errors during trace stitching.
ef091bdbc Fix recording of __concat metamethod.
a8322958a Prevent down-recursion for side traces.
1d8874a79 Check frame size limit before returning to a lower frame.
f5fcfcd99 FFI: Treat cdata finalizer table as a GC root.
8c1cc2af5 Handle stack reallocation in debug.setmetatable() and lua_setmetatable().
7ff5360e2 Merge branch 'master' into v2.1
bf6a57815 Rework stack overflow handling.
3bda471a1 Merge branch 'master' into v2.1
ad65b5371 Preserve keys with dynamic values in template tables when saving bytecode.
5e16b987d Fix documentation bug about '\z' string escape.
6111326b1 Fix zero stripping in %g number formatting.
ea2a2b4ab Merge remote-tracking branch 'origin/master' into sync-f2336c4-2024-01-25
d764e5ada Fix unsinking of IR_FSTORE for NULL metatable.
9516547f7 DynASM/x86: Add endbr instruction.
a0cf52bce Add cross-32/64 bit and deterministic bytecode generation.
325621e21 DynASM/x86: Allow [&expr] operand.
771693498 Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check.
0d21a797f Respect jit.off() on pending trace exit.
3feadf82e Simplify handling of instable types in TNEW/TDUP load forwarding.
5961c7a27 Only emit proper parent references in snapshot replay.
7406035ef Fix anchoring for string buffer set() method (again).
8fd377f40 Document workaround for multilib vs. cross-compiler conflict.
44f5c48cb Fix anchoring for string buffer set() method.
06176c8a3 Optimize table.new() with constant args to (sinkable) IR_TNEW.
8e76c234b Emit sunk IR_NEWREF only once per key on snapshot replay.
c9fbd875c Fix last commit.
95572d5e0 x86/x64: Don't fuse loads across IR_NEWREF.
5e74a8da1 x86/x64: Add more red zone checks to assembler backend.
7d8e32207 Invalidate SCEV entry when returning to lower frame.
3915b2d76 FFI: Fix pragma push stack limit check and throw on overflow.
c274f4026 Check for upvalue state transition in IR_UREFO.
7954eaa0f Add 'cc' file type for saving bytecode.
4bc1e3b86 FFI/Windows: Fix type declaration for int64_t and uint64_t.
8f8c4615f FFI: Fix dangling reference to CType in carith_checkarg().
61d5365eb Maintain chain invariant in DCE.
3b3b339dd LJ_FR2: Fix stack checks in vararg calls.
986bacfce Follow-up fix for stack overflow handling cleanup.
06ede71c3 Handle OOM error on stack resize in coroutine.resume and lua_checkstack.
83e762127 Restore cur_L for specific Lua/C API use case.
2da941f36 Consistently use 64 bit constants for 64 bit IR instructions.
b94d964a2 Add missing coercion when recording select(string, ...)
939d01ece Cleanup stack overflow handling.
2e6229a9a Windows/ARM64: Add MSVC cross-build support for x64 to ARM64.
39321968a IR_MIN/IR_MAX is non-commutative due to underlying FPU ops.
56420f5cb Windows/ARM64: Update install docs.
0022e4b32 Windows: Call C++ destructors without compiling with /EHa.
54e954406 FFI: Fix 64 bit shift fold rules.
c33938e34 Windows/ARM64: Support Windows calling conventions.
a7539913e Windows/ARM64: Add initial support.
4ebbfb4d5 Fix mcode limit check for non-x86 archs.
e18f8aeb0 Add NaN check to IR_NEWREF.
7570998d7 Add randomized register allocation for fuzz testing.
6603e79c8 Update external MSDN URL in code.
7bc9cd62b FFI/ARM64/OSX: Handle non-standard OSX C calling conventions.
6702b7513 FFI: Unify stack setup for C calls in interpreter.
387c1340d Handle table unsinking in the presence of IRFL_TAB_NOMM.
721560ae2 Fix external C call stack check when using LUAJIT_MODE_WRAPCFUNC.
bf8c48ed7 Fix predict_next() in parser (again).
f73a99e3b Add .gitattributes to dynamically resolve .relver.
46c439081 Switch to rolling releases: mark v2.1 as production.
caa2851c0 Switch build system to rolling releases.
a5b6cb244 Update documentation for switch to rolling releases.
ceae5e1d2 Bump copyright date.
6ce61bece Remove work-in-progress notice in string buffer docs.

git-subtree-dir: lib/luajit
git-subtree-split: 1d83bb4ec7581cc1715725af1514297ca3f3ac64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants