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

bpf: minor HostFW cleanups #25881

Merged
merged 3 commits into from Jun 5, 2023
Merged

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Jun 5, 2023

Some innocent cleanups from poking around in the HostFW paths.

Combine some ENABLE_HOST_FIREWALL sections, and limit the definition of
relevant variables.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
cil_to_netdev() only uses the `proto` variable when the hostFW is enabled.

handle_to_netdev_ipv6() is already wrapped in ENABLE_HOST_FIREWALL, no need
to also check it inside the function.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Jun 5, 2023
This feels rather bogus, the .node_port flag should only ever be used by
the nodeport code. We can also trust that ct_buffer.ct_state.node_port is
never actually set:
- the callers of ipv*_host_policy_ingress_lookup() 0-initialize the whole
  ct_buffer struct, and
- the CT lookup returned CT_NEW, thus ct_buffer.ct_state is unchanged.

So just remove this code.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title WIP HostFW bpf: minor HostFW cleanups Jun 5, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review June 5, 2023 10:37
@julianwiedmann julianwiedmann requested a review from a team as a code owner June 5, 2023 10:37
from_host_raw = ctx_load_meta(ctx, CB_FROM_HOST);
ctx_store_meta(ctx, CB_FROM_HOST, 0);
#endif /* ENABLE_HOST_FIREWALL */

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want this block to be above any potential early return, so that we don't leak any non-zero metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the only preceding return is a drop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, yes, I guess it should be safe then.

I was under impression we try to clear metadata the first thing in the function for extra robustness, but it doesn't look problematic in this case.

from_host_raw = ctx_load_meta(ctx, CB_FROM_HOST);
ctx_store_meta(ctx, CB_FROM_HOST, 0);
#endif /* ENABLE_HOST_FIREWALL */

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 5, 2023
@dylandreimerink dylandreimerink merged commit 6e9d5e0 into cilium:main Jun 5, 2023
61 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-bpf-hostfw branch June 5, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants