Skip to content
This repository has been archived by the owner on Nov 13, 2020. It is now read-only.

sysctl.d/50-default.conf: remove *, .all source route settings #11

Merged
merged 3 commits into from
Sep 10, 2020
Merged

sysctl.d/50-default.conf: remove *, .all source route settings #11

merged 3 commits into from
Sep 10, 2020

Conversation

t-lo
Copy link

@t-lo t-lo commented Sep 10, 2020 •
edited
Loading

Remove wildcard, "all" source route settings from sysctl defaults

The rules were added in systemd-245 and break cluster networking, e.g. cilium. Please see flatcar/Flatcar#181 for details.

Signed-off-by: Thilo Fromm thilo@kinvolk.io

Also cherry-pick to v246-flatcar which has the same issue.

@t-lo
sysctl.d/50-default.conf: remove *, .all source route settings
986ca69 
The rules were added in systemd-245 and break cluster
networking, e.g. cilium. Please see
flatcar/Flatcar#181
for details.

Signed-off-by: Thilo Fromm <thilo@kinvolk.io>
@t-lo t-lo requested a review from a team September 10, 2020 09:19
margamanterola
margamanterola reviewed Sep 10, 2020
View reviewed changes
Copy link

@margamanterola margamanterola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is commenting the rules really enough? Shouldn't we set them to the opposite?

sysctl.d/50-default.conf Outdated
@@ -23,18 +23,27 @@ kernel.core_uses_pid = 1

# Source route verification
net.ipv4.conf.default.rp_filter = 2
net.ipv4.conf.*.rp_filter = 2
-net.ipv4.conf.all.rp_filter
# the below deviates from upstream systemd-245 (and later) since the default
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a noun between "the" and "below"?
e.g. "settings", "rules", "configurations", etc

Same in the other two comments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... below is the noun. I get your point wrt. being more specific though, will update.

@t-lo
sysctl.d/50-default: better comments, re-activate promote_secondaries
65e0b0c 
This change updates comments as well as re-activates the
promote_secondaries wildcard since networkd's DHCP relies on this
(see systemd/systemd#7163)

Signed-off-by: Thilo Fromm <thilo@kinvolk.io>
@t-lo t-lo requested review from margamanterola and a team September 10, 2020 09:50
@margamanterola
Copy link

So, to answer my own question: commenting the rules is enough because the default is zero. Personally, I prefer explicit rather than implicit, but ok. In the comments from other issues, it was noted that Ubuntu was commenting all the lines. In this case, we're leaving the default line untouched. I think this is probably ok, but we might need to revisit if it doesn't actually solve the issues.

Now, my next question is why are we even touching accept_source_route? There's nothing in any of the issues that we saw that indicates that this setting needs to be modified. Unless we have a reason to change it, I'd leave it as is to avoid unnecessary divergence.

@t-lo
Copy link
Author

t-lo commented Sep 10, 2020

Agree on accept_source_route and will update the PR.
Also agree with revisiting this if it does not fix the cilium issue - however since adding an override rule which only sets rp_filter=0 seems to work around the cilium the issue I'm optimistic that we don't need to.

@t-lo
sysctl.d/50-default.conf: re-activate default accept_source_route
d5dc728 
Signed-off-by: Thilo Fromm <thilo@kinvolk.io>
margamanterola
margamanterola approved these changes Sep 10, 2020
View reviewed changes
Copy link

@margamanterola margamanterola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Alright, let's see how this works.

@t-lo t-lo merged commit 2383630 into v245-flatcar Sep 10, 2020
@t-lo t-lo deleted the t-lo/remove-rp_filter-wildcard-from-sysctl-defaults branch September 10, 2020 11:54
@t-lo t-lo restored the t-lo/remove-rp_filter-wildcard-from-sysctl-defaults branch September 10, 2020 11:54
t-lo added a commit that referenced this pull request Sep 10, 2020
@t-lo
Merge pull request #11 from flatcar-linux/systemd
912af48 
sysctl.d/50-default.conf: remove *, .all source route settings by t-lo
@t-lo
Copy link
Author

t-lo commented Sep 10, 2020

Merged and cherry-picked for v246-flatcar.

@t-lo t-lo deleted the t-lo/remove-rp_filter-wildcard-from-sysctl-defaults branch September 10, 2020 11:58
@t-lo t-lo mentioned this pull request Sep 10, 2020
Merged
krnowak pushed a commit that referenced this pull request Oct 19, 2020
@keszybz
bus-message: avoid dereferencing a NULL pointer
b7901dd 
We'd try to map a zero-byte buffer from a NULL pointer, which is undefined behaviour.

src/systemd/src/libsystemd/sd-bus/bus-message.c:3161:60: runtime error: applying zero offset to null pointer
    #0 0x7f6ff064e691 in find_part /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-message.c:3161:60
    #1 0x7f6ff0640788 in message_peek_body /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-message.c:3283:16
    #2 0x7f6ff064e8db in enter_struct_or_dict_entry /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-message.c:3967:21
    #3 0x7f6ff06444ac in bus_message_enter_struct /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-message.c:4009:13
    #4 0x7f6ff0641dde in sd_bus_message_enter_container /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-message.c:4136:21
    #5 0x7f6ff0619874 in sd_bus_message_dump /work/build/../../src/systemd/src/libsystemd/sd-bus/bus-dump.c:178:29
    #6 0x4293d9 in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-bus-message.c:39:9
    #7 0x441986 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:558:15
    #8 0x44121e in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/libfuzzer/FuzzerLoop.cpp:470:3
    #9 0x443164 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:770:7
    #10 0x4434bc in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/libfuzzer/FuzzerLoop.cpp:799:3
    #11 0x42d2bc in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:846:6
    #12 0x42978a in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #13 0x7f6fef13c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #14 0x407808 in _start (out/fuzz-bus-message+0x407808)

(cherry picked from commit b17af3e)
krnowak pushed a commit that referenced this pull request Oct 19, 2020
@ssahani @keszybz
network: Fix crash when SendOption= is invalid
2a41fb2 
```
p11-kit-0.23.20-1.fc32.x86_64 pam-1.3.1-26.fc33.x86_64 xz-libs-5.2.5-1.fc33.x86_64 zlib-1.2.11-21.fc32.x86_64
(gdb) bt
    lvalue=0x560e10 "SendOption", ltype=2, rvalue=0x560e1b "11:string", data=0x561e20, userdata=0x561cd0) at ../src/network/networkd-dhcp-common.c:580
    table=0x4392e0 <network_network_gperf_lookup>, section=0x560ef0 "DHCPv4", section_line=14, lvalue=0x560e10 "SendOption", rvalue=0x560e1b "11:string", flags=CONFIG_PARSE_WARN,
    userdata=0x561cd0) at ../src/shared/conf-parser.c:132
    lookup=0x7ffff7d2f76d <config_item_perf_lookup>, table=0x4392e0 <network_network_gperf_lookup>, flags=CONFIG_PARSE_WARN, section=0x7fffffffc9f8, section_line=0x7fffffffc9a0,
    section_ignored=0x7fffffffc99d, l=0x560e10 "SendOption", userdata=0x561cd0) at ../src/shared/conf-parser.c:270
    lookup=0x7ffff7d2f76d <config_item_perf_lookup>, table=0x4392e0 <network_network_gperf_lookup>, flags=CONFIG_PARSE_WARN, userdata=0x561cd0) at ../src/shared/conf-parser.c:395
    lookup=0x7ffff7d2f76d <config_item_perf_lookup>, table=0x4392e0 <network_network_gperf_lookup>, flags=CONFIG_PARSE_WARN, userdata=0x561cd0) at ../src/shared/conf-parser.c:452
    dropin_dirname=0x7fffffffcbd0 "veth99.network.d", sections=0x4f3a18 "Match", lookup=0x7ffff7d2f76d <config_item_perf_lookup>, table=0x4392e0 <network_network_gperf_lookup>,
    flags=CONFIG_PARSE_WARN, userdata=0x561cd0) at ../src/shared/conf-parser.c:511
(gdb) q
A debugging session is active.

	Inferior 1 [process 118718] will be killed.
```

```
$ printf '[DHCPv4]\nSendOption=1:uint8' >crash

$ ./out/fuzz-network-parser ./crash
INFO: Seed: 1158717610
INFO: Loaded 2 modules   (199728 inline 8-bit counters): 136668 [0x7faf3e91a930, 0x7faf3e93bf0c), 63060 [0xadf190, 0xaee7e4),
INFO: Loaded 2 PC tables (199728 PCs): 136668 [0x7faf3e93bf10,0x7faf3eb51cd0), 63060 [0xaee7e8,0xbe4d28),
./out/fuzz-network-parser: Running 1 inputs 1 time(s) each.
Running: ./crash
Assertion 's' failed at src/basic/parse-util.c:458, function int safe_atou8(const char *, uint8_t *)(). Aborting.
==5588== ERROR: libFuzzer: deadly signal
    #0 0x51811e in __sanitizer_print_stack_trace (/home/vagrant/systemd/out/fuzz-network-parser+0x51811e)
    #1 0x46b921 in fuzzer::PrintStackTrace() (/home/vagrant/systemd/out/fuzz-network-parser+0x46b921)
    #2 0x44ded6 in fuzzer::Fuzzer::CrashCallback() (.part.0) (/home/vagrant/systemd/out/fuzz-network-parser+0x44ded6)
    #3 0x44df9d in fuzzer::Fuzzer::StaticCrashSignalCallback() (/home/vagrant/systemd/out/fuzz-network-parser+0x44df9d)
    #4 0x7faf3d6d7b1f  (/lib64/libpthread.so.0+0x14b1f)
    #5 0x7faf3d3c2624 in raise (/lib64/libc.so.6+0x3c624)
    #6 0x7faf3d3ab8d8 in abort (/lib64/libc.so.6+0x258d8)
    #7 0x7faf3e12593a in log_assert_failed_realm /home/vagrant/systemd/build/../src/basic/log.c:819:9
    #8 0x7faf3e140ce1 in safe_atou8 /home/vagrant/systemd/build/../src/basic/parse-util.c:458:9
    #9 0x68089c in config_parse_dhcp_send_option /home/vagrant/systemd/build/../src/network/networkd-dhcp-common.c:517:21
    #10 0x7faf3debed4e in next_assignment /home/vagrant/systemd/build/../src/shared/conf-parser.c:132:32
    #11 0x7faf3deb7783 in parse_line /home/vagrant/systemd/build/../src/shared/conf-parser.c:270:16
    #12 0x7faf3deb606c in config_parse /home/vagrant/systemd/build/../src/shared/conf-parser.c:395:21
    #13 0x7faf3deb85ee in config_parse_many_files /home/vagrant/systemd/build/../src/shared/conf-parser.c:452:21
    #14 0x7faf3deb8c57 in config_parse_many /home/vagrant/systemd/build/../src/shared/conf-parser.c:511:16
    #15 0x57c2eb in network_load_one /home/vagrant/systemd/build/../src/network/networkd-network.c:470:13
    #16 0x543490 in LLVMFuzzerTestOneInput /home/vagrant/systemd/build/../src/network/fuzz-network-parser.c:26:16
    #17 0x44e3e8 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/vagrant/systemd/out/fuzz-network-parser+0x44e3e8)
    #18 0x433505 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/vagrant/systemd/out/fuzz-network-parser+0x433505)
    #19 0x43c449 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/vagrant/systemd/out/fuzz-network-parser+0x43c449)
    #20 0x42c4a6 in main (/home/vagrant/systemd/out/fuzz-network-parser+0x42c4a6)
    #21 0x7faf3d3ad1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #22 0x42c4fd in _start (/home/vagrant/systemd/out/fuzz-network-parser+0x42c4fd)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal

```

(cherry picked from commit 1eb7342)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@margamanterola margamanterola margamanterola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t-lo @margamanterola