ctlplane: set loose reverse path filtering on TAP representors#640
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds an environment-controlled reverse path filtering override. The configuration is parsed and logged at startup, and control-plane TAP creation writes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rjarry
changed the title
|
|
||
| ip link set lo up | ||
| sysctl -qw net.ipv4.conf.all.rp_filter=1 | ||
| sysctl -qw net.ipv4.conf.default.rp_filter=1 |
There was a problem hiding this comment.
In that commit, should we remove the other rp_filter config ?
For example:
smoke/bgp6_srv6_frr_test.sh:ip netns exec bgp-peer sysctl -w net.ipv4.conf.all.rp_filter=0
There was a problem hiding this comment.
These settings are for the bgp-peer netns that does not run grout. Just linux stack. I can try removing them but I think they are needed because of the assymetric vxlan routing.
There was a problem hiding this comment.
The default value is zero on kernel, why always enabled it ?
There was a problem hiding this comment.
Setting it to 2 (loose) is the only way to override anything set by system admins.
| // delivered by grout are not dropped by rp_filter. The effective | ||
| // rp_filter mode is max(conf.all, conf.<iface>), so setting the | ||
| // per-interface value to 2 (loose) overrides a global strict setting. | ||
| snprintf(path, sizeof(path), "/proc/sys/net/ipv4/conf/%s/rp_filter", iface->name); |
There was a problem hiding this comment.
rp_filter is disable by default, why you want to force at loose for tap. -> NACK
There was a problem hiding this comment.
Setting it to 2 (loose) is the only way to override net.ipv4.conf.all=1 set by system admins.
There was a problem hiding this comment.
Just don't put loose when it's disabled in all.
There was a problem hiding this comment.
I cannot know if the admin will enable it later. Why do you care about setting it loose? It has no impact at all. It will just verify there is one route to reach the source address. Since grout always installs a default route via the VRF tun interface, the check passes all the time.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/grout.default`:
- Around line 14-17: The reverse-path filtering override is enabled by default
in both configuration locations. Set GROUT_OVERRIDE_RP_FILTER to false or leave
it unset in main/grout.default, and remove the baked-in environment variable
from Containerfile; deployments requiring the workaround must opt in explicitly
at deployment time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30faf3a6-83b3-43fa-84fd-3093a08e1ddf
📒 Files selected for processing (7)
Containerfiledocs/grout.8.scdocmain/config.hmain/grout.defaultmain/main.cmodules/infra/control/ctlplane.csmoke/_init.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/infra/control/ctlplane.c
- smoke/_init.sh
| # Set loose reverse path filtering on control plane TAP interfaces. | ||
| # Required when net.ipv4.conf.all.rp_filter is set to 1 (strict) in the network | ||
| # namespace where grout is running. | ||
| GROUT_OVERRIDE_RP_FILTER=true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the reverse-path filtering override opt-in.
Both defaults enable rp_filter=2 on control-plane TAP interfaces, weakening strict anti-spoofing validation even when the workaround is unnecessary.
main/grout.default#L14-L17: defaultGROUT_OVERRIDE_RP_FILTERto false/unset.Containerfile#L26-L26: remove the baked-in environment variable; set it only for deployments that enable strict filtering.
📍 Affects 2 files
main/grout.default#L14-L17(this comment)Containerfile#L26-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/grout.default` around lines 14 - 17, The reverse-path filtering override
is enabled by default in both configuration locations. Set
GROUT_OVERRIDE_RP_FILTER to false or leave it unset in main/grout.default, and
remove the baked-in environment variable from Containerfile; deployments
requiring the workaround must opt in explicitly at deployment time.
When rp_filter=1 (strict mode) is enabled globally, the kernel drops packets arriving on TAP interfaces because the reverse path check finds the default route pointing to the TUN loopback rather than the TAP. Add support for a new GROUT_OVERRIDE_RP_FILTER environment variable to control how grout overrides the linux stack configuration. When GROUT_OVERRIDE_RP_FILTER is set to a true-ish value, set rp_filter=2 (loose mode) on each TAP at creation time. The effective rp_filter mode is max(conf.all, conf.<iface>), so the per-interface value of 2 overrides a global strict setting. Loose mode only checks that the source address is reachable via some interface, which always passes since grout has a default route. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Maxime Leroy <maxime@leroys.fr>
Set rp_filter=1 globally in the smoke test init so that all tests run with strict reverse path filtering. This exercises the per-TAP loose mode override set by grout on ctlplane interfaces. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Maxime Leroy <maxime@leroys.fr>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main/grout.default (1)
14-17: 🔒 Security & Privacy | 🟠 MajorKeep the reverse-path filtering override opt-in.
This is the same unresolved security issue from the previous review: enabling
GROUT_OVERRIDE_RP_FILTER=trueby default forces control-plane TAP interfaces to use loose filtering (rp_filter=2), weakening anti-spoofing validation for every deployment. Leave it unset/false here; deployments requiring the workaround should opt in explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main/grout.default` around lines 14 - 17, Change the default GROUT_OVERRIDE_RP_FILTER setting in the configuration to unset or false so reverse-path filtering remains strict by default; deployments needing loose filtering must opt in explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@main/grout.default`:
- Around line 14-17: Change the default GROUT_OVERRIDE_RP_FILTER setting in the
configuration to unset or false so reverse-path filtering remains strict by
default; deployments needing loose filtering must opt in explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5834048d-c203-429c-ae90-7d126e43b398
📒 Files selected for processing (7)
Containerfiledocs/grout.8.scdocmain/config.hmain/grout.defaultmain/main.cmodules/infra/control/ctlplane.csmoke/_init.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- Containerfile
- main/main.c
- main/config.h
- modules/infra/control/ctlplane.c
- smoke/_init.sh
3 participants
When rp_filter=1 (strict mode) is enabled, the kernel does a reverse path FIB lookup on incoming packets to verify that the source address is reachable via the same interface. With only oif rules, this lookup falls through to the main/VRF table where the default route points to the TUN loopback, causing an interface mismatch and dropping the packet.
Add a matching iif rule for each TAP so that the rp_filter FIB lookup hits table 999 and finds the default route via the same TAP the packet arrived on. This does not affect actual routing of return packets which still go through the TUN loopback into the datapath as before.
Update the stale rule flush logic to also collect and clean up iif rules on restart.
Enable rp_filter in all smoke tests to exercise the added iif rules.
smoke/_init.sh: after bringing uplo, sets IPv4 strict reverse path filtering for namespace defaults (net.ipv4.conf.all.rp_filter=1andnet.ipv4.conf.default.rp_filter=1); whenrun_groutis enabled, exportsGROUT_OVERRIDE_RP_FILTER=trueinto the grout environment.Containerfile+main/grout.default: setGROUT_OVERRIDE_RP_FILTER=truein the container/grout defaults (with comments that it’s required whennet.ipv4.conf.all.rp_filteris1/strict where grout runs).docs/grout.8.scdoc: documentsGROUT_OVERRIDE_RP_FILTERand that it configuresrp_filter=2(loose) on control plane TAP interfaces when strict (rp_filter=1) is enabled globally.main: addsgr_config.override_rp_filter, parsed fromGROUT_OVERRIDE_RP_FILTER; when set, emits aNOTICEthat reverse path filtering will be bypassed.modules/infra/control/ctlplane.c: during control-plane TAP creation, whengr_config.override_rp_filteris enabled, writes/proc/sys/net/ipv4/conf/<iface>/rp_filter=2for the TAP (after the existing/keep_addr_on_downsysctl write) and logsWARNINGif therp_filtersysctl can’t be opened/written.modules/infra/control/netlink.c: configures control-plane policy routing using table999for both IPv4 and IPv6 by installing per-TAP policy rules that matchoif=<tap>viaFRA_OIFNAMEand adefault dev <tap>route in table999; on init, flushes stale state for table999by dumpingRTM_GETRULE/RTM_GETROUTE, collecting entries wheretable == 999, and deleting the corresponding policy rules (by savedFRA_OIFNAME) and routes (in table999) to avoid orphan kernel entries across crash/restart cycles.