network: several cleanups and fix IPv4DAD and IP Masqurade #17240

yuwata · 2020-10-04T02:15:22Z

Most commits should be trivial, they move functions from networkd-link.c or networkd-manager.c to relevant files, or drop redundant or unused entries.

I hope only last few commits fix some minor issues.

Its error handling was bogus...

keszybz

Looks very nice.

I made a few comments that follow the overall pattern of renaming foo_verify_bar to foo_drop_invalid_bar. This is a suggestion, and if you think _verify_ is OK, I'm fine with that. If you decide to rename them, I think it'd be better to do it in a single commit at the end, to avoid nasty rebasing and patch failures.

src/network/networkd-routing-policy-rule.c

src/network/networkd-routing-policy-rule.h

keszybz · 2020-10-06T08:26:00Z

src/libsystemd/sd-netlink/netlink-util.h

        })

-#define netlink_add_match(nl, ret_slot, metch, callback, destroy_callback, userdata) \
+#define netlink_add_match(nl, ret_slot, match, callback, destroy_callback, userdata, description) \


Hmm, but match was referenced below? Did we happen to always have a match variable in the scope?

I was introduced the macro in the past, but it is never used. So, I did not notice the typo...

src/network/networkd-routing-policy-rule.c

src/network/networkd-address.c

keszybz · 2020-10-06T15:24:19Z

src/network/networkd-address.c

+                                          address->section->filename, address->section->line);
+
                address->scope = RT_SCOPE_HOST;
+        }


if (in_addr_is_localhost(address->family, &address->in_addr) > 0) { if (address->family == AF_INET && address->scope_set && address->scope != RT_SCOPE_HOST) log_warning_errno(SYNTHETIC_ERRNO(EINVAL), "%s: non-host scope is set in the [Address] section from line %u. " "Ignoring Scope= setting.", address->section->filename, address->section->line); address->scope = RT_SCOPE_HOST; address->scope_set = true; }

?

My change is intended the following:
For IPv4, scope should be always RT_SCOPE_HOST.
For IPv6, use RT_SCOPE_HOST when scope is not explicitly specified.

I keep the code, and added the above comments in the code.

test/test-network/conf/25-sriov.network

And reduce scope of variables.

[RoutingPolicyRule] sections are managed by both LIST and Hashmap. Let's drop list.

…rule.[ch]

Previously, IPv4 DAD is configured in each Address object stored in Network object. If a .network file matches multipe links, then it causes an assertion. To prevent it, now IPv4 DAD is configured in each Address object belogs to Link object.

When the MAC address of a link is updated, an address on the link may be under checking address duplication. Or, (currently such code is not implemented yet, but) address duplication check may be restarted later. For that case, the IPv4 ACD clients must use the new updated MAC address.

For IPv6 case, use RT_SCOPE_HOST only when scope is not explicitly specified.

Previously, address_establish() took Address object stored in Network object. And address_release() took Address object stored in Link object. Thus, address_release() always did nothing.

As 'verify' implies a boolean result.

We usually disable IPv6AcceptRA= if the test does not require any dynamic address configuration, as it makes slightly slow down the test. C.f. 491b79a.

yuwata · 2020-10-06T18:43:42Z

@keszybz Thank you for reviewing such a huge PR. Most suggestions are applied. For others, please see the above comments. PTAL.

keszybz · 2020-10-06T20:29:13Z

LGTM.

The usual: "empty string" is meaningless in this context. We are not assigning DestinationPort="". Just say "unset". Fixes systemd#17240.

When set to "kernel", systemd is not supposed to touch that sysctl. e0534f1c13cd50ec2b143a8b18156cd37e502f7, part of systemd#17240 forgot to handle that case. Fixes systemd#18003

When set to "kernel", systemd is not supposed to touch that sysctl. 5e0534f, part of systemd#17240 forgot to handle that case. Fixes systemd#18003

The usual: "empty string" is meaningless in this context. We are not assigning DestinationPort="". Just say "unset". Fixes systemd#17240. (cherry picked from commit e6fd398)

When set to "kernel", systemd is not supposed to touch that sysctl. 5e0534f, part of systemd/systemd#17240 forgot to handle that case. Fixes systemd/systemd#18003 (cherry picked from commit d3ccb1b)

yuwata added network sd-netlink labels Oct 4, 2020

yuwata force-pushed the network-cleanup branch 8 times, most recently from 3518bd8 to 4170786 Compare October 4, 2020 21:57

yuwata added 4 commits October 6, 2020 00:51

network: move link_request_set_routing_policy()

c0ec474

network: make several functions static

7532b88

network: drop routing_policy_rule_make_local()

ddc9df3

Its error handling was bogus...

network: reduce scope of variables

eb3445c

yuwata force-pushed the network-cleanup branch 2 times, most recently from 2af5f84 to ba4677f Compare October 5, 2020 21:36

keszybz closed this in e6fd398 Oct 6, 2020

yuwata reopened this Oct 6, 2020

keszybz reviewed Oct 6, 2020

View reviewed changes

keszybz added the good-to-merge/with-minor-suggestions label Oct 6, 2020

yuwata added 9 commits October 7, 2020 02:36

network: add more logs on loading routing policy rules failure

816ef8d

And reduce scope of variables.

network: make routing_policy_rule_free() returns NULL

b0815fa

network: do not duplicate rule if Family=both

02e9f4e

network: drop list of static routing policy rules

ca183bf

[RoutingPolicyRule] sections are managed by both LIST and Hashmap. Let's drop list.

network: move log_message_warning_errno() to networkd-util.h

0d7d377

sd-netlink: fix typo and make netlink_add_match() take description

3d65f7c

network: use typesafe macro netlink_add_match()

8dfed23

network: move manager_rtnl_process_rule() to networkd-routing-policy-…

51a0dc4

…rule.[ch]

network: make routing_policy_rule_new() static

f1828a2

yuwata added 12 commits October 7, 2020 02:54

network: introduce address_copy()

cde1f0e

network: constify one argument

693ec5c

network: always use RT_SCOPE_HOST for IPv4 loopback addresses

cd1caf3

For IPv6 case, use RT_SCOPE_HOST only when scope is not explicitly specified.

network: fix masquerade setting logic

494b6b4

Previously, address_establish() took Address object stored in Network object. And address_release() took Address object stored in Link object. Thus, address_release() always did nothing.

network: rename network_verify_xxx() -> network_drop_invalid_xxx()

13ffa39

As 'verify' implies a boolean result.

test-network: drop duplicated address

6dcc637

test-network: add missing file in the list

5cf52d9

test-network: disable RA in test_sriov

47f8411

We usually disable IPv6AcceptRA= if the test does not require any dynamic address configuration, as it makes slightly slow down the test. C.f. 491b79a.

test-network: add a missing netdev in the list

713bfda

test-network: also remove IPv6 rules

6649345

yuwata force-pushed the network-cleanup branch from ba4677f to 6649345 Compare October 6, 2020 18:24

keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Oct 6, 2020

yuwata merged commit ab582fd into systemd:master Oct 6, 2020

yuwata deleted the network-cleanup branch October 6, 2020 22:42

yuwata added the hacktoberfest-accepted label Oct 7, 2020

yuwata mentioned this pull request Oct 12, 2020

network: directly compare with in_addr element for IPv4 case systemd/systemd-stable#77

Merged

andersk mentioned this pull request Dec 16, 2020

networking.networkd.privacy test broken by systemd 247 upgrade NixOS/nixpkgs#106858

Closed

flokli mentioned this pull request Dec 16, 2020

networkd: IPv6PrivacyExtensions=kernel doesn't honor kernel setting #18003

Closed

flokli mentioned this pull request Dec 23, 2020

network: fix IPv6PrivacyExtensions=kernel #18069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

network: several cleanups and fix IPv4DAD and IP Masqurade #17240

network: several cleanups and fix IPv4DAD and IP Masqurade #17240

Uh oh!

yuwata commented Oct 4, 2020 •

edited

Loading

Uh oh!

keszybz left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz Oct 6, 2020

Uh oh!

yuwata Oct 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz Oct 6, 2020

Uh oh!

yuwata Oct 6, 2020 •

edited

Loading

Uh oh!

Uh oh!

yuwata commented Oct 6, 2020 •

edited

Loading

Uh oh!

keszybz commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

network: several cleanups and fix IPv4DAD and IP Masqurade #17240

network: several cleanups and fix IPv4DAD and IP Masqurade #17240

Uh oh!

Conversation

yuwata commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

yuwata Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

yuwata Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuwata commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

yuwata commented Oct 4, 2020 •

edited

Loading

yuwata Oct 6, 2020 •

edited

Loading

yuwata commented Oct 6, 2020 •

edited

Loading