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.

Sign up for GitHub

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

Jump to bottom

ipsec: Generate from-stack trace for ipsec packets #18608

Merged
merged 4 commits into from
Jan 28, 2022
Merged

ipsec: Generate from-stack trace for ipsec packets #18608

merged 4 commits into from
Jan 28, 2022

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Jan 25, 2022 •
edited
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

When ipsec packets comes from the host (xfrm) stack to from-host program
(bpf_host), it bypasses the send_trace_notify (<- stack). As a result, we
only see the trace for inner packet (ICMPRequest/Reply with <- endpoint
and -> stack). Fix bpf_host program not to bypass the trace.

Trace on source node before fix

<- endpoint ... EchoRequest
-> stack ... EchoRequest

Trace on the source node after fix

<- endpoint ... EchoRequest
-> stack ... EchoRequest
<- stack encrypted ...

This PR also contains some minor fixes related needed to achieving the main goal. Please see commit messages for more details.

Related #14625

Fix the bug that ipsec packets bypass the <- stack trace after encryption

@YutaroHayakawa YutaroHayakawa requested a review from a team January 25, 2022 11:33
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 25, 2022
@YutaroHayakawa YutaroHayakawa marked this pull request as draft January 25, 2022 11:35
@YutaroHayakawa
Copy link
Member Author

/test

YutaroHayakawa referenced this pull request in YutaroHayakawa/cilium Jan 25, 2022
@YutaroHayakawa
ipsec: Generate Generate to-<somewhere> trace for ipsec packets
c8e15b9 
When ipsec encrypted packets come to the from-host program (bpf_host),
in case of the native routing mode, it will be handled by
do_netdev_encrypt, then redirected to the network-facing device when the
fib_lookup is available. Otherwise, it will be sent to stack for
routing. There are three possible packet paths.

1. fib_lookup available + to-netdev program attached to the dest device

In this case, to-netdev program may drop packet with host firewall. So,
we shouldn't generate TRACE_TO_NETWORK trace before seeing the verdict.
Thus, trace should be generated from to-netdev program.

2. fib_lookup available + to-netdev program is not attached

In this case, packet will go to the network without involving BPF
program. Thus, we should generate trace in the context of from-host
program.

3. Packet goes to stack and route

Unlike above two, packet goes to stack. In this case, TRACE_TO_STACK
should be generated. However, we don't have to generate the trace from
from-host program, because if we do CTX_ACT_OK from from-host program,
the packet pass through veth and hooked into to-host program
immediately. In to-host program, there is a TRACE_TO_STACK.

Trace on source node before fix

```
<- endpoint ... EchoRequest
-> stack ... EchoRequest
<- host encrypted ...
```

Trace on the source node after fix

```
<- endpoint ... EchoRequest
-> stack ... EchoRequest
<- host encrypted ...
-> network encryped ...
```

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the issue-14625-1 branch 3 times, most recently from 6a43a8f to ae6868c Compare January 26, 2022 02:16
@YutaroHayakawa YutaroHayakawa mentioned this pull request Jan 26, 2022
Closed
6 tasks
@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa
Copy link
Member Author

Known flakes

@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review January 26, 2022 07:31
@YutaroHayakawa
Copy link
Member Author

/ci-multicluster

@YutaroHayakawa
Copy link
Member Author

/ci-gke

@pchaigno pchaigno self-requested a review January 26, 2022 08:51
@pchaigno pchaigno added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jan 26, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 26, 2022
@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 26, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 26, 2022
@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 26, 2022
@YutaroHayakawa
Copy link
Member Author

Oops, sorry I found the change in this PR causes the packet looping in tunnel mode. Let me put this PR back to draft.

@YutaroHayakawa
bpf: Extend inherit_identity_from_host to handle encrypt case
c32e3ac 
Current inherit_identity_from_host cannot inherit identity from
encrypted packets (packets with MARK_MAGIC_ENCRYPT coming from host
stack). Extend it to handle it.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
bpf: Extend inherit_identity_from_host to preserve mark magics
252dd15 
Currently, return value of inherit_identity_from_host indicates the
packet is coming from proxy or not. However, in some cases, we want to
know whether the packet was encrypted or not (and it is impossible to
know it after the call since inherit_identity_from_host wipes the mark).
To cover both of the cases, we can just extend it to return magic value
extracted from the mark.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
ipsec: Encode src_id correctly in ipsec + native routing
6e9fb64 
Currently, when packets passed from from-container (bpf_lxc) to host
stack, src_id is not encoded to the mark. It is inconvenient for rest of
the datapath especially when we generate the trace.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa YutaroHayakawa changed the title ipsec: Generate from-host trace for ipsec packets ipsec: Generate from-stack trace for ipsec packets Jan 28, 2022
pchaigno
pchaigno approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

The changes look good to me!

Fix the bug that ipsec packets bypass the <- stack trace in bpf_host (from-host)

Watch out for references to internals of Cilium in release notes. Users probably won't know what bpf_host and from-host refer to.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.2 Jan 28, 2022
@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa
ipsec: Generate from-stack trace for ipsec packets
a1734d6 
When ipsec packets comes from the host (xfrm) stack to from-host program
(bpf_host), it bypasses the send_trace_notify (<- stack). As a result, we
only see the trace for inner packet (ICMPRequest/Reply with <- endpoint
and -> stack). Fix bpf_host program not to bypass the trace.

Note that since we moved do_netdev_encrypt after
inherit_identity_from_host, we cannot extract identity from ctx->mark
anymore, because inherit_identity_from_host wipes the mark. That's why
we need to pass inherited identity to do_netdev_encrypt. Otherwise, in
tunneling mode, get_identity doesn't work correctly.

Trace on source node before fix

```
<- endpoint ... EchoRequest
-> stack ... EchoRequest
```

Trace on the source node after fix

```
<- endpoint ... EchoRequest
-> stack ... EchoRequest
<- stack encrypted ...
```

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Copy link
Member Author

Fixed too long line warning from checkpatch.pl.

@YutaroHayakawa
Copy link
Member Author

/test

joamaki
joamaki approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

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

looks ok to me, but please double-check the use of ctx->mark.

bpf/bpf_host.c Show resolved Hide resolved
@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 Jan 28, 2022
@glibsm glibsm merged commit 144e9e0 into cilium:master Jan 28, 2022
@glibsm glibsm mentioned this pull request Jan 31, 2022
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.2 Jan 31, 2022
@pchaigno pchaigno mentioned this pull request Feb 1, 2022
Merged
@joamaki joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.2 Feb 8, 2022
@joestringer joestringer mentioned this pull request Feb 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.11.2
Backport done to v1.11
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@YutaroHayakawa @joamaki @pchaigno @glibsm